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

AbortController support #4250

Merged
merged 1 commit into from
Oct 23, 2024
Merged

AbortController support #4250

merged 1 commit into from
Oct 23, 2024

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Oct 20, 2024

This adds support for aborting execution from the outside or resolvers, this adds a few tests and tries to make the support as easy as possible.

Do we want to support having abort support on subscriptions, I guess it makes sense for server-sent events.

I've chosen 2 places to place these interrupts

  • executeFieldsSerially - every time we start a new mutation we check whether the runtime has interrupted
  • executeFields - every time we start executing a new field we check whether the runtime has interrupted
    • inside of the catch block as well so we return a singular error, all though this doesn't really matter as the consumer would not receive anything
    • this here should also take care of deferred fields

When comparing this to graphql-tools/execute I am not sure whether we want to match this behavior, this throws a DomException which would be a whole new exception that gets thrown while normally during execution we wrap everything with GraphQLErrors.

Supersedes #3791
Resolves #3764

Copy link

netlify bot commented Oct 20, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 370d0f1
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6718c1e354978300082df3c0
😎 Deploy Preview https://deploy-preview-4250--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

Hi @JoviDeCroock, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@JoviDeCroock JoviDeCroock force-pushed the abort-signal branch 3 times, most recently from 3a61463 to f49401d Compare October 20, 2024 09:24
@JoviDeCroock JoviDeCroock marked this pull request as ready for review October 20, 2024 09:25
@JoviDeCroock JoviDeCroock requested a review from a team as a code owner October 20, 2024 09:25
@JoviDeCroock JoviDeCroock force-pushed the abort-signal branch 4 times, most recently from d51d39f to ae108b2 Compare October 22, 2024 06:41
@JoviDeCroock JoviDeCroock added the PR: feature 🚀 requires increase of "minor" version number label Oct 22, 2024
@ardatan
Copy link
Member

ardatan commented Oct 22, 2024

When comparing this to graphql-tools/execute I am not sure whether we want to match this behavior, this throws a DomException which would be a whole new exception that gets thrown while normally during execution we wrap everything with GraphQLErrors.

In fetch, when the request is aborted, it throws AbortError as DOMException.
I think it uses abortSignal.throwIfAborted under the hood instead of creating a new Error and throw it as you did here, so the AbortSignal decides on what to throw.

@JoviDeCroock JoviDeCroock force-pushed the abort-signal branch 2 times, most recently from 3841aad to 370d0f1 Compare October 23, 2024 09:29
Co-Authored-By: yaacovCR <yaacovCR@gmail.com>
@JoviDeCroock JoviDeCroock merged commit 84b122c into main Oct 23, 2024
35 checks passed
@yaacovCR yaacovCR deleted the abort-signal branch October 23, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants