Typescript: New error: This spread always overwrites this property

[amcasey]

I believe the error is both correct and valuable, but filing it for completeness, since it wasn't produced by 3.8.

https://github.com/apollographql/apollo-server/blob/c177acd7959aaf516f8ac814f1a6bb2e6d3ed2e6/packages/apollo-server-core/src/ApolloServer.ts#L775

packages/apollo-server-core/src/ApolloServer.ts:775:7 - error TS2783: 'context' is specified more than once, so this usage will be overwritten.

775       context,
          ~~~~~~~

  packages/apollo-server-core/src/ApolloServer.ts:787:7
    787       ...this.requestOptions,
              ~~~~~~~~~~~~~~~~~~~~~~
    This spread always overwrites this property.

To repro:
1) npm ci
2) tsc -b -f tsconfig.build.json

[sethlowie]

I'm not sure this error is correct. I have the use case of wanting to overwrite the properties. I have to use Object.assign which compiles fine, even though it's the same behavior. This change is forcing users to choose a specific syntax for the same behavior.

[amcasey]

@sethlowie I'm not sure I follow - do you have an example? In this case, it looks like the user thought specifying context was going to have some effect, but it actually won't because it will always be clobbered by the property spread from requestOptions. I think the intended fix is just to move the overriding context after the spread.

[sethlowie]

In this example, I can place a script tag in the head of my HTML document and use that to enable/disable specific combinations of feature flags for testing locally and in CI. Changing this to use MockFlags: Partial<Flags>; fixes the type error. So maybe the issue here is that using Object.assign instead of spreading doesn't throw an error.

type Flags = {
  featureA: boolean;
  featureB: boolean;
}

interface Window {
  MockFlags: Flags;
}

window.MockFlags = {
  featureA: false,
  featureB: false,
  ...(window.MockFlags || {}),
}

[amcasey]

So maybe the issue here is that using Object.assign instead of spreading doesn't throw an error.

It does seem like they should be consistent.

In your example, it looks like MockFlags should be optional? Otherwise, the || {} will never do anything, will it? As written, it seems to guarantee that both featureA and featureB will be present on window.MockFlags and will override your explicit values.

[sethlowie]

👍 Partial and optional both work. I was thrown by Object.assign not throwing the error.

Here's an example that better demonstrates the inconsistency between spread and Object.assign

type Flags = {
  featureA: boolean;
  featureB: boolean;
}

interface Window {
  MockFlags: Flags;
}

window.MockFlags = {
  featureA: false,
  featureB: false,
  ...window.MockFlags,
}

window.MockFlags = Object.assign({
  featureA: false,
  featureB: false,
}, window.MockFlags);

[amcasey]

It's unfortunate that they don't agree, but I'm not sure there's a good way around that. Since Object.assign is just a function (vs a syntactic construct, like spread), there's no way to know for sure which functions are actually performing assignment. For example, I could make an alias const oa = Object.assign; or I could redefine Object.assign to do something else and there'd be no practical way for the compiler to detect those cases.