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

Throwing an error in a next() callback leads to an ApolloError #971

Closed
tmeasday opened this issue Nov 30, 2016 · 4 comments
Closed

Throwing an error in a next() callback leads to an ApolloError #971

tmeasday opened this issue Nov 30, 2016 · 4 comments
Assignees

Comments

@tmeasday
Copy link
Contributor

tmeasday commented Nov 30, 2016

I think there are a variety of issues around about this, the clearest being apollographql/react-apollo#347 (also apollographql/react-apollo#303, #884, and more)

If you throw an exception in a next() function in a query subscriber, it throws all the way to here (logging on the way). Note that an bug in a React render function will throw such an error.

This leads to an ApolloError being dispatched.

If the same error path runs in that error handling (which is very possible with RA), you get an infinite loop (see #884).

I think we should separate the error handling for GraphQL problems from dispatch problems, via maybe adding an extra promise or an explicit try catch around the dispatch.

@helfer let me know if putting a repro together would help here.

@helfer
Copy link
Contributor

helfer commented Nov 30, 2016

Oh, interesting. Thanks for the writeup @tmeasday! I thought we fixed this in #910. Looks like there was another try-catch block higher up that we missed. 😬 This is a really annoying bug, so let's make sure we fix it for good this time.

@helfer
Copy link
Contributor

helfer commented Dec 1, 2016

After talking to @tmeasday and taking another look, here's what I think we should do:

Step 1: Intercept errors thrown from observer.next and observer.error and print them to the console.
Step 2: Introduce separate actions for network errors and store errors (right now everything is called a network error)

@stubailo
Copy link
Contributor

stubailo commented Dec 1, 2016

@helfer I don't think you covered step 2 in the PR?

@helfer
Copy link
Contributor

helfer commented Dec 1, 2016

@stubailo Yes that's right, I didn't cover step 2, because we'll need a major version bump to make that change. I think the PR took care of the issue here, and step 2 should be taken care of for 0.6 or possibly even a later version.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants