Skip to content
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

Merged
merged 4 commits into from
Jan 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changeset/great-kids-visit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
'@apollo/server': patch
---

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()`.
8 changes: 5 additions & 3 deletions docs/source/integrations/building-integrations.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ With the request body parsed, we can now construct an `HTTPGraphQLRequest`:
```ts
interface HTTPGraphQLRequest {
method: string;
headers: Map<string, string>;
headers: HeaderMap; // the `HeaderMap` class is exported by @apollo/server
search: string;
body: unknown;
}
Expand All @@ -164,7 +164,9 @@ Finally, we have to create the `headers` property because Apollo Server expects
In the Express integration, we construct a `Map` by iterating over the `headers` object, like so:

```ts
const headers = new Map<string, string>();
import { HeaderMap } from '@apollo/server';

const headers = new HeaderMap();
for (const [key, value] of Object.entries(req.headers)) {
if (value !== undefined) {
headers.set(key, Array.isArray(value) ? value.join(', ') : value);
Expand Down Expand Up @@ -221,7 +223,7 @@ After awaiting the Promise returned by `executeHTTPGraphQLRequest`, we receive a
```ts
interface HTTPGraphQLHead {
status?: number;
headers: Map<string, string>;
headers: HeaderMap;
}

type HTTPGraphQLResponseBody =
Expand Down
4 changes: 2 additions & 2 deletions packages/server/src/__tests__/runQuery.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) },
Copy link
Member Author

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.

},
}),
]),
Expand All @@ -862,7 +862,7 @@ describe('request pipeline life-cycle hooks', () => {
'Cannot query field "testStringWithParseError" on type "QueryType".',
extensions: {
code: 'GRAPHQL_VALIDATION_FAILED',
http: { status: 400, headers: new HeaderMap() },
http: { status: 400, headers: expect.any(HeaderMap) },
},
}),
]),
Expand Down
5 changes: 5 additions & 0 deletions packages/server/src/utils/HeaderMap.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
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.
// @ts-ignore (this is just unused)
private __identity = Symbol('HeaderMap');

override set(key: string, value: string): this {
return super.set(key.toLowerCase(), value);
}
Expand Down