-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(graphiql-toolkit): accept HeadersInit #3854
base: main
Are you sure you want to change the base?
feat(graphiql-toolkit): accept HeadersInit #3854
Conversation
|
I will add a changeset once I receive feedback that the changes will be accepted. |
const fetcherOptsHeadersEntries: [string, string][] = [ | ||
...fetcherOptsHeaders.entries(), | ||
]; | ||
// todo: If there are headers with multiple values, they will be lost. Is this a problem? |
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 am not familiar with graphql-ws
and have not investigated what it does with these key-values yet.
Can someone advise what the correct logic would be here? I am currently apply a lossy approach, but I do not know if that is ok here.
const fetcherOptsHeaders = new Headers(fetcherOpts?.headers ?? {}); | ||
// @ts-expect-error: Current TS Config target does not support `Headers.entries()` method. | ||
// However it is reported as "widely available" and so should be fine to use. This could | ||
// would be more complicated without it. | ||
// @see https://developer.mozilla.org/en-US/docs/Web/API/Headers/entries | ||
const fetcherOptsHeadersEntries: [string, string][] = [ | ||
...fetcherOptsHeaders.entries(), | ||
]; |
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.
Ideally this isn't needed. However I don't want to open a pandora's box by changing the tsconfig.json
file.
This PR enhances
graphiql-toolkit
package fetcher utility to acceptHeadersInit
type instead ofRecord<string, string>
in places where headers are accepted.This is a backwards compatible change because
Record<string, string>
is a sub-type ofHeadersInit
. As well, care in this PR has been taken to preserve the way in which same-name headers from different sources would overwrite, not merge, together.The benefit of this change is that consumers can now pass values of
Headers
or[string,string][]
type which permits the possibility of a header repeated multiple, which HTTP supports.In practice a place this issue became relevant was in my work on Hive Console's Laboratory Preflight Script feature:
graphql-hive/console#6378 (comment)