Add support for componentDidCatch Component method #819

rpetrich · 2017-08-20T04:03:30Z

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 · 2017-08-21T17:37:42Z

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 · 2017-08-22T07:19:35Z

test/browser/lifecycle.js

+		it('should be called when child fails in constructor', () => {
+			class ErrorReceiverComponent extends Component {
+				componentDidCatch(error) {
+					this.setState({ "error": error });


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

@paranoidjk Thanks for the tip. Resolved.

coveralls · 2017-08-22T12:45:21Z

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

rpetrich · 2017-08-23T02:08:27Z

@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 · 2017-08-23T02:23:44Z

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 · 2017-08-26T18:57:55Z

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

rpetrich · 2017-08-26T18:59:46Z

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 · 2017-08-26T19:29:10Z

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

developit · 2017-08-28T18:00:31Z

@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 · 2017-09-12T04:52:22Z

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

rpetrich · 2017-09-12T04:58:28Z

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

developit · 2017-09-18T00:38:12Z

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

developit · 2017-09-18T00:55:23Z

src/vdom/component.js

-			base._component = componentRef;
-			base._componentConstructor = componentRef.constructor;
-		}
+		base._component = component;


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.

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

developit · 2017-09-18T00:57:46Z

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 · 2017-09-20T16:58:46Z

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

…defined; Add tests for _ancestorComponent/_parentComponent

coveralls · 2017-09-20T18:29:53Z

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

gaearon · 2017-09-27T11:52:06Z

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 · 2017-09-27T14:14:44Z

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

gaearon · 2017-09-27T14:23:25Z

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 · 2017-09-27T14:31:17Z

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

gaearon · 2017-09-28T11:31:35Z

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 · 2018-01-24T21:55:42Z

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

rpetrich · 2018-07-06T16:23:07Z

@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 · 2018-07-10T06:12:10Z

@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 :)

…-support

rpetrich · 2018-08-05T23:32:57Z

Latest Preact 8.3.0 release has been merged in.

studentIvan · 2018-08-14T22:07:02Z

@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 · 2018-08-15T01:25:54Z

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

studentIvan · 2018-08-15T13:13:41Z

@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 · 2018-08-15T13:17:57Z

@rpetrich studentIvan@d15a82b

rpetrich · 2018-08-16T14:38:55Z

@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 · 2018-09-17T16:15:31Z

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 · 2018-09-18T01:38:34Z

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 · 2018-10-10T16:33:05Z

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

johnhaitas · 2018-11-02T17:48:31Z

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

…cause `preact`'s support for `componentDidCatch()` does not pass this argument (preactjs/preact#819 (comment))

sampotts · 2019-02-12T03:34:25Z

Will this get merged in 2019 do we think?

marvinhagemeister · 2019-02-12T06:40:46Z

@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 · 2019-02-12T06:41:54Z

Awesome. Can't wait 👍

huhuanming · 2019-02-18T03:20:11Z

Awesome!

pierpo · 2019-03-01T15:49:17Z

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

marvinhagemeister · 2019-03-01T16:21:21Z

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

pierpo · 2019-03-01T16:45:18Z

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

marvinhagemeister · 2019-03-05T21:18:56Z

The alpha release is out 🎉

developit added the enhancement label Aug 21, 2017

paranoidjk suggested changes Aug 22, 2017

View reviewed changes

This was referenced Sep 18, 2017

wrap render in try when component defines componentDidCatch #880

Closed

Support for componentDidCatch #799

Closed

developit reviewed Sep 18, 2017

View reviewed changes

developit mentioned this pull request Sep 18, 2017

RenderQueue doesn't appear to be ordered by depth #874

Closed

adamsilverstein mentioned this pull request Sep 20, 2017

Add support for componentDidCatch Component method #886

Closed

Add support for Component.componentDidCatch variant with single argument

d54d337

rpetrich force-pushed the componentDidCatch-support branch from b616cc8 to d54d337 Compare September 20, 2017 16:53

Make root components have their _ancestorComponent property set to un…

6ff8cc6

…defined; Add tests for _ancestorComponent/_parentComponent

andrewiggins approved these changes Jul 10, 2018

View reviewed changes

reznord and others added 2 commits July 13, 2018 03:01

Merge branch 'master' into componentDidCatch-support

e7cfc20

Merge remote-tracking branch 'upstream/master' into componentDidCatch…

3ef85c4

…-support

johnhaitas mentioned this pull request Nov 2, 2018

Support componentDidCatch(error) preactjs/preact-render-to-string#64

Closed

This was referenced Nov 2, 2018

Adds support for componentDidCatch() preactjs/preact-render-to-string#66

Closed

Adds support for static getDerivedStateFromError(error) preactjs/preact-render-to-string#67

Closed

marvinhagemeister added the in X label Feb 12, 2019

marvinhagemeister closed this Mar 5, 2019

Add support for componentDidCatch Component method #819

Add support for componentDidCatch Component method #819

Conversation

rpetrich commented Aug 20, 2017

developit commented Aug 21, 2017

paranoidjk Aug 22, 2017

Choose a reason for hiding this comment

rpetrich Aug 23, 2017

Choose a reason for hiding this comment

coveralls commented Aug 22, 2017

rpetrich commented Aug 23, 2017

developit commented Aug 23, 2017

coveralls commented Aug 26, 2017

rpetrich commented Aug 26, 2017

coveralls commented Aug 26, 2017

developit commented Aug 28, 2017

coveralls commented Sep 12, 2017

rpetrich commented Sep 12, 2017

developit commented Sep 18, 2017

developit Sep 18, 2017

Choose a reason for hiding this comment

adamsilverstein Sep 20, 2017

Choose a reason for hiding this comment

developit commented Sep 18, 2017

coveralls commented Sep 20, 2017

coveralls commented Sep 20, 2017 • edited Loading

gaearon commented Sep 27, 2017

thysultan commented Sep 27, 2017

gaearon commented Sep 27, 2017

thysultan commented Sep 27, 2017

gaearon commented Sep 28, 2017 • edited Loading

hassanbazzi commented Jan 24, 2018

rpetrich commented Jul 6, 2018

andrewiggins left a comment

Choose a reason for hiding this comment

andrewiggins commented Jul 10, 2018 • edited Loading

rpetrich commented Aug 5, 2018

studentIvan commented Aug 14, 2018 • edited Loading

rpetrich commented Aug 15, 2018

studentIvan commented Aug 15, 2018

studentIvan commented Aug 15, 2018

rpetrich commented Aug 16, 2018

photz commented Sep 17, 2018

studentIvan commented Sep 18, 2018

roberttod commented Oct 10, 2018

johnhaitas commented Nov 2, 2018

sampotts commented Feb 12, 2019

marvinhagemeister commented Feb 12, 2019

sampotts commented Feb 12, 2019

huhuanming commented Feb 18, 2019

pierpo commented Mar 1, 2019

marvinhagemeister commented Mar 1, 2019

pierpo commented Mar 1, 2019

marvinhagemeister commented Mar 5, 2019

coveralls commented Sep 20, 2017 •

edited

Loading

gaearon commented Sep 28, 2017 •

edited

Loading

andrewiggins commented Jul 10, 2018 •

edited

Loading

studentIvan commented Aug 14, 2018 •

edited

Loading