The fake event trick for rethrowing errors in DEV fires unexpected global error handlers and makes testing harder

[brandonbloom]

I'm trying to make use of componentDidCatch in the React 16 beta. I already had a global window error handler which was working fine, but it unexpectedly catches errors that I would expect componentDidCatch to have handled. That is, component-local errors are being treated as window-global errors in dev builds.

The problem seems to stem from invokeGuardedCallbackDev in ReactErrorUtils.js. I think that this entire __DEV__ block of code is problematic. The stated rational is:

  // In DEV mode, we swap out invokeGuardedCallback for a special version
  // that plays more nicely with the browser's DevTools. The idea is to preserve
  // "Pause on exceptions" behavior. Because React wraps all user-provided
  // functions in invokeGuardedCallback, and the production version of
  // invokeGuardedCallback uses a try-catch, all user exceptions are treated
  // like caught exceptions, and the DevTools won't pause unless the developer
  // takes the extra step of enabling pause on caught exceptions. This is
  // untintuitive, though, because even though React has caught the error, from
  // the developer's perspective, the error is uncaught.

This is misguided because it's not about pausing on exceptions, it's about "pause on uncaught exceptions." However, componentDidCatch makes exceptions caught!

Rather than switching on prod vs dev and using try/catch in prod and window's error handler in dev, React should always use try/catch, but rethrow if you reach the root without hitting a componentDidCatch handler. This would preserve the correct "pause on uncaught exceptions" behavior without messing with global error handlers.

[brandonbloom]

Workaround:

window.addEventListener("error", function (event) {
  let {error} = event;
  // XXX Ignore errors that will be processed by componentDidCatch.
  // SEE: https://github.com/facebook/react/issues/10474
  if (error.stack && error.stack.indexOf('invokeGuardedCallbackDev') >= 0) {
    return true;
  }
  // Original event handler here...
});

[sophiebits]

The way I use that button is:

• Pause on exception: I try to use this whenever possible whenever I'm trying to debug an unexpected exception in my code.
• Pause on caught exception: I use this when I think the exception might get caught by a try/catch, but this also tends to include lots of feature tests which I don't care about.

In React we don't recommend you use exceptions except for programmer errors, so I prefer (as a React user) that they always show up in "pause on exception" despite there being an error boundary. Error boundaries are a failsafe check that you don't expect to hit in normal operation.

Not sure if other folks have a different perspective.

[sophiebits]

Honestly what I would love to do is annotate individual try/catch blocks in the source as "this is just a feature test" vs "this is error handling for real errors" and have the latter always stop but the former stop only in a special mode.

[brandonbloom]

Whatever your preference, there are plenty of libraries out there that internally use exceptions. I've never found "pause on caught exception" to ever be useful, since it produces an incredible number of false positives. Still, what React is doing is against the grain of the browser's provided exception debugging tools (which I agree are inadequate).

[acdlite]

One reason we chose to do it this way is that we expect the best practice for React apps will be to always have a top-level error boundary — if you don't, then the whole app will unmount whenever there's an uncaught error. So if we disable this feature inside error boundaries, we will effectively disable it for all errors that originate inside a React lifecycle or render method.

[acdlite]

It would be nice to get some actual data on how often people rely on this feature. Personally, the number of times I've used it is small, but it's not zero.

[brandonbloom]

A top-level component error boundary is fine for errors that occur during rendering or lifecycle callbacks, but does not address code that responds to network or other top-level events. For that, you need to hook window.onerror, which is precisely how I ran in to this problem.

[acdlite]

@brandonbloom Right, I just mean to say that if we decide to always use try-catch inside error boundaries, then we're effectively deciding to use try-catch everywhere. In which case we should remove invokeGuardedCallback entirely and just use try-catch directly. That might be the right thing to do, but it's a trade-off either way.

[brandonbloom]

I think it's the right thing to do, so let's discuss the tradeoff.

On one hand, by removing this logic and replacing it with standard try/catch behavior, you have 1) less code and 2) better match with the standard "pause on caught exception" behavior. And 3) There is no wonkiness necessary for dev vs prod with respect to window.onerror. The workaround for (3) is quite kludgey, even if you provided some way to better distinguish these sorts of error events.

On the other hand, debugging caught exceptions is slightly more annoying (although no more annoying than it is in general, outside of React). The workaround for this annoyance is to simply disable error boundary components, either with a configuration flag, or by commenting out componentDidCatch, just like commenting out a catch block. Moreover, if a stack-trace rendering boundary component gets popular, this workaround is unlikely to be necessary. It's only if you want to actively debug/inspect the error object. In that case, adding a debugger; statement does the trick once you get the stack trace from either the browser's console or the boundary component's rendering.

Am I overlooking other things in favor of keeping this logic?

[acdlite]

I think that's about right. Another benefit is it removes the problem of cross-origin errors being obscured on the error event in DEV.

@sebmarkbage had the strongest feelings about preserving this feature, if I recall correctly. There could be additional factors that I've forgotten.

[brandonbloom]

I noticed some voodoo about that, but wasn't immediately affected by it, so didn't want to comment. In general, this feels like one of those situations where the complications are sufficient to justify doing less in the interest of avoiding compounding complexity.

[sebmarkbage]

To be clear: This doesn't just affect error boundaries. It'll have to be all errors that are "caught". Because, to preserve consistency, we effectively have a built-in boundary at the root. Even if we rethrow, that is still considered "caught". This is not common practice outside React so it would be significant downside.

The ecosystem is full of things that cause the noise. E.g. just open http://jsbin.com/ with "Pause On Caught Exception". You could possibly find a fix to all of those callsites. But it only takes a few popular libraries to not agree, to change the momentum for everyone. The game theory here plays out such that pausing only on uncaught exceptions is the winning strategy.

Therefore, I think that this proposal effectively becomes "pausing on exception is not viable by default in React apps".

The flag to turn on and off would technically work, but 1) it's not very discoverable 2) it's not very scalable for every lib to do this 3) we know from previous versions that people get very confused by subsequent errors that happens due to inconsistencies when we can't properly clean up. This would make the debugging experience worse.

These are things that every developer is affected by.

Meanwhile, top level onerror handling is relatively rare to deal with explicitly. If you deal with it at all on your site, it is usually just one person that deals with its implementation details.

For error boundaries, we typically log the errors anyway in DEV mode just in case someone forgets to in their handling. It turns out that it is way too easy to silence errors and make debugging silent ones a pain. So this part happens anyway. It similar to unhandled Promises.

Maybe we should always raise top level onerror events even in PROD?

[sebmarkbage]

@brandonbloom Can you elaborate more about your set up and why you wouldn't want to handle any error, including in boundaries, in onerror? E.g. for logging and such I'd expect anything treated as an error to be logged somewhere.

[brandonbloom]

Even if we rethrow, that is still considered "caught".

Huh?

I just tried these individually with the various options in Firefox and Chrome:

try { throw new Error() } catch(e) { }
try { throw new Error() } catch(e) { throw e; }

The behavior matches my expectations: Rethrowing is considered uncaught.

Meanwhile, top level onerror handling is relatively rare to deal with explicitly. If you deal with it at all on your site, it is usually just one person that deals with its implementation details.

I'll freely admit: I'm that person on pretty much every project I ever work on.

[sebmarkbage]

Rethrowing is considered uncaught.

Yea, I meant the rethrow callsite is where the break point happens. That would be somewhere in the React internals way later after the source of the error happened. In that case it doesn't provide any more value than a log.