Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds support for componentDidCatch() #66

Closed
wants to merge 8 commits into from

Conversation

johnhaitas
Copy link
Contributor

This does not include support for the info argument because preact's support for componentDidCatch() does not pass this argument (preactjs/preact#819 (comment))

Also includes 3 new tests.

…cause `preact`'s support for `componentDidCatch()` does not pass this argument (preactjs/preact#819 (comment))
@johnhaitas
Copy link
Contributor Author

This should resolve #64

@johnhaitas
Copy link
Contributor Author

This current implementation is incorrect per this documentation: https://reactjs.org/docs/react-component.html#error-boundaries

NOTE
Error boundaries only catch errors in the components below them in the tree. An error boundary can’t catch an error within itself.

Updating the code, starting with the tests.

@johnhaitas
Copy link
Contributor Author

I have updated the tests to verify that the error is in fact being passed to componentDidCatch with c7aba79.

@developit developit self-requested a review February 17, 2019 21:41
@@ -82,7 +83,17 @@ function renderToString(vnode, context, opts, inner, isSvgMode) {
}
}

return renderToString(rendered, context, opts, opts.shallowHighOrder!==false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: we can circumvent having to catch and re-throw the error when there's no componentDidCatch:

if (c && c.componentDidCatch) {
	try {
		return renderToString(rendered, context, opts, opts.shallowHighOrder!==false);
	}
	catch (error) {
		c.componentDidCatch(error);
		rendered = c.render(c.props, c.state, c.context);
		// now we fall through to the non-componentDidCatch render!
	}
}
return renderToString(rendered, context, opts, opts.shallowHighOrder!==false);

@developit
Copy link
Member

Just noticed the React docs seem to indicate they don't support componentDidCatch during SSR:
https://reactjs.org/docs/error-boundaries.html

Are we early on this and risking breaking compat?

@developit
Copy link
Member

/cc @andrewiggins

@developit
Copy link
Member

One thing I think we might want to consider is extending this to also allow catching enqueued state updates. Something like "render the component, then check if it errored or was dirtied via setState()/forceUpdate(). If so, render again".

@johnhaitas
Copy link
Contributor Author

per react's server support for error boundaries (#66) ... if react does not support it, then it probably is not right to support it

@marvinhagemeister
Copy link
Member

I concur, this would be an awesome feature to support imo 👍

@rschristian
Copy link
Member

Closing as implemented in #305, upon the work done here.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants