Add support for componentDidCatch Component method

[rpetrich]

Allow components to catch errors that occur during child components' constructors or lifecycle methods. For simplicity's sake does not pass a second "info" argument like React does. If no component catches and handles a thrown exception, DOM will be left in an inconsistent state as it is in react 8.2.1

[developit]

Hiya! Thanks for all the work here - just reading up on your solution and forming some thoughts.

I had a question - was the existing _parentComponent property not working for you? It's set here:
https://github.com/developit/preact/blob/master/src/vdom/component.js#L122

[paranoidjk]

eslint error cause ci failed https://travis-ci.org/developit/preact/builds/266889109?utm_source=github_status&utm_medium=notification

[rpetrich]

@paranoidjk Thanks for the tip. Resolved.

[coveralls]

Coverage remained the same at 100.0% when pulling 10bd841 on rpetrich:componentDidCatch-support into 0dea3b7 on developit:master.

[rpetrich]

@developit _parentComponent wasn't set in the path where components are created inside buildComponentFromVNode. Perhaps I've misinterpreted the meaning of _parentComponent and should actually introduce a new property?

[developit]

Not sure - _parentComponent only indicates a compositional parent (where a component has rendered the current component at its root), not grandparents (where there are DOM elements between the two).

[coveralls]

Coverage remained the same at 100.0% when pulling 38f943e on rpetrich:componentDidCatch-support into 0dea3b7 on developit:master.

[rpetrich]

Ah, that makes sense. Error handling should pass through non-component ancestors to be compatible with React.

Would you prefer this PR keep the adjusted behaviour of _parentComponent, or be amended to restore the previous behaviour of _parentComponent and implement a different approach of finding the deep ancestor components?

[coveralls]

Coverage remained the same at 100.0% when pulling 7f72326 on rpetrich:componentDidCatch-support into e29de9d on developit:master.

[developit]

@rpetrich definitely need to use a second property - using the current one will break higher order components (the existence of that property is used to determine if a component is a HoC).

[coveralls]

Coverage remained the same at 100.0% when pulling b616cc8 on rpetrich:componentDidCatch-support into db6ab26 on developit:master.

[rpetrich]

@developit This is ready for an updated review. I took the liberty of naming the new property _ancestorComponent (minifying to __a)

[developit]

@rpetrich perfect, that's exactly the name I would have chosen! I'll re-review this.

[developit]

Removing the conditional & crawl here seems like it would break component composition. When re-rendering a component that is a child of another component, it will update the _component property in the DOM do point to itself, which is then incorrect.

[adamsilverstein]

I removed these changes in #886 and verified the tests still pass. Wasn't sure how/if I could amend this PR directly.

[developit]

Would it be possible to split up some of the changes here? Right now this adds componentDidCatch(), but also changes some pretty fundamental things about the diff that seem like they would break real-world apps (not sure why the tests still pass, seems like we're either missing tests or I'm missing something haha). I'm super curious about those other changes and want to figure out if we can merge them too, but they would represent a major version bump and a lot of scrutiny, whereas the componentDidCatch stuff could be merged right away.

[coveralls]

Coverage remained the same at 100.0% when pulling d54d337 on rpetrich:componentDidCatch-support into f7834ec on developit:master.

[coveralls]

Coverage remained the same at 100.0% when pulling 3ef85c4 on rpetrich:componentDidCatch-support into 8dea9cc on developit:master.

[gaearon]

Here is the React error boundary test suite. It took us a long time to iron out the edge cases so it would be great to aim for at least partial parity.

https://github.com/facebook/react/blob/master/src/renderers/__tests__/ReactErrorBoundaries-test.js

[thysultan]

@gaearon Would a recursive React error boundary continue to bubble up the tree or bail?

[gaearon]

It stops at the closest boundary. If that boundary throws attempting to re-render, it bubbles up again, and the process repeats until we reach the root. If we have reached the root, and the error is still unhandled, we unmount the whole thing.

[thysultan]

What happens to errors thrown in componentWillUnmount , do they also try to bubble/recover or are they passive?

[gaearon]

Yes, errors in all lifecycle hooks are caught, including componentWillUnmount, and trigger the closest error boundary. However, there is some nuance there that is easy to miss.

Both componentDidMount and componentDidUpdate methods often set up subscriptions. Whenever we‘re unmounting due to an error, we need to at least try to execute componentWillUnmount for every component that has had (now or previously) its componentDidMount called. That is our best effort at preventing a memory leak (and dead subscriptions causing future errors).

However, since componentWillUnmount itself might throw, we need to isolate them from each other, and still execute them despite potential further errors.

Here is an example of the interplay between a broken componentDidMount in one component and a broken componentWillUnmount in another one. Hope this helps!

[hassanbazzi]

@gaearon Hey Dan! I'm attempting to get this going again. Is there an updated link to that test suite?

[rpetrich]

@sampotts Currently it's stable and usable by referencing the branch as an npm dependency, but the PR is somewhat stalled (which is understandable considering that it's a substantial adjustment, increases the API surface area, and has performance implications)

[andrewiggins]

LGTM!

[andrewiggins]

@lukeed Is there a way to unset your review to unblock this PR (since the "request changes" status was an accident)? Or if you have time, could you take another look and leave more comments or approve?

I done goofed! Can't review carefully quite yet, but at least I won't block this :)

[rpetrich]

Latest Preact 8.3.0 release has been merged in.

[studentIvan]

@rpetrich just tested your current branch code, seems it throws

preact.js?10a9:324 Uncaught (in promise) TypeError: Failed to execute 'replaceChild' on 'Node': parameter 1 is not of type 'Node'.
    at renderComponent (preact.js?10a9:324)
    at rerender (preact.js?10a9:37)

at this line baseParent.replaceChild(base, initialBase);

on error, but componentDidCatch works

p.s.: not sure what this code does, but seems
if (typeof base === 'undefined') { return; }
or if (void 0 === base) return;
before baseParent.replaceChild(base, initialBase);
saves the situation

[rpetrich]

@studentIvan Do you have a test case I could examine?

[studentIvan]

@rpetrich sorry it's on my project, so only if we can skype/etc and I will share my screen to you.
what I have done is simple added componentDidCatch method to root App component with latest preact code base and throw something while render.
By the way - if (void 0 === base) return; saves the situation, can you tell me what will broke with this code? If nothing can we just add this simple line into it?)

[studentIvan]

@rpetrich studentIvan@d15a82b

[rpetrich]

@studentIvan base should not be undefined at that point and I'm wary to add a check there if it's only hiding problems elsewhere. Is it possible to extract a smaller test case from your code? Alternatively, feel free to reach out to me on the Preact Slack team—I have the same handle.

[photz]

Until this is merged, is there some workaround? I need to be able to catch exceptions in my root component and respond to them.

[studentIvan]

Until this is merged, is there some workaround? I need to be able to catch exceptions in my root component and respond to them.

I use preact from studentIvan/preact#componentDidCatch-support before merge and base errors resolution

[roberttod]

I am also having to use studentIvan/preact#componentDidCatch-support. What needs to be done to merge this?

[johnhaitas]

What about support for static getDerivedStateFromError(error)? https://reactjs.org/docs/react-component.html#static-getderivedstatefromerror

[sampotts]

Will this get merged in 2019 do we think?

[marvinhagemeister]

@sampotts componentDidCatch will ship with Preact X. We need the PR a while back there. Our current these estimate is early March and we'll likely merge the 2 repos before that 🎉

[sampotts]

Awesome. Can't wait 👍

[huhuanming]

Awesome!

[pierpo]

Fantastic! 🎉
Any news about the "early March" estimate? 😊

[marvinhagemeister]

@pierpo An alpha release is scheduled for March 4th✌️ See: https://twitter.com/marvinhagemeist/status/1097973028426788864

[pierpo]

@marvinhagemeister Amazing! 😍
Thank you for the very quick answer!

[marvinhagemeister]

The alpha release is out 🎉