ErrorBoundaries: Initial pass at the easy case of updates#6020
ErrorBoundaries: Initial pass at the easy case of updates#6020jimfb wants to merge 1 commit intofacebook:masterfrom
Conversation
8d41345 to
b7c534b
Compare
b7c534b to
3745894
Compare
|
@jimfb updated the pull request. |
3745894 to
60e23bf
Compare
|
@jimfb updated the pull request. |
60e23bf to
ef84061
Compare
|
@jimfb updated the pull request. |
|
cc @spicyj @sebmarkbage I think this one is now ready for review. |
| } | ||
| } | ||
|
|
||
| class InnocentParnet extends React.Component { |
743f081 to
27b0f58
Compare
27b0f58 to
d959599
Compare
|
Ping @spicyj @sebmarkbage |
|
Do we need to make a branch cut before we land?
|
|
That's probably a question for @zpao, I don't care either way (before/after branch cut). I just want to make forward progress on this diff, so we should do a CR either way. |
|
There are mutations that happen during this phase. https://github.com/facebook/react/blob/master/src/renderers/shared/reconciler/ReactMultiChild.js#L325 These are not applied to the underlying nodes until later: https://github.com/facebook/react/blob/master/src/renderers/shared/reconciler/ReactMultiChild.js#L357 This will definitely leave the system in an inconsistent state. I guess the strategy here is that we'll not touch the DOM nodes during the safe unmount and therefore it doesn't matter. Can you add some tests that ensures that child removals, additions and reordering works when the error is thrown in the middle of all that? |
| // and take a new checkpoint | ||
| transaction.rollback(checkpoint); | ||
| this._instance.unstable_handleError(e); | ||
| if (this._pendingStateQueue) { |
There was a problem hiding this comment.
Doesn't this mean that we always render null? Regardless if the component updated state.
Can you add something to your tests that checks that the error message is actually shown in the DOM, not just executed?
There was a problem hiding this comment.
Rendering null means something. It means inserting a placeholder.
Ideally this would be unmounting safely first. Then try to mount the new child as defined by the state change and if that fails, bubble up the error one more level.
There was a problem hiding this comment.
Inserting a placeholder is an implementation detail, not something guaranteed by React. I think my initial approach was to unmount safely first, but the issue is that you need to unmount the children but not the parent, in which case the parent's state is wrong, and the whole thing just starts introducing complicated branching logic and introducing a lot of surface area for bugs. Rendering to null was much cleaner.
|
Tests and the null thing. |
d959599 to
a81ff82
Compare
|
Added some tests - let me know if they don't test what you wanted. Also responded to the null thing. |
|
Works great! |
|
Just verified that this works great as illustrated by @gaearon above. I see no problems with the implementation, but I'm by no means a react internals expert. Nonetheless, can we please get this to land soon? |
|
Closing this as there hasn't been any updates in 3 months. Feel free to comment on this one or recreate a pull request if this is something you'd like to continue. |
|
We want to get this in probably. I'm planning to take a look at it this week. |
|
I plan to change it to not depend on it. This doesn't preserve state deeply anyway. I think I'll build error handling into react-proxy instead. |
|
@gaearon ok thx |
|
Any update on this? Our internal FB teams will benefit hugely when it's working |
|
Sorry, been busy with the ES6 class conversion. Will try to look tomorrow. |
| 'error handled', | ||
| 'BrokenUnmount is attempting to unmount', | ||
| 'BrokenUnmount is attempting to unmount', | ||
| 'BrokenUnmount is attempting to unmount', |
There was a problem hiding this comment.
A little unfortunate that we re-call componentWillUnmount on the component that already failed to unmount.
|
I’m rebasing and addressing comments on this in #7949. |
This is a very initial pass at the easy case of updates (updates that pass through the error boundary by themselves). There are likely bugs and more testing is needed before this can be merged. Don't feel obligated to code review yet, posting mostly for visibility.