-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Unify subscription error behavior and update emitted type #12476
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
Unify subscription error behavior and update emitted type #12476
Conversation
🦋 Changeset detectedLatest commit: b474106 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 |
|
4e25df0 to
c79b5bd
Compare
0ce183c to
f75f867
Compare
commit: |
c79b5bd to
7a5edf4
Compare
046e148 to
13a3fc6
Compare
7a5edf4 to
9e97db8
Compare
13a3fc6 to
f4076c8
Compare
9e97db8 to
6b1bfc8
Compare
f4076c8 to
cf30381
Compare
| if (onError) { | ||
| onError(error); | ||
| } else { | ||
| invariant.error("Unhandled GraphQL subscription error", error); |
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.
invariant.error will only display in dev and only log, not throw - do we want that behaviour to be dev-only?
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 think its ok to leave as dev-only. I'm guessing the original intent here was to let someone know that an error occurred that isn't handled since the error would otherwise get swallowed. I think I'm ok leaving this as a dev-only check since its more of a "warning" that you might want to do something to handle the error.
cf30381 to
b474106
Compare
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
Closes #12456
Closes #12422
Closes #12427
Changes the type emitted from subscriptions to a
SubscribeResult, which useserrorinstead oferrors. This change also unifies the error behavior between GraphQL errors and network errors so that network errors adhere to the configurederrorPolicy. This change means that results are no longer emitted when usingerrorPolicy: 'ignore'if there is no associateddataas well so that we avoid emitting an empty value.This change also no longer terminates the downstream connection when encountering an error. If the downstream connection closes as a result of the error, a
nextevent will be followed by acompleteevent instead. This change essentially means that using theerrorcallback is useless and no longer needed. This change allows for additional results to be emitted from the subscription if the subscription remains open after errors are emitted.