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

feat: exclude operation name via a field in RequestConfig #645

Merged

Conversation

robertobadalamenti
Copy link
Contributor

No description provided.

README.md Outdated
Comment on lines 156 to 164
### 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 })
```

Copy link
Member

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.

Copy link
Contributor Author

@robertobadalamenti robertobadalamenti Jan 19, 2024

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.

@jasonkuhrt
Copy link
Member

Thanks @robertobadalamenti, left a comment on the docs before diving into the rest.

Copy link
Member

@jasonkuhrt jasonkuhrt left a 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:

  1. Can you show an example of how the GraphQL document sent over the wire changes, if at all. (if nothing, ignore this point)
  2. Please write a test

After that we can merge. Thanks!

@robertobadalamenti
Copy link
Contributor Author

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.

Copy link
Member

@jasonkuhrt jasonkuhrt left a 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.

@robertobadalamenti
Copy link
Contributor Author

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...?
If I reverse the logic by using include..., if the flag is not defined, do we still include it?

@jasonkuhrt
Copy link
Member

@robertobadalamenti exclude... works yep!

@robertobadalamenti
Copy link
Contributor Author

I did the commit for 'exclude...' and rebase

Copy link
Member

@jasonkuhrt jasonkuhrt left a 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!

@robertobadalamenti
Copy link
Contributor Author

robertobadalamenti commented Feb 1, 2024

I have solved the checks

@jasonkuhrt
Copy link
Member

@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!

@jasonkuhrt jasonkuhrt changed the title feat: ignore operation name via a flag in RequestConfig feat: exclude operation name via a field in RequestConfig Feb 1, 2024
@jasonkuhrt jasonkuhrt merged commit 0f1b7b5 into graffle-js:main Feb 1, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants