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

Support passthrough updates for error boundaries #7949

Merged
merged 23 commits into from
Oct 15, 2016
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7ebc488
Initial pass at the easy case of updates (updates that start at the r…
jimfb Feb 11, 2016
9017999
Don't expect an extra componentWillUnmount call
gaearon Oct 12, 2016
965d12f
Remove duplicate expectations from the test
gaearon Oct 12, 2016
8236279
Fix style issues
gaearon Oct 12, 2016
d09d33f
Make naming consistent throughout the tests
gaearon Oct 12, 2016
aedd80d
receiveComponent() does not accept safely argument
gaearon Oct 12, 2016
5f869d2
Assert that lifecycle and refs fire for error message
gaearon Oct 12, 2016
3890a3a
Add more tests for mounting
gaearon Oct 13, 2016
045dcb5
Do not call componentWillMount twice on error boundary
gaearon Oct 13, 2016
b69d428
Document more of existing behavior in tests
gaearon Oct 13, 2016
e374fc7
Do not call componentWillUnmount() when aborting mounting
gaearon Oct 13, 2016
1fa0b36
Consistently display error messages in tests
gaearon Oct 14, 2016
8066a50
Add more logging to tests and remove redundant one
gaearon Oct 14, 2016
b0e6a08
Refactor tests
gaearon Oct 14, 2016
842bd75
Split complicated tests into smaller ones
gaearon Oct 14, 2016
0783dee
Assert clean unmounting
gaearon Oct 14, 2016
b36526e
Add assertions about update hooks
gaearon Oct 14, 2016
db4c306
Add more tests to document existing behavior and remove irrelevant de…
gaearon Oct 14, 2016
99dad51
Verify we can recover from error state
gaearon Oct 14, 2016
de5bdf1
Fix lint
gaearon Oct 14, 2016
c64dbfd
Error in boundary’s componentWillMount should propagate up
gaearon Oct 14, 2016
7026adc
Move calling componentWillMount() into mountComponent()
gaearon Oct 14, 2016
9f66fa4
Remove extra whitespace
gaearon Oct 14, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/renderers/dom/client/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,15 @@ function batchedMountComponentIntoNode(
* @internal
* @see {ReactMount.unmountComponentAtNode}
*/
function unmountComponentFromNode(instance, container, safely) {
function unmountComponentFromNode(instance, container) {
if (__DEV__) {
ReactInstrumentation.debugTool.onBeginFlush();
}
ReactReconciler.unmountComponent(instance, safely);
ReactReconciler.unmountComponent(
instance,
false /* safely */,
false /* skipLifecycle */
);
if (__DEV__) {
ReactInstrumentation.debugTool.onEndFlush();
}
Expand Down Expand Up @@ -618,8 +622,7 @@ var ReactMount = {
ReactUpdates.batchedUpdates(
unmountComponentFromNode,
prevComponent,
container,
false
container
);
return true;
},
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/dom/shared/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -1155,7 +1155,7 @@ ReactDOMComponent.Mixin = {
*
* @internal
*/
unmountComponent: function(safely) {
unmountComponent: function(safely, skipLifecycle) {
switch (this._tag) {
case 'audio':
case 'form':
Expand Down Expand Up @@ -1197,7 +1197,7 @@ ReactDOMComponent.Mixin = {
break;
}

this.unmountChildren(safely);
this.unmountChildren(safely, skipLifecycle);
ReactDOMComponentTree.uncacheNode(this);
EventPluginHub.deleteAllListeners(this);
this._rootNodeID = 0;
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/native/ReactNativeBaseComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,10 @@ ReactNativeBaseComponent.Mixin = {
return this;
},

unmountComponent: function() {
unmountComponent: function(safely, skipLifecycle) {
ReactNativeComponentTree.uncacheNode(this);
deleteAllListeners(this);
this.unmountChildren();
this.unmountChildren(safely, skipLifecycle);
this._rootNodeID = 0;
},

Expand Down
16 changes: 12 additions & 4 deletions src/renderers/shared/stack/reconciler/ReactChildReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,11 @@ var ReactChildReconciler = {
} else {
if (prevChild) {
removedNodes[name] = ReactReconciler.getHostNode(prevChild);
ReactReconciler.unmountComponent(prevChild, false);
ReactReconciler.unmountComponent(
prevChild,
false, /* safely */
false /* skipLifecycle */
);
}
// The child must be instantiated before it's mounted.
var nextChildInstance = instantiateReactComponent(nextElement, true);
Expand All @@ -170,7 +174,11 @@ var ReactChildReconciler = {
!(nextChildren && nextChildren.hasOwnProperty(name))) {
prevChild = prevChildren[name];
removedNodes[name] = ReactReconciler.getHostNode(prevChild);
ReactReconciler.unmountComponent(prevChild, false);
ReactReconciler.unmountComponent(
prevChild,
false, /* safely */
false /* skipLifecycle */
);
}
}
},
Expand All @@ -182,11 +190,11 @@ var ReactChildReconciler = {
* @param {?object} renderedChildren Previously initialized set of children.
* @internal
*/
unmountChildren: function(renderedChildren, safely) {
unmountChildren: function(renderedChildren, safely, skipLifecycle) {
for (var name in renderedChildren) {
if (renderedChildren.hasOwnProperty(name)) {
var renderedChild = renderedChildren[name];
ReactReconciler.unmountComponent(renderedChild, safely);
ReactReconciler.unmountComponent(renderedChild, safely, skipLifecycle);
}
}
},
Expand Down
156 changes: 129 additions & 27 deletions src/renderers/shared/stack/reconciler/ReactCompositeComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,14 @@ var ReactCompositeComponent = {
context
);
} else {
markup = this.performInitialMount(renderedElement, hostParent, hostContainerInfo, transaction, context);
markup = this.performInitialMount(
renderedElement,
hostParent,
hostContainerInfo,
transaction,
context,
false /* skipLifecyle */
);
}

if (inst.componentDidMount) {
Expand Down Expand Up @@ -434,7 +441,14 @@ var ReactCompositeComponent = {
var markup;
var checkpoint = transaction.checkpoint();
try {
markup = this.performInitialMount(renderedElement, hostParent, hostContainerInfo, transaction, context);
markup = this.performInitialMount(
renderedElement,
hostParent,
hostContainerInfo,
transaction,
context,
false /* skipLifecyle */
);
} catch (e) {
// Roll back to checkpoint, handle error (which may add items to the transaction), and take a new checkpoint
transaction.rollback(checkpoint);
Expand All @@ -443,39 +457,59 @@ var ReactCompositeComponent = {
this._instance.state = this._processPendingState(this._instance.props, this._instance.context);
}
checkpoint = transaction.checkpoint();

this._renderedComponent.unmountComponent(true);
this._renderedComponent.unmountComponent(
true, /* safely */
// Don't call componentWillUnmount() because they never fully mounted:
true /* skipLifecyle */
);
transaction.rollback(checkpoint);

// Try again - we've informed the component about the error, so they can render an error message this time.
// If this throws again, the error will bubble up (and can be caught by a higher error boundary).
markup = this.performInitialMount(renderedElement, hostParent, hostContainerInfo, transaction, context);
markup = this.performInitialMount(
renderedElement,
hostParent,
hostContainerInfo,
transaction,
context,
// We have already called componentWillMount() before retrying:
true /* skipLifecyle */
);
}
return markup;
},

performInitialMount: function(renderedElement, hostParent, hostContainerInfo, transaction, context) {
performInitialMount: function(
renderedElement,
hostParent,
hostContainerInfo,
transaction,
context,
skipLifecyle
) {
var inst = this._instance;

var debugID = 0;
if (__DEV__) {
debugID = this._debugID;
}

if (inst.componentWillMount) {
if (__DEV__) {
measureLifeCyclePerf(
() => inst.componentWillMount(),
debugID,
'componentWillMount'
);
} else {
inst.componentWillMount();
}
// When mounting, calls to `setState` by `componentWillMount` will set
// `this._pendingStateQueue` without triggering a re-render.
if (this._pendingStateQueue) {
inst.state = this._processPendingState(inst.props, inst.context);
if (!skipLifecyle) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@sebmarkbage sebmarkbage Oct 14, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ooh good catch. (See what I did there?)

if (inst.componentWillMount) {
if (__DEV__) {
measureLifeCyclePerf(
() => inst.componentWillMount(),
debugID,
'componentWillMount'
);
} else {
inst.componentWillMount();
}
// When mounting, calls to `setState` by `componentWillMount` will set
// `this._pendingStateQueue` without triggering a re-render.
if (this._pendingStateQueue) {
inst.state = this._processPendingState(inst.props, inst.context);
}
}
}

Expand Down Expand Up @@ -521,7 +555,7 @@ var ReactCompositeComponent = {
* @final
* @internal
*/
unmountComponent: function(safely) {
unmountComponent: function(safely, skipLifecycle) {
if (!this._renderedComponent) {
return;
}
Expand All @@ -532,8 +566,10 @@ var ReactCompositeComponent = {
inst._calledComponentWillUnmount = true;

if (safely) {
var name = this.getName() + '.componentWillUnmount()';
ReactErrorUtils.invokeGuardedCallback(name, inst.componentWillUnmount.bind(inst));
if (!skipLifecycle) {
var name = this.getName() + '.componentWillUnmount()';
ReactErrorUtils.invokeGuardedCallback(name, inst.componentWillUnmount.bind(inst));
}
} else {
if (__DEV__) {
measureLifeCyclePerf(
Expand All @@ -548,7 +584,11 @@ var ReactCompositeComponent = {
}

if (this._renderedComponent) {
ReactReconciler.unmountComponent(this._renderedComponent, safely);
ReactReconciler.unmountComponent(
this._renderedComponent,
safely,
skipLifecycle
);
this._renderedNodeType = null;
this._renderedComponent = null;
this._instance = null;
Expand Down Expand Up @@ -941,7 +981,11 @@ var ReactCompositeComponent = {
inst.state = nextState;
inst.context = nextContext;

this._updateRenderedComponent(transaction, unmaskedContext);
if (inst.unstable_handleError) {
this._updateRenderedComponentWithErrorHandling(transaction, unmaskedContext);
} else {
this._updateRenderedComponent(transaction, unmaskedContext);
}

if (hasComponentDidUpdate) {
if (__DEV__) {
Expand All @@ -961,16 +1005,70 @@ var ReactCompositeComponent = {
}
},

/**
* Call the component's `render` method and update the DOM accordingly.
*
* @param {ReactReconcileTransaction} transaction
* @internal
*/
_updateRenderedComponentWithErrorHandling: function(transaction, context) {
var checkpoint = transaction.checkpoint();
try {
this._updateRenderedComponent(transaction, context);
} catch (e) {
// Roll back to checkpoint, handle error (which may add items to the transaction),
// and take a new checkpoint
transaction.rollback(checkpoint);
this._instance.unstable_handleError(e);
if (this._pendingStateQueue) {
this._instance.state = this._processPendingState(this._instance.props, this._instance.context);
}
checkpoint = transaction.checkpoint();

// Gracefully update to a clean state
this._updateRenderedComponentWithNextElement(
transaction,
context,
null,
true /* safely */
);

// Try again - we've informed the component about the error, so they can render an error message this time.
// If this throws again, the error will bubble up (and can be caught by a higher error boundary).
this._updateRenderedComponent(transaction, context);
}
},

/**
* Call the component's `render` method and update the DOM accordingly.
*
* @param {ReactReconcileTransaction} transaction
* @internal
*/
_updateRenderedComponent: function(transaction, context) {
var nextRenderedElement = this._renderValidatedComponent();
this._updateRenderedComponentWithNextElement(
transaction,
context,
nextRenderedElement,
false /* safely */
);
},

/**
* Call the component's `render` method and update the DOM accordingly.
*
* @param {ReactReconcileTransaction} transaction
* @internal
*/
_updateRenderedComponentWithNextElement: function(
transaction,
context,
nextRenderedElement,
safely
) {
var prevComponentInstance = this._renderedComponent;
var prevRenderedElement = prevComponentInstance._currentElement;
var nextRenderedElement = this._renderValidatedComponent();

var debugID = 0;
if (__DEV__) {
Expand All @@ -986,7 +1084,11 @@ var ReactCompositeComponent = {
);
} else {
var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance);
ReactReconciler.unmountComponent(prevComponentInstance, false);
ReactReconciler.unmountComponent(
prevComponentInstance,
safely,
false /* skipLifecycle */
);

var nodeType = ReactNodeTypes.getType(nextRenderedElement);
this._renderedNodeType = nodeType;
Expand Down
20 changes: 16 additions & 4 deletions src/renderers/shared/stack/reconciler/ReactMultiChild.js
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,11 @@ var ReactMultiChild = {
updateTextContent: function(nextContent) {
var prevChildren = this._renderedChildren;
// Remove any rendered children.
ReactChildReconciler.unmountChildren(prevChildren, false);
ReactChildReconciler.unmountChildren(
prevChildren,
false, /* safely */
false /* skipLifecycle */
);
for (var name in prevChildren) {
if (prevChildren.hasOwnProperty(name)) {
invariant(false, 'updateTextContent called on non-empty component.');
Expand All @@ -309,7 +313,11 @@ var ReactMultiChild = {
updateMarkup: function(nextMarkup) {
var prevChildren = this._renderedChildren;
// Remove any rendered children.
ReactChildReconciler.unmountChildren(prevChildren, false);
ReactChildReconciler.unmountChildren(
prevChildren,
false, /* safely */
false /* skipLifecycle */
);
for (var name in prevChildren) {
if (prevChildren.hasOwnProperty(name)) {
invariant(false, 'updateTextContent called on non-empty component.');
Expand Down Expand Up @@ -423,9 +431,13 @@ var ReactMultiChild = {
*
* @internal
*/
unmountChildren: function(safely) {
unmountChildren: function(safely, skipLifecycle) {
var renderedChildren = this._renderedChildren;
ReactChildReconciler.unmountChildren(renderedChildren, safely);
ReactChildReconciler.unmountChildren(
renderedChildren,
safely,
skipLifecycle
);
this._renderedChildren = null;
},

Expand Down
Loading