react
react copied to clipboard
facebook
The fake event trick for rethrowing errors in DEV fires unexpected global error handlers and makes testing harder
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.
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...
});
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.
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.
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).
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.
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.
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.
@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.
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?
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.
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.
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?
@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.
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.
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.
@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.
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.
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...
});
class Foo extends React.Component {
...
componentDidCatch(error) {
Server.logError(error);
this.setState({ didError: true });
markAsHandled(error);
}
...
}
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.
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. :/
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.
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?
@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
@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?).
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?
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
Ah that's the thing. We don't use real browsers for testing so we haven't bumped into this.
@gaearon Could this also apply to async operations that emit errors like setState function/callback pattern.
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.
What if jest had some sort of syntax like:
expect(() => {
TestUtils.renderIntoDocument(<DoSomething naughty/>)
})
.toThrow(/Bad developer!/)
.andCatch() // <---- prevents React 16 error boundary warning
❓🤷♂️
