Discussion/feature request: custom error handling

[dallonf]

For my current project, we use apollo-react and Sentry (for error reporting and tracking).

Now, errors in render functions are historically something that React doesn't handle very elegantly, but usually they at least bubble up as an uncaught exception or unhandled promise rejection, and Sentry can detect this and report them.

However, when a render error happens in response to an update from Apollo, the error gets logged by console.error, here: https://github.com/apollostack/apollo-client/blob/v0.5.25/src/core/QueryManager.ts#L375

In order to report these errors to Sentry, I actually have to monkeypatch console.error, which I'm not really pleased with as a solution.

I don't know what a better solution would be - probably raising them as an uncaught exception. Or if this is happening in promise-land, I'd also be fine with a promise rejection, as we're using core-js promises and those do a pretty good job of reporting unhandled rejections, but I suspect that's not the case for everyone. Maybe the Apollo client should be configurable as to where these errors go?

[helfer]

@dallonf The reason we solved it the way we did for now is that we can't call the error callback because that's what threw the error in the first place, and we can't just throw the error, because if we did none of the other queries would get updated in queryListenerForObserver. I guess we could collect all the errors and throw one error for all once we're done?

@calebmer what are your thoughts here on how best to deal with those errors?

[calebmer]

It looks like the try/catch logic in that part of the code has had some real history 😊

I think the correct thing to do here is to make sure errors like these make their way out of Apollo as unhandled exceptions. By logging them to the console with console.error we are effectively re-inventing unhandled exceptions. An error in error handling code definitely sounds like something that should qualify as an unhandled exception to me 😊 (or any error in React render functions for that matter).

I’ve read up on some of the issues/PRs that ended up adding the try/catch and my intuition is that these problems are all happening because you are trying to deal with the errors synchronously. observer.next and observer.error synchronously call React render code, so when there are errors they propagate up to the subscription function. I think the right answer here is instead of using try/catch as was implemented in #980 would instead be to make observer.next and observer.error calls asynchronous. So:

setImmediate(() => observer.next(value))
// ...or for better runtime support
setTimeout(() => observer.next(value), 0)

The try/catch was added in response to apollographql/react-apollo#347 / #971 so that render errors would not become ApolloErrors and would instead log to the console. By making the call to observer.next/observer.error asynchronous then the errors thrown in render should become unhandled errors given there is no Apollo try/catch logic in the async context.

When looking at the issues/PRs that added the try/catch logic I didn’t see mention of “none of the other queries would get updated in queryListenerForObserver,” @helfer. Could you go into that more? Specifically the “other queries” bit. I may be missing something 😖

[dallonf]

That doesn't sound like a super hard PR for me. Let me know if that's the right approach and I can cook something up in a couple days!

[helfer]

@calebmer Great idea! I think setImmediate is exactly what we need. It makes a lot of sense to execute these callbacks asynchronously and make sure errors in those callbacks don't propagate back into Apollo Client. I think that's something we should do consistently for observable callbacks. 👍

[calebmer]

Sounds good @helfer. A PR would be welcome @dallonf 😊

[dallonf]

Well... I thought it would be pretty easy. But the unit tests for this are something else, lol. Could use some help on this.

[fischerman]

Any progress on this?

[calebmer]

@fischerman or @dallonf if you’d like to open a PR with this change we would be happy with helping you fix the unit tests 👍

[dallonf]

OK, created the PR!