CODEGEN-792 - @graphql-codegen/graphql-request - Allow signal request options #1116
Conversation
🦋 Changeset detectedLatest commit: a1a80fc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| o.operationVariablesTypes | ||
| }, requestHeaders?: GraphQLClientRequestHeaders): Promise<${o.operationResultType}> { | ||
| }, requestHeaders?: GraphQLClientRequestHeaders, signal?: RequestInit['signal']): Promise<${o.operationResultType}> { | ||
| return withWrapper((wrappedRequestHeaders) => client.request<${ | ||
| o.operationResultType | ||
| }>(${docVarName}, variables, {...requestHeaders, ...wrappedRequestHeaders}), '${operationName}', '${operationType}', variables); | ||
| }>({ document: ${docVarName}, variables, requestHeaders: { ...requestHeaders, ...wrappedRequestHeaders }, signal }), '${operationName}', '${operationType}', variables); | ||
| }`; |
There was a problem hiding this comment.
Are you familiar with how the generated SDK work, and could help me do a sense check please @jasonkuhrt?
Given a doc like this:
const AddDocument = gql`
query Add($x: Int!, $y: Int!) {
add(x: $x, y: $y)
}
`;Currently, it's generating a function that takes in variables and requestHeaders as positional params, then uses client.request's positional param like this:
client.request<AddQuery>(
AddDocument,
variables,
{...requestHeaders, ...wrappedRequestHeaders }
);This PR changes it to use single object param like this:
client.request<AddQuery>({
document: AddDocument,
variables,
requestHeaders: { ...requestHeaders, ...wrappedRequestHeaders },
signal
})2 questions from me:
- For
graphql-request@7, I can see that there are only 4 possible keuys for the object style, is this correct? - Do you see any issues with this approach? I'm mindful of v8 development so I don't want to change how v7 SDK work too much to avoid breaking changes.
There was a problem hiding this comment.
Hey, not familiar with this generator myself. IIUC your object there with 4 keys is I guess correct. I would trust whatever the TS types say. Its weird to me that signal gets shoved in there but I suspect it was added just to unblock people and keep things moving at some point.
In terms of issues with approach, hard for me to say having been distant from v7 for a while and not familiar with this generator so can't help much off hand with that. But if you make a quick test and it seems to be working, then that's a decent confirmation that it should be working in other cases too.
Sorry to not be more helpful!
There was a problem hiding this comment.
Thanks @jasonkuhrt ! All good! Appreciate the context and advices.
I've done a few tests and it's working fine, will merge this 🙏
|
FYI in my case, this was a breaking change, as the signal interface is not compatible with the interface of @types/node so if someone else gets the error |
Also ran into this and was therefore unable to update to |
Description
Related: #1017
client.requestcan an object as the first param with 4 key-value pairs:document: the GraphQL docvariables: the GraphQL operation variablesrequestHeaders: the request headers for the requestsignal:AbortSignalto set request's signalPreviously, the
document,variablesandrequestHeaderswere passed in as positional params, which didn't allow forsignalto be passed in. This PR changes that:signalas the last param for each SDK functionType of change
Note: this could be a breaking change in tests if consumers try to spy on the
client.requestfunction and try to assert said function to be called with certain params.However, there's no functional changes in terms of the public interface i.e. if users just test using the SDK functions, it'd still work, so I think this is a minor instead of a breaking change.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can
reproduce. Please also list any relevant details for your test configuration