-
Notifications
You must be signed in to change notification settings - Fork 311
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: TypedDocumentNode support & strict variable typings #350
feat: TypedDocumentNode support & strict variable typings #350
Conversation
Will try to look at this next week. |
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.
Let's add some tests that show there are no type errors where we don't expect them and vice versa. You've done some or all of that work, but at least it needs to be moved.
|
||
export type BatchRequestsOptions<V = Variables> = { | ||
documents: BatchRequestDocument<V>[] | ||
requestHeaders?: Dom.RequestInit['headers'] | ||
signal?: Dom.RequestInit['signal'] | ||
} | ||
|
||
export type RequestExtendedOptions<V = Variables> = { url: string } & RequestOptions<V> | ||
export type RequestExtendedOptions<V = Variables, T = any> = { url: string } & RequestOptions<V, T> |
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.
Let's rename T
to what it is.
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.
Hi @jasonkuhrt,
I'm working with @n1ru4l at The Guild, making this PR move forward.
Regarding your point, the T
naming is used everywhere for the result type, do you want me to change it everywhere or just there?
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.
Hey, everywhere I guess.
examples/typed-document-node.ts
Outdated
console.log(data.greetings) | ||
})().catch((error) => console.error(error)) | ||
|
||
/** TypeScript API Tests */ |
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.
Move this to the test suite sommewhere.
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.
done in 362cc9b
@jasonkuhrt, I ran Do all the format changes in this PR looks ok, or should I revert them? |
It is fine, thanks! Yeah we need a format check in CI. |
Thanks, @jasonkuhrt! ⚡ |
This PR adds support for
TypedDocumentNode
which is already adopted within clients such as urql or apollo and is widely used by people through@graphql-codegen/typed-document-node
and@graphql-codegen/gql-tag-operations-preset
.Resources: