React: App unmounts on caught error

[jshado1]

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

What is the current behavior?
Basically 3 levels of components

    Shell
        Header
        view // "children" via react-router
        Footer

Shell.tsx (catches)

class Shell extends React.Component {
    state: ShellState = {
        alerts: [],
    }

    componentDidCatch(err, info) {
        const alert = new Alert(err);
        this.setState({ alerts: _.concat(alerts, alert) });
        debugger; // all good in the neighbourhood
    }

    render() {
        return (
            <React.Fragment>
                <Header alerts={this.state.alerts} />
                {this.props.children}
                <Footer />
            </React.Fragment>
        );
    }
}

SomeView.tsx (always throws in render)

class SomeView extends React.Component {
    // …

    render() {
        throw new Error('test');
    }
}

---

Shell.componentDidCatch() fires and does its thing (no new errors thrown), but the app still unmounts anyway.

I understand why this is happening: SomeView's render always throws an error, so after the componentDidCatch handles the error and Shell re-renders, SomeView throws again, and apparently React shuts down (probably as an easy way to avoid an endless loop).

What is the expected behavior?

App does not unmount; abort/blacklist/ignore the miscreant component (which could be N-levels deep in the app).

I expect it to work like the codepen example.

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

No, before v16 React did not explode (to use less colourful language than this deserves).

react 16.3.1

---

This componentDidCatch is both neat and horrendous:

• Neat in that there is a (mostly) standard way to catch and handle errors.
• Horrendous in that it unmounts the entire app, creating an unrecoverable state (have you seen Spotify lately?). If anything, the unmounting should be a development-only bug-pretending-to-be-a-feature that can be disabled (because this is hella annoying and any cursory google search on it clearly shows countless hours wasted).

[gaearon]

I think the description of how we recommend to use error boundaries here is fairly explicit:

https://reactjs.org/docs/error-boundaries.html#where-to-place-error-boundaries
https://reactjs.org/docs/error-boundaries.html#new-behavior-for-uncaught-errors

Yes, this is intentional behavior. No, we don't recommend to just re-render the same component because indeed, it will likely fail again. Instead, we recommend rendering an unobtrusive placeholder that notifies the user that something went wrong. So in your case, instead of just displaying the alert, the solution would be to wrap individual views themselves in boundaries (the level of granularity is up to you).

Even if that placeholder is somehow buggy (we suggest writing tests for those to ensure the error fallbacks are solid), you can still have several error boundaries on the way up to the app. Yes, if all of them have bugs, your app will unmount, but that sounds like a big problem if the code is so buggy that even error recovery causes errors on every step. It points to bigger problems in your app.

Thinking that just keeping DOM in place is a graceful recovery is empty comfort. In practice the app does stay broken (because the assumptions held by the code are no longer true), just in more subtle ways. Like sending a message to the wrong person, displaying the wrong product price, etc. If you're losing user data input because of unmounting, consider what happens if the user accidentally closes the tab or their battery dies. It's indicative of a bigger problem you need to fix anyway.

Error boundaries are a low-level mechanism so you can definitely guard every little thing in them if you want, just like you can wrap every line of code in a try / catch. However, this doesn't necessarily result in better experience. Instead it might be worth intentionally designing failure states for different parts of your app, investing into unit tests for error boundary components to make sure they don't fail, and having a good error reporting system so you actually know where the bugs happen (and can fix them).

[jshado1]

@gaearon I'm sure Spotify are thrilled that their app is unmounting because an icon can't be found. Thank god for that, because their app would be completely unusable without that icon.

You say _the level of granularity is up to you_, but the bomb you've implanted into every app disagrees: In order to know where a problem has triggered this sentinel zealot, it essentially requires wrapping every component that does anything (especially since components can't handle this on their own).

The presumption you impose on everyone is that anything that goes wrong is a disaster, when in fact, most of the time, it's trivial and easily recoverable. I can decide when it's a disaster.

Thinking that just keeping DOM in place is a graceful recovery is empty comfort.

That is a fallacy: ViewA throws, ViewsB...Z are fine, and the nav is unaffected. When React doesn't explode, a user can navigate to any of the other 25 views that are fine.

Perhaps think of it this way: When Chrome freezes, do you expect your OS to explode? Of course not. And when it does, that's most definitely a bug (or a design flaw, if you prefer).

I understand unit tests, etc., and I understand that the problem _should_ be fixed anyway. However, an entire team, department, etc. doesn't need to be blocked with a bricked app whilst someone tries to find a missing icon (I don't work at Spotify and this is just a hypothetical that I could easily see happening).

[gaearon]

I'm sure Spotify are thrilled that their app is unmounting because an icon can't be found. Thank god for that, because their app would be completely unusable without that icon.

There is no need for sarcasm, we can have a technical discussion without making strawman arguments. Not having a single error boundary in the app is a bad practice, and what you describe would only happen if you didn’t have any.

it essentially requires wrapping every component that does anything (especially since components can't handle this on their own).

I’m confused by this statement. The error propagates up the tree. You don’t need to wrap every individual component. But yes, it’s a good idea to wrap big “parts” of your app (navigation bar, sidebar, content area) and maybe a few more granular ones (e.g. message input).

The presumption you impose on everyone is that anything that goes wrong is a disaster, when in fact, most of the time, it's trivial and easily recoverable. I can decide when it's a disaster.

I don’t see where you’re reading this. It is exactly my point that it is easily recoverable by wrapping a part of UI in a boundary. Just like try/catch.

Is it unclear that errors propagate up the tree from the documentation? It works just like try/catch.

[jshado1]

Sorry, perhaps the sarcasm came across too strong. But I do think the core of it was appropriate: That a trivial issue now brings down an entire site.

I’m confused by this statement. The error propagates up the tree. You don’t need to wrap every individual component.

You do if you don't want a missing Icon to explode the whole "big part".

---

The asynchronous convo is a bit too long-form. I'll post a summary here after we meet up this week / next week.