-
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: exclude operation name via a field in RequestConfig #645
feat: exclude operation name via a field in RequestConfig #645
Conversation
README.md
Outdated
### IgnoreOperationName | ||
|
||
By default the GraphQLClient tries to extract the operationName from the document. | ||
You can define `ignoreOperationName` in the constructor of GraphQLClient to avoid the extraction process if it is not needed. | ||
|
||
```ts | ||
const client = new GraphQLClient(endpoint, { ignoreOperationName: true }) | ||
``` | ||
|
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.
Could you explain the following:
- Example of document sent, ignored vs not ignored
- Why this matters
Links to other information sources are fine.
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 have created a new commit with an updated readme that I hope answers your questions. Feel free to ask me anything, I will be happy to answer.
Thanks @robertobadalamenti, left a comment on the docs before diving into the rest. |
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.
Thanks for the effort! Two things:
- Can you show an example of how the GraphQL document sent over the wire changes, if at all. (if nothing, ignore this point)
- Please write a test
After that we can merge. Thanks!
I made a new commit with the tests for the ignoreOperationName flag. While below are the logs with the two different configurations for ignoreOperationName. I hope it answers your requests, in case you don't hesitate to ask me anything else. current case i.e. not ignoring the operationName const client = new GraphQLClient(context.base_url, {
method: "POST",
errorPolicy: "ignore",
requestMiddleware: requestMiddleware,
});
//or equivalently
const client = new GraphQLClient(context.base_url, {
method: "POST",
errorPolicy: "ignore",
ignoreOperationName: false, // <--
requestMiddleware: requestMiddleware,
}); related log request {
method: 'POST',
headers: _HeadersList {
cookies: null,
[Symbol(headers map)]: Map(7) {
'graphql-request-hash' => [Object],
'accept' => [Object],
'content-type' => [Object]
},
[Symbol(headers map sorted)]: null
},
body: '{"query":"query GetPage($id: ID!) { getPage(id: $id) { lastModified } }","variables":{"id":"/"},"operationName":"GetPage"}',
errorPolicy: 'ignore',
url: 'https://your_shema_url.it',
operationName: 'GetPage',
variables: { id: '/' }
}` instead by setting the new "ignoreOperationName" flag to true const client = new GraphQLClient(context.base_url, {
method: "POST",
errorPolicy: "ignore",
ignoreOperationName: true, //<--
requestMiddleware: requestMiddleware,
}); related log request {
method: 'POST',
headers: _HeadersList {
cookies: null,
[Symbol(headers map)]: Map(7) {
'graphql-request-hash' => [Object],
'accept' => [Object],
'content-type' => [Object]
},
[Symbol(headers map sorted)]: null
},
body: '{"query":"query GetPage($id: ID!) { getPage(id: $id) { lastModified } }","variables":{"id":"/"}}',
errorPolicy: 'ignore',
url: 'https://your_shema_url.it',
operationName: undefined,
variables: { id: '/' }
} thank you for your support. |
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.
Looking good. I think ignore...
is the wrong term here, as that suggests not paying attention to something received. In fact, this is about not sending something to the server. Let's call this includeOperationName
or something (will require flipping the boolean value in that case, since the boolean sense is reversed).
Also, please rebase.
The idea was to use a negative term so that it is backwards compatible, i.e. if it is not defined, the behaviour remains as it is now. What do you think about exclude...? |
@robertobadalamenti |
I did the commit for 'exclude...' and rebase |
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.
ty!
Just some checks to fix then I can merge!
I have solved the checks |
@robertobadalamenti There's some kind of formatting check issue, but, I won't bother you with that. Will merge and fix myself later. Thanks for your dedication on this PR! |
No description provided.