New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems goo
.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be private? (Guessing no.)
Can this change be tested with a @ts-expect-error
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Map
was compatible withHeaderMap
according to TypeScript, so the error wasn't a typing error (when we wanted it to be). Adding an arbitrary class property to theHeaderMap
class gives us the typing error we'd expect when aMap
is used in lieu of aHeaderMap
.