facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.59k stars 46.79k forks source link

How to prevent ReactDOM.render errors from bubbling when otherwise explicitly handled #13681

Open jneander opened 6 years ago

jneander commented 6 years ago

Do you want to request a feature or report a bug?

This is a bug. Ordinarily, this would probably be considered a feature request. However, the stated purpose of the feature referenced below is being violated in certain environments.

What is the current behavior?

React 16+ surfaces an uncaught error during render, even when using componentDidCatch as designed or using try/catch around the render. As described in the comment above the related code, this is a convenience provided for developers using DevTools for debugging purposes. However, the convenience provided for development debugging is changing behavior in specs, causing failures for otherwise protected code paths, which goes against this statement from the comment description for the code:

But because the error happens in a different event loop context, it does not interrupt the normal program flow.

When the error occurs, a spec runner such as Mocha will fail the test with the uncaught error, then continue with the next test. After advancing, the second render of the component will complete and call the ReactDOM.render callback, which continues code from the already-failed test while a subsequent test is in progress. This pollutes the spec suite and leads to other issues that are not produced when using the Production version of React.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

All relevant code and content has been included in this CodeSandbox. Due to the use of karma/mocha, tests must be run locally. Inline comments add detail to behavior and expectations.

To see the tests pass, switch "test" to "production" in the karma.js file.

What is the expected behavior?

Typically, DevTools are used in a different context from running specs—automation vs investigation, for lack of more precise terms. It should be an option rather than the default when using React in a non-production environment. At least in an environment of test, where spec runners are conditionally sensitive to global errors, developers must have the option to disable or disallow this behavior as it is implemented at this time.

For a second, perhaps more intuitive option, refer to this portion of the mentioned comment, talking about "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.

When an exception during render is captured using componentDidCatch or try/catch as mentioned above, the exception should be considered "caught," as the developer has explicitly created an error boundary around this render. In this case, expected behavior would be for the error to not be surfaced globally and for the developer to debug any exceptions within the error boundary they defined.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

This is present only in the non-production version of React 16+. The development or test environments of React 16+ feature this behavior. React 15.* and below do not have this issue. Prior to React 16, explicit try/catch handlers were solely responsible for being an error boundary during render.

gaearon commented 6 years ago

We've observed the opposite: in large products people have error boundaries all over the place, accidentally catching and failing to surface legitimate bugs in tests.

jneander commented 6 years ago

It is certainly a valuable tool and should not be removed outright. But by retaining this behavior as a permanent path in specs, it eliminates the option to gracefully detect and assert around render failures in specs. This is especially important in non-sandboxed spec frameworks, where failures in the global scope are both disruptive to the flow of specs and often tricky to debug. Jest and JSDom eliminate this form of pollution. But the choice of those two tools is not available for every project.

Allowing this behavior as an option would ensure that the intended convenience remains for those who can benefit from it and that projects depending on non-sandboxed spec frameworks are not hindered by the conflicts this behavior can introduce into a spec suite.

gaearon commented 6 years ago

Can you add an error handler, like

window.addEventListener('error', e => {
  e.preventDefault();
});

?

jneander commented 6 years ago

Probably. But that seems … hacky. It would also smother any non- render-related errors that could be relevant during specs. Having more deliberate error handling could be a valuable addition to the ReactDOM.render api, where its use determines whether render errors are surfaced globally or through a designated handler.

bisubus commented 6 years ago

@gaearon error handler can interfere with error handling in other places. As the issue states, the problem can occur in tests. I would expect that test framework may use error handler for its own needs and start to malfunction if we mess with it.

jacobrask commented 6 years ago

I want to be able to throw things like network or authentication errors anywhere in my tree and catch them at a boundary to show an error message, instead of handling these errors at every place they can occur in my application. Like I would use try/catch.

If a network error occurs in a regular image somewhere I can just show alternative content or nothing, if it happens in a gallery it is a fatal error that I want to handle differently, but my image component can throw the same error. Error boundaries are great for this use case.

The console and Sentry will be spammed with errors because of this though, and adding a global error handler will silence legitimate errors that I have not caught with either try/catch or componentDidCatch.

It was surprising and unintuitive to me that componentDidCatch behaved differently than catch.

andycarrell commented 5 years ago

I believe I've observed something similar - I could never work out why subsequent tests would fail, but subsequent test suites would pass. Given that separate components are rendered between tests would explain that.

When the error occurs, a spec runner such as Mocha will fail the test with the uncaught error, then continue with the next test. After advancing, the second render of the component will complete and call the ReactDOM.render callback, which continues code from the already-failed test while a subsequent test is in progress. This pollutes the spec suite and leads to other issues that are not produced when using the Production version of React.

Is this an issue with error handling specifically, or maybe that React isn't properly being "cleaned up" between tests? I would expect to be able to run tests in isolation and use abandon the paused (on caught exceptions) render and start again.

I appreciate there are cases for maintaining the same instance of React across tests, it would be good to have the choice 👐