React: 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.

[brandonbloom]

@sebmarkbage In my system, there's a bunch of background processing and, if it fails, future event handlers will be all out of sync and the system will have cascading failures. I could wrap every callback for every subsystem in try/catch, but asynchrony makes that not practical, so I hook window.onerror. I want to log error, to the server, of course, but I also want to display some fatal error screen. In dev, that would show a trace, in prod, that would be akin to a :( browser tab.

[brandonbloom]

Yea, I meant the rethrow callsite is where the break point happens.

Ah, OK, understood. That is the only compelling argument I've heard thus far :)

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

My experience is that pausing on exceptions is not viable in any JavaScript scenario I've ever encountered, but again, I admit I am atypical here.

Probably unnecessary aside: JavaScript, and indeed most exception handling mechanisms, are semantically broken. Go, by contrast, runs defer/recover logic prior to unwinding the stack, so that the context at the point of the original throw would be available at the point of the last uncaught rethrow.

Short of fixing every major browser's debuggers, I don't have a better solution to propose. However, I will say that I still think the disease is worse than the cure. As long as you get a stack trace, you can add a conditional debugger statement at the site of the original throw.

[sebmarkbage]

A tangentially related problem is that there's a lot of responsibility put on users of try/catch, async, promises and error boundaries. To ensure that the error gets propagated to the centralized handler so that it can be properly logged to the server and a message displayed in DEV. People mess this up all the time. That's why we added a console.error even if an error is caught by an error boundary.

It seems to me that, this rationale extends to your use case. You'd ideally want to log and show the dialog even if the error is handled by a poorly constructed third party error boundary to render a fallback view. This would argue for us issuing a global onerror event in PROD.

On the other hand, you have your own local error boundaries that are designed to play nicely with your global error dialog/:( screen and logging. You want to explicitly state that you have properly handled this error and that you're not a poorly constructed third-party error boundary.

Maybe a solution could be that you have an API that tags errors as handled?

const handledErrors = new WeakSet();
function markAsHandled(error) {
  handledErrors.add(error);
}
window.addEventListener("error", function (event) {
  let {error} = event;
  if (handledErrors.has(error)) {
    return;
  }
  // Original event handler here...
});

```js
class Foo extends React.Component {
...
componentDidCatch(error) {
Server.logError(error);
this.setState({ didError: true });
markAsHandled(error);
}
...
}

[brandonbloom]

You'd ideally want to log and show the dialog even if the error is handled by a poorly constructed third party error boundary to render a fallback view.

I'd consider this a bug in the 3rd party library, just as I would if _.takeWhile(xs, pred) suppressed errors from calls to pred.

Maybe a solution could be that you have an API that tags errors as handled?

The code you posted depends on the subtle order of execution for the callbacks.

Have you tried it? I suspect it won't work because a handler installed at startup will run before the handler React installs and uninstalls during rendering and lifecycle method execution.

[sebmarkbage]

I'd consider this a bug in the 3rd party library

The issue with bugs like this (unhandled Promises being the most common), is how do you find them if they only happen to some users and they don't log to the server?

The code you posted depends on the subtle order of execution for the callbacks.

True. I was thinking of the PROD case where we'd use try/catch and dispatchEvent so we could control ordering. For the DEV mode case, like this issue is talking about, that wouldn't work. :/

[mweststrate]

Some way of marking exceptions as handled would be great. I have currently a hard time of testing exceptions thrown by components or error boundaries themselves. Although I can assert that the error boundaries work correctly; it seems impossible to prevent the exception from be thrown further. The test framework I am using (tape) has no way of preventing an uncaught exception to not fail the test.

I created a very minimal reproduction of this issue here: https://github.com/mweststrate/react-error-boundaries-issue

If there is something wrong with the setup I'll gladly hear! But if error boundaries are conceptually try / catches then it should be optional imho whether errors get rethrown or not.

[gaearon]

We use error boundaries to display a reasonable fallback to the application user, but still consider them errors in development or during testing. They’re not meant to be used for control flow. You don’t want somebody to introduce a bug to a leaf component and accidentally ship it because the tests swallowed the exception in a boundary. Does this make sense?

[thysultan]

@mweststrate Couldn't you handle this with t.throws(fn, expected, msg)?

Though from the repo it doesn't look like it would/should make it to the error boundary since it might be a top-level invariant error like Children.only.

Edit: Strange that it reaches the Oops error

[mweststrate]

@thysultan no, as fn won't throw the exception synchronously

They’re not meant to be used for control flow. You don’t want somebody to introduce a bug to a leaf component and accidentally ship it because the tests swallowed the exception in a boundary. Does this make sense?

@gaearon
Not really, as that would indicate that the component introducing the bug isn't tested itself? But regardless of that question, the problem is that as lib author I cannot create an environment anymore in which I can sandbox and test exceptions (using error boundaries or any other means).

As lib author I want to test failure scenarios as well. For example that my component throws when it is wrongly configured (in my specific case; context lacks some expected information). Previously (<16) it was no problem to test this behavior (Probably because the error was thrown sync so I could wrap a try / catch around ReactDOM.render). But now I cannot prevent exceptions from escaping anymore.

So I am fine with the new default, but I think an option to influence this behavior is missing (or any another strategy to test exception behavior?).

[gaearon]

Hmm I'm pretty sure wrapping with try/catch still works for us. The throwing is still synchronous. Maybe there's some difference in the testing environment?

[mweststrate]

See the test: https://github.com/mweststrate/react-error-boundaries-issue/blob/master/index.js#L23

Real browser/ DOM rendering / nested components / no test utils / no enzyme

[gaearon]

Ah that's the thing. We don't use real browsers for testing so we haven't bumped into this.

[thysultan]

@gaearon Could this also apply to async operations that emit errors like setState function/callback pattern.

[mweststrate]

Yeah these kind of subtle differences are exactly the reason why I prefer to have (part of) the test suite running in the DOM :). There are subtle differences in how batches etc work, and I wanted to make sure the IRL behavior is as expected in regards of the amount of renderings, their order etc. But it might be over-cautious.

[erikras]

What if jest had some sort of syntax like:

expect(() => {
  TestUtils.renderIntoDocument(<DoSomething naughty/>)
})
.toThrow(/Bad developer!/)
.andCatch() // <---- prevents React 16 error boundary warning

❓🤷‍♂️

[gaearon]

I agree this behavior doesn't make sense in Jest environment. I actually don't think it happens in our tests but maybe we're mocking something and not seeing it.

Can you please raise a separate issue about seeing it in Jest?

[kpdecker]

One more datapoint here: Our env utilizing Sentry loaded via CDN ended up masking all errors passed to error boundaries as cross origin errors due to the way that Sentry hooks into most async APIs. We don't normally run in our dev mode with global error handling connected for obvious reasons, but this led to a lot of confusion and concern while debugging and figuring out the scope of the problem.

For anyone searching: this was the react-crossorigin-error error.

[ConradIrwin]

We are also having a problem with invokeGuardedCallback.

It's necessary when dealing with browser focus to know where is the focus going to be at the end of this tick. You can assume in most cases that it'll be the same place it currently is (document.activeElement), but during a blur event it'll be on the relatedTarget, and during click events on an iframe, it will be the iframe even though the current activeElement is the body.

The easiest way to do this is to look at window.event in our getFocusedElement function, but window.event is overridden by invokeGuardedCallback in development, and there does not seem to be a mechanism to get the original value back.

In React 15 we could do this by hooking into ReactEventListeners, by requiring it directly, but that loophole has now been closed. Any other ideas?

[gaearon]

In React 15 we could do this by hooking into ReactEventListeners, by requiring it directly, but that loophole has now been closed. Any other ideas?

Can you open a separate issue with more details about this particular use case?

[Jessidhia]

No matter how React handles the error (printing to console, calling error boundary, re-throwing a top level exception, etc), the main DX causality I'm feeling with this system is that I can't have access to the stack and environment conditions that caused an exception to be originally thrown.

I'd really appreciate if my "pause on uncaught exception" debugger flag would pause exactly on the original throw, with the entire context available, but I understand it may be impossible to support this case while also supporting error boundaries, unless there is a flag you can turn on on a __DEV__ build of React to completely disable error boundaries.

[awreese]

Hmmm... I totally agree with @mweststrate. I wasted about half a day researching why, after adding error boundaries to our app and through live testing seeing the errors get caught/handled, the fallback element wasn't being rendered and instead the error propagated further and the app crashed. This isn't the expected behavior based on the error boundary description. Perhaps it should be mentioned in the docs that by design the fallback only gets rendered in production. I too wanted to test the fallback rendering should an invalid prop be passed to a child component or the child component falls over. We should be able to test error boundary branching and 'control flow', since that's what it's actually doing, from within the dev environment.

[edit 1/11/18]
I must revise my complaint. The app wasn't crashing and the fallback element _was_ getting rendered, it's just that a full screen error pops up, covering my app, where it wasn't immediately or overtly clear it could be dismissed. Now knowing this is the intended behavior I don't have an issue. Docs could be a little more informational regarding this.

[davidjb]

I'm writing a set of components and part of this are common ErrorBoundary objects that I'd like to render to visually demonstrate what happens how an error is handled. At present, I see the same behaviour as @awreese in that demo exceptions are caught, the fallback is getting rendered but the re-thrown exception covers up the app.

This lead to my initial impression that I _wasn't_ catching the exceptions because the console states uncaught exception: [object]. So after hours of hunting & second guessing, I finally ended up here after noting the componentDidCatch functions _are_ being called and this problem doesn't affect non-dev environments (eg test, prod etc).

In my case, the exceptions are meaningless and they are being caught; seeing a full-screen overlay just leads to confusion. Any manner of avoiding this would be helpful -- a way of marking exceptions as handled; an option for not re-throwing exceptions already handled at a boundary; or some way of turning off the traceback overlay etc.

Quick-and-dirty workaround in public/index.html to just hide the overlay:

    <style>
      body > iframe { display: none; }
    </style>

[hoschi]

I also had the problem that I first thought I did something wrong as I always see the last resort error view, which is triggered by a global "onerror" handler. At very least, the docs should mention this.

Another thing is that I cannot really believe global error handles are that rare, but lacking of real data as well. As React "only" catches the errors in renders, a global error handler needs to be used to catch all errors in a last resort manner. I personally did this already before, to catch expected errors in e.g. network errors in sagas.

[Alphy11]

It seems really unintuitive that React calls it componentDidCatch, when in reality it is not "catching" it is more of componentOnError.

As some context on the scenario this is causing issues for us on. Now that we've added intelligent error boundaries to our app, we want to start using that same boundary to handle an API error. So we are throwing when we hit an API error, and then it bubbles up to the nearest error boundary, and is handled just like any other error. Unfortunately, in dev, this causes redbox to appear, and since our dev environment is complex, a couple api calls pretty regularly fail. So this redbox appearing a couple seconds into every page load is extremely frustrating.

[awreese]

@Alphy11 componentDidCatch is actually a rather intuitive name as it did catch an error produced by one of its descendants. Is your API actually producing errors, or have you defined your application to be in some error state given certain API call results? If the error boundary 'red box' is happening that much in dev, a workaround could be to set a dev env var and basically suspend the error throwing on specific api calls that routinely falsely fail in dev.

[monfera]

@brandonbloom :

This is misguided because it's not about pausing on exceptions, it's about "pause on uncaught exceptions."

I keep running into the same thing. I'd like to have Dev Tools stop right when an exception happens in my code. But it'll still fly over that because I don't enable Pause on caught expressions. Because, as you say,

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

Yes, for example Babel routinely throws and catches exceptions. Exceptions are no longer exceptions, they're the norm. This feels wrong, but I'm unfamiliar with the constraints Babel development faced. It's one of the reasons why I avoid Babel on new projects and prefer Bublé or just no transpilation during development (ES2015 is rather well covered by browsers of 2018). In general I'd prefer if transpilation were removed from development time (browser reload latency, sourcemap needs, exception pollution etc.), relegated to become part of a release build and automated tests; not sure why we as a community cling to transpiling large codebases on every single line change. But it's how it's done in most shops.

In short, i'm trying to use a facility available for decades: be in the stack where my exception happens, without other libraries constraining this freedom.

@sebmarkbage :

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".

_Side question: why are you adding "by default"? Is there a flag that makes it happen?_

Maybe browser vendors could help restore sanity when it comes to uncaught exception? Browser blackboxing could be generalized; this, or something analogous to this could cover the suppression of uncaught exceptions (yes, sounds weird). I think that 'uncaught-blackboxing' a few libraries such as jQuery and Babel would help restore this otherwise useful function. Sure it's a hassle but on any specific project, there may not be more than a handful of offending libraries, and it's a one-time blackboxing step. Or maybe all could be blackboxed and one's own app code whitelisted. Btw. i'd like to have blackboxing for performance measurement too 😄

@Kovensky : completely agreeing with your comment and think such a flag would be feasible

[lrowe]

@sophiebits wrote:

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.

Does suspense change your perspective here? I'm wondering whether error boundaries should be the appropriate mechanism with which to handle data fetch failures, simply mapping as promise rejection, or whether to wrap as Promise<Result<T>> to handle separately to programmer errors.

[guillaume86]

I'd just like to point out that that behavior should be clearly explained in the error boudaries / componentDidCatch documentation (it is not at the moment).

I had missing error logs in production builds because I was using the window error event to capture errors globally. It was working in dev, but not in production with error boundaries (which seems like the right thing to do, but confusing when the different behavior is not documented).

[noscripter]

I think the core point of catch rethrowing errors in tests or with componentDidCatch is to limit or reduce the noises introduced by known error situations, which should be handled instead of interrupting the whole javascript thread.

[donaldpipowitch]

Sadly this doesn't play nice with other tools like Cypress which fully stops on uncaught errors which confused us a lot initially when tests run fine in production, but not locally.