Errors in componentWillUnmount should be caught by error boundary on …

…initial render.
reactjs-hispano · Feb 12, 2016 · dd390b3 · dd390b3
1 parent bd39799
commit dd390b3
Show file tree

Hide file tree

Showing 8 changed files with 74 additions and 21 deletions.
diff --git 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 <div>{this.props.children}</div>;
+        }
+        return <div>Error has been caught</div>;
+      }
+
+      unstable_handleError() {
+        this.setState({error: true});
+      }
+    }
+
+    class BrokenRender extends React.Component {
+      render() {
+        throw new Error('Always broken.');
+      }
+    }
+
+    class BrokenUnmount extends React.Component {
+      render() {
+        return <div />;
+      }
+      componentWillUnmount() {
+        throw new Error('Always broken.');
+      }
+    }
+
+    var container = document.createElement('div');
+    ReactDOM.render(
+      <ErrorBoundary>
+        <BrokenUnmount />
+        <BrokenRender />
+        <BrokenUnmount />
+      </ErrorBoundary>,
+      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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -448,7 +448,7 @@ ReactShallowRenderer.prototype.getRenderOutput = function() {
 
 ReactShallowRenderer.prototype.unmount = function() {
   if (this._instance) {
-    this._instance.unmountComponent();
+    this._instance.unmountComponent(false);
   }
 };