Add arbitrary property to HeaderMap for type differentiation
#7314
Conversation
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 450fa85:
|
.changeset/great-kids-visit.md
Outdated
| '@apollo/server': patch | ||
| --- | ||
|
|
||
| Add an `__identity` property to `HeaderMap` class to disallow standard `Map`s (in TypeScript) |
There was a problem hiding this comment.
Maybe this should be more like "Ensure via typechecking that fields declared to take HeaderMap (notably, the httpGraphQLRequest.headers option to ApolloServer.executeHTTPGraphQLRequest and the http.headers option to ApolloServer.executeOperation) actually use a HeaderMap rather than allowing other Map<string, string>s. This may be a breaking change for framework integrations, but it is easily fixed by switching from new Map<string, string>() to new HeaderMap() and those integrations previously had bugs" or something.
| @@ -1,4 +1,8 @@ | |||
| export class HeaderMap extends Map<string, string> { | |||
| // In order for TypeScript to prevent a standard `Map` from being compatible | |||
| // with a `HeaderMap`, we need some additional property on the class. | |||
| __identity = Symbol('HeaderMap'); | |||
There was a problem hiding this comment.
Can this be private? (Guessing no.)
Can this change be tested with a @ts-expect-error?
There was a problem hiding this comment.
(also re "can this be private", guessing this might even be a weird thing where if made private it sorta works within our ts project but then works differently when published as DTS.)
| @@ -837,7 +837,7 @@ describe('request pipeline life-cycle hooks', () => { | |||
| message: 'Syntax Error: Expected Name, found "}".', | |||
| extensions: { | |||
| code: 'GRAPHQL_PARSE_FAILED', | |||
| http: { status: 400, headers: new HeaderMap() }, | |||
| http: { status: 400, headers: expect.any(HeaderMap) }, | |||
There was a problem hiding this comment.
I actually don't understand why this worked before or why my change required this test update, but this is what we should've had here to begin with.
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/server-gateway-interface@1.1.0 ### Minor Changes - [#7325](#7325) [`e0f959a63`](e0f959a) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Add optional `nonFtv1ErrorPaths` to Gateway metrics data. This change is a prerequisite to: - <apollographql/federation#2242> - <#7136> ## @apollo/server-integration-testsuite@4.3.2 ### Patch Changes - [#7316](#7316) [`37d884650`](37d8846) Thanks [@renovate](https://github.com/apps/renovate)! - Update graphql-http dependency - Updated dependencies \[[`f246ddb71`](f246ddb), [`e25cb58ff`](e25cb58)]: - @apollo/server@4.3.2 ## @apollo/server@4.3.2 ### Patch Changes - [#7314](#7314) [`f246ddb71`](f246ddb) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Add an `__identity` property to `HeaderMap` class to disallow standard `Map`s (in TypeScript). This ensures that typechecking occurs on fields which are declared to accept a `HeaderMap` (notably, the `httpGraphQLRequest.headers` option to `ApolloServer.executeHTTPGraphQLRequest` and the `http.headers` option to `ApolloServer.executeOperation`). This might be a breaking change for integration authors, but should be easily fixed by switching from `new Map<string, string>()` to `new HeaderMap()`. - [#7326](#7326) [`e25cb58ff`](e25cb58) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Pin `node-abort-controller` version to avoid breaking change. Apollo Server users can enter a broken state if they update their package-lock.json due to a breaking change in a minor release of the mentioned package. Ref: <southpolesteve/node-abort-controller#39> - Updated dependencies \[[`e0f959a63`](e0f959a)]: - @apollo/server-gateway-interface@1.1.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Related: #7313
This shouldn't have been allowed by the typings in the first place. Previously, a
Mapwas compatible withHeaderMapaccording to TypeScript, so the error wasn't a typing error (when we wanted it to be). Adding an arbitrary class property to theHeaderMapclass gives us the typing error we'd expect when aMapis used in lieu of aHeaderMap.