Support passthrough updates for error boundaries #7949
Conversation
|
FWIW I didn't get fully through reviewing #6020, only through where my last comment was so please make sure you take a thorough look. |
|
Got it. I'll add full logging to all tests and add more tests to understand how this works. |
Previously, we would call componentWillUnmount() safely on the tree whenever we abort mounting it. However this is likely risky because the tree was never mounted in the first place. People shouldn't hold resources in componentWillMount() so it's safe to say that we can skip componentWillUnmount() if componentDidMount() was never called. Here, we introduce a new flag. If we abort during mounting, we will not call componentWillUnmount(). However if we abort during an update, it is safe to call componentWillUnmount() because the previous tree has been mounted by now.
gaearon
changed the title
| // `this._pendingStateQueue` without triggering a re-render. | ||
| if (this._pendingStateQueue) { | ||
| inst.state = this._processPendingState(inst.props, inst.context); | ||
| if (!skipLifecyle) { |
There was a problem hiding this comment.
You should just move this out of the try/catch so you can avoid the argument bloat and runtime check. It would help with my understanding as well because this one is not recursive where as the componentWillUnmount equivalent flag is recursive.
There was a problem hiding this comment.
Move what out? Yeah I'd like to make this less ambiguous but I don't see what you propose yet.
I want to "retry mounting but without calling the hook". Do you propose to extract everything except calling the hook in another method, and then calling that method specifically from the "retry" path?
There was a problem hiding this comment.
I'm suggesting moving the content of this block (calling the life-cycle) to mountComponent (line 335).
This would also move it out of the try/catch in performInitialMountWithErrorHandling but I believe that is currently a bug. If an error happens in this life-cycle, then we should propagate the error up to the outer error boundary. Not handle it in this failed one.
There was a problem hiding this comment.
Ooh good catch. (See what I did there?)
This test is currently failing.
This removes the unnecessary non-recursive skipLifecycle check. It fixes the previously failing test that verifies that if a boundary throws in its own componentWillMount(), the error will propagate.
|
That should fix it. |
|
lgtm |
|
Hopefully Travis will work by Monday. 😄 |
* Initial pass at the easy case of updates (updates that start at the root). * Don't expect an extra componentWillUnmount call It was fixed in facebook#6613. * Remove duplicate expectations from the test * Fix style issues * Make naming consistent throughout the tests * receiveComponent() does not accept safely argument * Assert that lifecycle and refs fire for error message * Add more tests for mounting * Do not call componentWillMount twice on error boundary * Document more of existing behavior in tests * Do not call componentWillUnmount() when aborting mounting Previously, we would call componentWillUnmount() safely on the tree whenever we abort mounting it. However this is likely risky because the tree was never mounted in the first place. People shouldn't hold resources in componentWillMount() so it's safe to say that we can skip componentWillUnmount() if componentDidMount() was never called. Here, we introduce a new flag. If we abort during mounting, we will not call componentWillUnmount(). However if we abort during an update, it is safe to call componentWillUnmount() because the previous tree has been mounted by now. * Consistently display error messages in tests * Add more logging to tests and remove redundant one * Refactor tests * Split complicated tests into smaller ones * Assert clean unmounting * Add assertions about update hooks * Add more tests to document existing behavior and remove irrelevant details * Verify we can recover from error state * Fix lint * Error in boundary’s componentWillMount should propagate up This test is currently failing. * Move calling componentWillMount() into mountComponent() This removes the unnecessary non-recursive skipLifecycle check. It fixes the previously failing test that verifies that if a boundary throws in its own componentWillMount(), the error will propagate. * Remove extra whitespace
This is a work in progress.
Right now this is just #6020 rebased to merge cleanly, but I'll try to address comments from its review as well later.