-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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
Error boundaries #5602
Error boundaries #5602
Conversation
2f486f1
to
d08db51
Compare
); | ||
} | ||
handleError(e) { | ||
this._errorMessage = e.message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does setState work here? If not, can we make it work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setState
does not work there. Among other things, we rollback the transaction after calling handleError. I'll try to figure out a way to make it work before merging.
d08db51
to
47b0a55
Compare
@jimfb updated the pull request. |
Now with setState support :). @sebmarkbage @spicyj |
if (inst.handleError) { | ||
var checkpoint = transaction.checkpoint(); | ||
try { | ||
markup = this.performInitialMount(renderedElement, nativeParent, nativeContainerInfo, transaction, context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly out of curiosity.
according to: https://github.com/petkaantonov/bluebird/wiki/Optimization-killers#2-unsupported-syntax try
/catch
/finally
make their containing function unoptimizable (in V8 at least).
Is there a reason you don't use the suggested workaround of isolating the try
/catch
/finally
statements into minimal functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we should probably do that – though I don't like making the stack deeper unnecessarily because it hampers debugging, and performance in the case that an exception is thrown is probably not a huge concern. Would be nice if the common path was broken up intelligently though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only adds a single stack frame to the boundary, not every component so I think it is probably fine. We should do it.
47b0a55
to
b6cfade
Compare
@jimfb updated the pull request. |
b6cfade
to
1a1727d
Compare
@jimfb updated the pull request. |
1a1727d
to
b54e010
Compare
@sebmarkbage Now with a unit test to assert the event handlers (eg. |
b54e010
to
3afced6
Compare
checkpoint = transaction.checkpoint(); | ||
|
||
this._renderedComponent.unmountComponent(); | ||
transaction.rollback(checkpoint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is interesting. Can this actually happen? Doesn't matter if we do it as a precaution since we're already in a slow path. Just curious.
shipit |
YAYYYYY!!! |
can't wait for the 0.15 release hahaha |
Awesome; great work! Really looking forward to trying this out! |
Awesome thanks! |
This is fantastic! :) |
Much of a performance hit with this? |
@mhart Should be virtually none. |
@jimfb – cool, good to know! Thanks. Now that I look at it in more detail it looks like the try/catch is happening at the mount stage, and not for each child render (is that correct?) – which certainly allays my initial fears. |
Is there an intention to also add error boundary checks for component updates? I really want to kill react-transform-catch-errors but unfortunately the current error boundary implementation doesn’t satisfy my use case. I am interested in catching errors occurring when a person hot reloads It would look like this: Currently, error boundaries only handle “broken, later fixed” part of the equation, but not the “OK, but later broke”. From the technical point of view, it is equivalent to let calls = 0;
// ...
render() {
calls++;
if (calls === 1) {
throw new Error('Yo');
}
return <NormalStuff />
} If there is a desire to support this I can look into implementing this. |
@gaearon's use-case is my own as well. With component playgrounds, it's easy for fixtures to be passed into a |
+1 i use hot reload and not yet "react-transform-catch-errors" and it's a pain because on any single render error due to bad transient code it requires an erroir even if I fix the code and hot-reload it |
It feels like this is also the solution for implementing 404 and 401 views when using React Router. ie if a child route throws a special class of exception (eg NotFoundError or UnauthorizedError), any parent route can catch it and render the appropriate component. Assuming I'm right, when this feature is fully implementing, the biggest issue with using React Router will be solved. |
@AndrewIngram you'd still need to detect the error state to set the proper response status when rendering on the server, though. |
Is there any intent to remove the |
It's not fully working yet: crashes in updates don't stop at the boundary. I intend to look into corresponding PR soon but API is likely to stay unstable_ until we use it for a while ourselves. |
Implements error boundaries for initial render, which allow React components to become isolation boundaries. If a child of an error bounadry crashes, the error boundary has the ability to handle the error and render something different (like an error message or a frown face). This allows application authors to partition their app into regions, and a crash in a single region won't take down the entire page/application.
Fixes #2461