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!: add signal support #303

Merged
merged 12 commits into from
Jan 8, 2022

Conversation

arnaudbzn
Copy link
Contributor

@arnaudbzn arnaudbzn commented Nov 15, 2021

Add a new signal request option to request, rawRequest and batchRequests GraphQL Client methods.

New GraphQL Client request, rawRequest and batchRequests method overloads with a single options object argument.

The signal defined as a function argument overrides the signal defined in the GraphQLClient constructor.

This feature will add more flexibility and it will allow GraphQL Code Gen React Query plugin to support signal and request cancellation with graphql-request.

Related issue: #182

Also included in this PR: yarn test:coverage.

@alejo4373
Copy link

I was just looking for a way to cancel requests with this lib and the changes in this PR will enable me to do just that. Thanks, @arnaudbzn!

README.md Outdated Show resolved Hide resolved
tests/signal.test.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@arnaudbzn
Copy link
Contributor Author

@jasonkuhrt Node.js supports AbortController since v14.x.
To support previous Node.js versions (v12 in the current checks) we have to use a polyfill like node-abort-controller.
Or we can just remove Node.js v12 check here:

      matrix:
        node: [12, 14, 16]

According to the README, we should not have to fully support v12.

v12 is not officially supported so the impact should be minimal.
@arnaudbzn
Copy link
Contributor Author

@jasonkuhrt Node.js supports AbortController since v14.x. To support previous Node.js versions (v12 in the current checks) we have to use a polyfill like node-abort-controller. Or we can just remove Node.js v12 check here:

      matrix:
        node: [12, 14, 16]

According to the README, we should not have to fully support v12.

@jasonkuhrt , I've removed the Node v12 from the test matrix, everything should be green 🚦

@jasonkuhrt
Copy link
Member

@arnaudbzn Are there simple instructions for how Node 12 users can polyfill themselves? If so let's add that to our REDAME?

@arnaudbzn
Copy link
Contributor Author

@arnaudbzn Are there simple instructions for how Node 12 users can polyfill themselves? If so let's add that to our REDAME?

Actually AbortController has been added in Node.js v14.17.0
So I'll have to use an AbortController polyfill to support both v14 and v12.

@arnaudbzn
Copy link
Contributor Author

arnaudbzn commented Jan 5, 2022

@jasonkuhrt An AbortController polyfill is now used in the new signal tests to support both Node.js v12 and v14.
All checks ✅

tests/signal.test.ts Show resolved Hide resolved
@jasonkuhrt jasonkuhrt changed the title feat: add signal support by request feat!: add signal support by request Jan 8, 2022
@jasonkuhrt jasonkuhrt changed the title feat!: add signal support by request feat!: add signal support Jan 8, 2022
@jasonkuhrt jasonkuhrt merged commit 12c2e8d into graffle-js:master Jan 8, 2022
@bviebahn
Copy link

Looking forward to using this. Can this be released?

@arnaudbzn
Copy link
Contributor Author

Looking forward to using this. Can this be released?

@jasonkuhrt Thank you for the merge 🙂
When do you plan the next release? 🚀

@jasonkuhrt
Copy link
Member

I'll cut a preview release today. Unfortunately the automation for that is still failing.

@arnaudbzn
Copy link
Contributor Author

I'll cut a preview release today. Unfortunately the automation for that is still failing.

@jasonkuhrt Thank you Jason, any update about the release?

@jasonkuhrt
Copy link
Member

Preview went out 10 days ago, I can cut a stable today I guess.

@arnaudbzn
Copy link
Contributor Author

arnaudbzn commented Feb 4, 2022

Preview went out 10 days ago, I can cut a stable today I guess.

@jasonkuhrt Have you planned a release this week? Thks.

@jasonkuhrt
Copy link
Member

@arnaudbzn we cut one last week, there's a problem with the release tool so no activity on the git repo about that. But it is on npm now.

@arnaudbzn
Copy link
Contributor Author

@arnaudbzn we cut one last week, there's a problem with the release tool so no activity on the git repo about that. But it is on npm now.

That's great! thank you Jason.

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.

4 participants