From dd390b3e36e63f528173eb75cc2e8350d3209de9 Mon Sep 17 00:00:00 2001 From: jim Date: Mon, 8 Feb 2016 16:21:11 -0800 Subject: [PATCH] Errors in componentWillUnmount should be caught by error boundary on initial render. --- .../__tests__/ReactErrorBoundaries-test.js | 46 +++++++++++++++++++ src/renderers/dom/client/ReactMount.js | 7 +-- src/renderers/dom/shared/ReactDOMComponent.js | 4 +- .../shared/reconciler/ReactChildReconciler.js | 8 ++-- .../reconciler/ReactCompositeComponent.js | 16 +++++-- .../shared/reconciler/ReactMultiChild.js | 8 ++-- .../shared/reconciler/ReactReconciler.js | 4 +- src/test/ReactTestUtils.js | 2 +- 8 files changed, 74 insertions(+), 21 deletions(-) diff --git a/src/core/__tests__/ReactErrorBoundaries-test.js b/src/core/__tests__/ReactErrorBoundaries-test.js index b81d267f2a..0ba37323d5 100644 --- a/src/core/__tests__/ReactErrorBoundaries-test.js +++ b/src/core/__tests__/ReactErrorBoundaries-test.js @@ -55,6 +55,52 @@ describe('ReactErrorBoundaries', function() { expect(EventPluginHub.putListener).not.toBeCalled(); }); + it('will catch exceptions in componentWillUnmount', function() { + class ErrorBoundary extends React.Component { + constructor() { + super(); + this.state = {error: false}; + } + + render() { + if (!this.state.error) { + return
{this.props.children}
; + } + return
Error has been caught
; + } + + unstable_handleError() { + this.setState({error: true}); + } + } + + class BrokenRender extends React.Component { + render() { + throw new Error('Always broken.'); + } + } + + class BrokenUnmount extends React.Component { + render() { + return
; + } + componentWillUnmount() { + throw new Error('Always broken.'); + } + } + + var container = document.createElement('div'); + ReactDOM.render( + + + + + , + container + ); + ReactDOM.unmountComponentAtNode(container); + }); + it('expect uneventful render to succeed', function() { class Boundary extends React.Component { constructor(props) { diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index f4fc866119..ea62047b0a 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -168,8 +168,8 @@ function batchedMountComponentIntoNode( * @internal * @see {ReactMount.unmountComponentAtNode} */ -function unmountComponentFromNode(instance, container) { - ReactReconciler.unmountComponent(instance); +function unmountComponentFromNode(instance, container, safely) { + ReactReconciler.unmountComponent(instance, safely); if (container.nodeType === DOC_NODE_TYPE) { container = container.documentElement; @@ -567,7 +567,8 @@ var ReactMount = { ReactUpdates.batchedUpdates( unmountComponentFromNode, prevComponent, - container + container, + false ); return true; }, diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index 1377956636..6221f05c95 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -1008,7 +1008,7 @@ ReactDOMComponent.Mixin = { * * @internal */ - unmountComponent: function() { + unmountComponent: function(safely) { switch (this._tag) { case 'iframe': case 'img': @@ -1043,7 +1043,7 @@ ReactDOMComponent.Mixin = { break; } - this.unmountChildren(); + this.unmountChildren(safely); ReactDOMComponentTree.uncacheNode(this); EventPluginHub.deleteAllListeners(this); ReactComponentBrowserEnvironment.unmountIDFromEnvironment(this._rootNodeID); diff --git a/src/renderers/shared/reconciler/ReactChildReconciler.js b/src/renderers/shared/reconciler/ReactChildReconciler.js index 8d277f7c04..634b3444c7 100644 --- a/src/renderers/shared/reconciler/ReactChildReconciler.js +++ b/src/renderers/shared/reconciler/ReactChildReconciler.js @@ -100,7 +100,7 @@ var ReactChildReconciler = { } else { if (prevChild) { removedNodes[name] = ReactReconciler.getNativeNode(prevChild); - ReactReconciler.unmountComponent(prevChild); + ReactReconciler.unmountComponent(prevChild, false); } // The child must be instantiated before it's mounted. var nextChildInstance = instantiateReactComponent(nextElement); @@ -113,7 +113,7 @@ var ReactChildReconciler = { !(nextChildren && nextChildren.hasOwnProperty(name))) { prevChild = prevChildren[name]; removedNodes[name] = ReactReconciler.getNativeNode(prevChild); - ReactReconciler.unmountComponent(prevChild); + ReactReconciler.unmountComponent(prevChild, false); } } }, @@ -125,11 +125,11 @@ var ReactChildReconciler = { * @param {?object} renderedChildren Previously initialized set of children. * @internal */ - unmountChildren: function(renderedChildren) { + unmountChildren: function(renderedChildren, safely) { for (var name in renderedChildren) { if (renderedChildren.hasOwnProperty(name)) { var renderedChild = renderedChildren[name]; - ReactReconciler.unmountComponent(renderedChild); + ReactReconciler.unmountComponent(renderedChild, safely); } } }, diff --git a/src/renderers/shared/reconciler/ReactCompositeComponent.js b/src/renderers/shared/reconciler/ReactCompositeComponent.js index 8914fb6eaf..fb9c7c2397 100644 --- a/src/renderers/shared/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/reconciler/ReactCompositeComponent.js @@ -14,6 +14,7 @@ var ReactComponentEnvironment = require('ReactComponentEnvironment'); var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactElement = require('ReactElement'); +var ReactErrorUtils = require('ReactErrorUtils'); var ReactInstanceMap = require('ReactInstanceMap'); var ReactNodeTypes = require('ReactNodeTypes'); var ReactPerf = require('ReactPerf'); @@ -316,7 +317,7 @@ var ReactCompositeComponentMixin = { } checkpoint = transaction.checkpoint(); - this._renderedComponent.unmountComponent(); + this._renderedComponent.unmountComponent(true); transaction.rollback(checkpoint); // Try again - we've informed the component about the error, so they can render an error message this time. @@ -368,18 +369,23 @@ var ReactCompositeComponentMixin = { * @final * @internal */ - unmountComponent: function() { + unmountComponent: function(safely) { if (!this._renderedComponent) { return; } var inst = this._instance; if (inst.componentWillUnmount) { - inst.componentWillUnmount(); + if (safely) { + var name = this.getName() + '.componentWillUnmount()'; + ReactErrorUtils.invokeGuardedCallback(name, inst.componentWillUnmount.bind(inst)); + } else { + inst.componentWillUnmount(); + } } if (this._renderedComponent) { - ReactReconciler.unmountComponent(this._renderedComponent); + ReactReconciler.unmountComponent(this._renderedComponent, safely); this._renderedNodeType = null; this._renderedComponent = null; this._instance = null; @@ -805,7 +811,7 @@ var ReactCompositeComponentMixin = { ); } else { var oldNativeNode = ReactReconciler.getNativeNode(prevComponentInstance); - ReactReconciler.unmountComponent(prevComponentInstance); + ReactReconciler.unmountComponent(prevComponentInstance, false); this._renderedNodeType = ReactNodeTypes.getType(nextRenderedElement); this._renderedComponent = this._instantiateReactComponent( diff --git a/src/renderers/shared/reconciler/ReactMultiChild.js b/src/renderers/shared/reconciler/ReactMultiChild.js index 7a22c76cea..66f3861321 100644 --- a/src/renderers/shared/reconciler/ReactMultiChild.js +++ b/src/renderers/shared/reconciler/ReactMultiChild.js @@ -242,7 +242,7 @@ var ReactMultiChild = { updateTextContent: function(nextContent) { var prevChildren = this._renderedChildren; // Remove any rendered children. - ReactChildReconciler.unmountChildren(prevChildren); + ReactChildReconciler.unmountChildren(prevChildren, false); for (var name in prevChildren) { if (prevChildren.hasOwnProperty(name)) { invariant(false, 'updateTextContent called on non-empty component.'); @@ -262,7 +262,7 @@ var ReactMultiChild = { updateMarkup: function(nextMarkup) { var prevChildren = this._renderedChildren; // Remove any rendered children. - ReactChildReconciler.unmountChildren(prevChildren); + ReactChildReconciler.unmountChildren(prevChildren, false); for (var name in prevChildren) { if (prevChildren.hasOwnProperty(name)) { invariant(false, 'updateTextContent called on non-empty component.'); @@ -366,9 +366,9 @@ var ReactMultiChild = { * * @internal */ - unmountChildren: function() { + unmountChildren: function(safely) { var renderedChildren = this._renderedChildren; - ReactChildReconciler.unmountChildren(renderedChildren); + ReactChildReconciler.unmountChildren(renderedChildren, safely); this._renderedChildren = null; }, diff --git a/src/renderers/shared/reconciler/ReactReconciler.js b/src/renderers/shared/reconciler/ReactReconciler.js index 4e1a1d3934..507d351612 100644 --- a/src/renderers/shared/reconciler/ReactReconciler.js +++ b/src/renderers/shared/reconciler/ReactReconciler.js @@ -68,9 +68,9 @@ var ReactReconciler = { * @final * @internal */ - unmountComponent: function(internalInstance) { + unmountComponent: function(internalInstance, safely) { ReactRef.detachRefs(internalInstance, internalInstance._currentElement); - return internalInstance.unmountComponent(); + return internalInstance.unmountComponent(safely); }, /** diff --git a/src/test/ReactTestUtils.js b/src/test/ReactTestUtils.js index 0855f6da26..04ab62dbf7 100644 --- a/src/test/ReactTestUtils.js +++ b/src/test/ReactTestUtils.js @@ -448,7 +448,7 @@ ReactShallowRenderer.prototype.getRenderOutput = function() { ReactShallowRenderer.prototype.unmount = function() { if (this._instance) { - this._instance.unmountComponent(); + this._instance.unmountComponent(false); } };