From 7ebc488e6f44bc4056350e436e2d5a839125d6cd Mon Sep 17 00:00:00 2001 From: Jim Date: Thu, 11 Feb 2016 09:58:19 -0800 Subject: [PATCH 01/23] Initial pass at the easy case of updates (updates that start at the root). --- .../reconciler/ReactCompositeComponent.js | 53 ++- .../__tests__/ReactErrorBoundaries-test.js | 351 +++++++++++++++++- 2 files changed, 398 insertions(+), 6 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 56869d0ae698..56fb6c4fc80d 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -443,7 +443,6 @@ var ReactCompositeComponent = { this._instance.state = this._processPendingState(this._instance.props, this._instance.context); } checkpoint = transaction.checkpoint(); - this._renderedComponent.unmountComponent(true); transaction.rollback(checkpoint); @@ -941,7 +940,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__) { @@ -961,6 +964,35 @@ 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); + + // 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. * @@ -968,9 +1000,19 @@ var ReactCompositeComponent = { * @internal */ _updateRenderedComponent: function(transaction, context) { + var nextRenderedElement = this._renderValidatedComponent(); + this._updateRenderedComponentWithNextElement(transaction, context, nextRenderedElement, false); + }, + + /** + * 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__) { @@ -982,11 +1024,12 @@ var ReactCompositeComponent = { prevComponentInstance, nextRenderedElement, transaction, - this._processChildContext(context) + this._processChildContext(context), + safely ); } else { var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance); - ReactReconciler.unmountComponent(prevComponentInstance, false); + ReactReconciler.unmountComponent(prevComponentInstance, safely); var nodeType = ReactNodeTypes.getType(nextRenderedElement); this._renderedNodeType = nodeType; diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 75c8654e9b5e..51dafafbf14c 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -112,7 +112,7 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('will catch exceptions in componentWillUnmount', () => { + it('will catch exceptions in componentWillUnmount initial render', () => { class ErrorBoundary extends React.Component { constructor() { super(); @@ -302,4 +302,353 @@ describe('ReactErrorBoundaries', () => { 'Box componentWillUnmount', ]); }); + + it('catches errors on update', function() { + var log = []; + + class Box extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + log.push('Box renderError'); + return
Error: {this.state.errorMessage}
; + } + log.push('Box render'); + var ref = function(x) { + log.push('Inquisitive ref ' + x); + }; + return ( +
+ + {this.props.angry ? :
} +
+ ); + } + unstable_handleError(e) { + log.push('error handled'); + this.setState({errorMessage: e.message}); + } + componentDidMount() { + log.push('Box componentDidMount'); + } + componentWillUnmount() { + log.push('Box componentWillUnmount'); + } + } + + class Inquisitive extends React.Component { + render() { + log.push('Inquisitive render'); + return
What is love?
; + } + componentDidMount() { + log.push('Inquisitive componentDidMount'); + } + componentWillUnmount() { + log.push('Inquisitive componentWillUnmount'); + } + } + + class Angry extends React.Component { + render() { + log.push('Angry render'); + throw new Error('Please, do not render me.'); + } + componentDidMount() { + log.push('Angry componentDidMount'); + } + componentWillUnmount() { + log.push('Angry componentWillUnmount'); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(container.textContent).toBe('Error: Please, do not render me.'); + expect(log).toEqual([ + 'Box render', + 'Inquisitive render', + 'Inquisitive componentDidMount', + 'Inquisitive ref [object Object]', + 'Box componentDidMount', + 'Box render', + 'Inquisitive ref null', + 'Inquisitive render', + 'Angry render', + 'error handled', + 'Inquisitive ref null', + 'Inquisitive componentWillUnmount', + 'Box renderError', + ]); + }); + + it('catches componentWillUnmount errors on update', function() { + var log = []; + + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + log.push('Box renderError'); + return
Error: I am now a sad component :(
; + } + log.push('Box render'); + + return ( +
+ + + {this.props.angry ? null : } +
+ ); + } + unstable_handleError(e) { + log.push('error handled'); + this.setState({errorMessage: e.message}); + } + componentDidMount() { + log.push('Box componentDidMount'); + } + componentWillUnmount() { + log.push('Box componentWillUnmount'); + } + } + + class BrokenUnmount extends React.Component { + render() { + return
text
; + } + componentWillUnmount() { + log.push('BrokenUnmount is attempting to unmount'); + throw new Error('Always broken.'); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(container.textContent).toBe('Error: I am now a sad component :('); + expect(log).toEqual([ + 'Box render', + 'Box componentDidMount', + 'Box render', + 'BrokenUnmount is attempting to unmount', + 'error handled', + 'BrokenUnmount is attempting to unmount', + 'BrokenUnmount is attempting to unmount', + 'BrokenUnmount is attempting to unmount', + 'Box renderError', + ]); + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'Box render', + 'Box componentDidMount', + 'Box render', + 'BrokenUnmount is attempting to unmount', + 'error handled', + 'BrokenUnmount is attempting to unmount', + 'BrokenUnmount is attempting to unmount', + 'BrokenUnmount is attempting to unmount', + 'Box renderError', + 'Box componentWillUnmount', + ]); + }); + + it('catches componentWillUnmount errors nested children', function() { + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + return
Error: I am now a sad component :(
; + } + + return ( +
+ + {this.props.angry ? null : } +
+ ); + } + unstable_handleError(e) { + this.setState({errorMessage: e.message}); + } + } + + class InnocentParent extends React.Component { + render() { + return ; + } + } + + class BrokenUnmount extends React.Component { + render() { + return
text
; + } + componentWillUnmount() { + throw new Error('Always broken.'); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(container.textContent).toBe('Error: I am now a sad component :('); + ReactDOM.unmountComponentAtNode(container); + }); + + it('doesn\'t get into inconsistent state during removals', function() { + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + return
Error: I am now a sad component :(
; + } + + return ( +
{this.props.children}
+ ); + } + unstable_handleError(e) { + this.setState({errorMessage: e.message}); + } + } + + class InnocentComponent extends React.Component { + render() { + return
text
; + } + } + + class GuiltyComponent extends React.Component { + render() { + return
text
; + } + componentWillUnmount() { + throw new Error('Always broken.'); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(container.textContent).toBe('Error: I am now a sad component :('); + ReactDOM.unmountComponentAtNode(container); + }); + + it('doesn\'t get into inconsistent state during additions', function() { + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + return
Error: I am now a sad component :(
; + } + + return ( +
{this.props.children}
+ ); + } + unstable_handleError(e) { + this.setState({errorMessage: e.message}); + } + } + + class InnocentComponent extends React.Component { + render() { + return
text
; + } + } + + class GuiltyComponent extends React.Component { + render() { + throw new Error('Always broken.'); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + ReactDOM.render(, container); + expect(container.textContent).toBe('Error: I am now a sad component :('); + ReactDOM.unmountComponentAtNode(container); + }); + + it('doesn\'t get into inconsistent state during reorders', function() { + + function generateElements() { + var elements = []; + for (var i = 0; i < 100; i++) { + elements.push(); + } + elements.push(); + + var currentIndex = elements.length; + while (0 !== currentIndex) { + var randomIndex = Math.floor(Math.random() * currentIndex); + currentIndex -= 1; + var temporaryValue = elements[currentIndex]; + elements[currentIndex] = elements[randomIndex]; + elements[randomIndex] = temporaryValue; + } + return elements; + } + + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + return
Error: I am now a sad component :(
; + } + + return ( +
{this.props.children}
+ ); + } + unstable_handleError(e) { + this.setState({errorMessage: e.message}); + } + } + + class InnocentComponent extends React.Component { + render() { + return
text
; + } + } + + class GuiltyComponent extends React.Component { + render() { + if (fail) { + throw new Error('Always broken.'); + } + return
text
; + } + } + + var fail = false; + + var container = document.createElement('div'); + ReactDOM.render({generateElements()}, container); + fail = true; + ReactDOM.render({generateElements()}, container); + + expect(container.textContent).toBe('Error: I am now a sad component :('); + ReactDOM.unmountComponentAtNode(container); + }); }); From 901799989985714ab27f6ac9366284253e0e1215 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 12 Oct 2016 15:24:04 +0100 Subject: [PATCH 02/23] Don't expect an extra componentWillUnmount call It was fixed in #6613. --- .../stack/reconciler/__tests__/ReactErrorBoundaries-test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 51dafafbf14c..b00b7a5f2f4a 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -443,7 +443,6 @@ describe('ReactErrorBoundaries', () => { 'error handled', 'BrokenUnmount is attempting to unmount', 'BrokenUnmount is attempting to unmount', - 'BrokenUnmount is attempting to unmount', 'Box renderError', ]); ReactDOM.unmountComponentAtNode(container); @@ -455,7 +454,6 @@ describe('ReactErrorBoundaries', () => { 'error handled', 'BrokenUnmount is attempting to unmount', 'BrokenUnmount is attempting to unmount', - 'BrokenUnmount is attempting to unmount', 'Box renderError', 'Box componentWillUnmount', ]); From 965d12f73aca40a7200fb4da55c58eeb41a66021 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 12 Oct 2016 15:43:41 +0100 Subject: [PATCH 03/23] Remove duplicate expectations from the test --- .../reconciler/__tests__/ReactErrorBoundaries-test.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index b00b7a5f2f4a..9691ec5f56bf 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -445,16 +445,10 @@ describe('ReactErrorBoundaries', () => { 'BrokenUnmount is attempting to unmount', 'Box renderError', ]); + + log.length = 0; ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ - 'Box render', - 'Box componentDidMount', - 'Box render', - 'BrokenUnmount is attempting to unmount', - 'error handled', - 'BrokenUnmount is attempting to unmount', - 'BrokenUnmount is attempting to unmount', - 'Box renderError', 'Box componentWillUnmount', ]); }); From 8236279039b4a10fa1de5f2a488de1910aa1752f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 12 Oct 2016 15:46:06 +0100 Subject: [PATCH 04/23] Fix style issues --- .../__tests__/ReactErrorBoundaries-test.js | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 9691ec5f56bf..0098008278d6 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -303,7 +303,7 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('catches errors on update', function() { + it('catches errors on update', () => { var log = []; class Box extends React.Component { @@ -386,7 +386,7 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('catches componentWillUnmount errors on update', function() { + it('catches componentWillUnmount errors on update', () => { var log = []; class ErrorBoundary extends React.Component { @@ -453,7 +453,7 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('catches componentWillUnmount errors nested children', function() { + it('catches componentWillUnmount errors nested children', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); @@ -498,7 +498,7 @@ describe('ReactErrorBoundaries', () => { ReactDOM.unmountComponentAtNode(container); }); - it('doesn\'t get into inconsistent state during removals', function() { + it('doesn\'t get into inconsistent state during removals', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); @@ -534,13 +534,20 @@ describe('ReactErrorBoundaries', () => { } var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render( + + + + + , + container + ); ReactDOM.render(, container); expect(container.textContent).toBe('Error: I am now a sad component :('); ReactDOM.unmountComponentAtNode(container); }); - it('doesn\'t get into inconsistent state during additions', function() { + it('doesn\'t get into inconsistent state during additions', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); @@ -574,13 +581,19 @@ describe('ReactErrorBoundaries', () => { var container = document.createElement('div'); ReactDOM.render(, container); - ReactDOM.render(, container); + ReactDOM.render( + + + + + , + container + ); expect(container.textContent).toBe('Error: I am now a sad component :('); ReactDOM.unmountComponentAtNode(container); }); - it('doesn\'t get into inconsistent state during reorders', function() { - + it('doesn\'t get into inconsistent state during reorders', () => { function generateElements() { var elements = []; for (var i = 0; i < 100; i++) { @@ -636,9 +649,19 @@ describe('ReactErrorBoundaries', () => { var fail = false; var container = document.createElement('div'); - ReactDOM.render({generateElements()}, container); + ReactDOM.render( + + {generateElements()} + , + container + ); fail = true; - ReactDOM.render({generateElements()}, container); + ReactDOM.render( + + {generateElements()} + , + container + ); expect(container.textContent).toBe('Error: I am now a sad component :('); ReactDOM.unmountComponentAtNode(container); From d09d33f874b9ad69f0a0c98f47479031208dc869 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 12 Oct 2016 16:40:05 +0100 Subject: [PATCH 05/23] Make naming consistent throughout the tests --- .../__tests__/ReactErrorBoundaries-test.js | 386 ++++++++++-------- 1 file changed, 205 insertions(+), 181 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 0098008278d6..3d62633d0af2 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -22,27 +22,29 @@ describe('ReactErrorBoundaries', () => { }); it('does not register event handlers for unmounted children', () => { - class Angry extends React.Component { + class BrokenRender extends React.Component { render() { throw new Error('Please, do not render me.'); } } - class Boundary extends React.Component { + class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {error: false}; } render() { - if (!this.state.error) { - return ( -
- ); - } else { - return
Happy Birthday!
; + if (this.state.error) { + return
Caught an error.
; } + return ( +
+ + +
+ ); } - onClick() { + handleClick() { /* do nothing */ } unstable_handleError() { @@ -53,47 +55,49 @@ describe('ReactErrorBoundaries', () => { var EventPluginHub = require('EventPluginHub'); var container = document.createElement('div'); EventPluginHub.putListener = jest.fn(); - ReactDOM.render(, container); + ReactDOM.render(, container); expect(EventPluginHub.putListener).not.toBeCalled(); }); - it('renders an error state', () => { + it('renders an error state if child throws rendering', () => { var log = []; - class Angry extends React.Component { + class BrokenRender extends React.Component { render() { - log.push('Angry render'); + log.push('BrokenRender render [!]'); throw new Error('Please, do not render me.'); } componentDidMount() { - log.push('Angry componentDidMount'); + log.push('BrokenRender componentDidMount'); } componentWillUnmount() { - log.push('Angry componentWillUnmount'); + log.push('BrokenRender componentWillUnmount'); } } - class Boundary extends React.Component { + class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {error: false}; } render() { - log.push('Boundary render'); - if (!this.state.error) { - return ( -
- ); - } else { - return
Happy Birthday!
; + log.push('ErrorBoundary render'); + if (this.state.error) { + return
Caught an error.
; } + return ( +
+ + +
+ ); } componentDidMount() { - log.push('Boundary componentDidMount'); + log.push('ErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('Boundary componentWillUnmount'); + log.push('ErrorBoundary componentWillUnmount'); } - onClick() { + handleClick() { /* do nothing */ } unstable_handleError() { @@ -102,30 +106,28 @@ describe('ReactErrorBoundaries', () => { } var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Happy Birthday!'); + ReactDOM.render(, container); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); expect(log).toEqual([ - 'Boundary render', - 'Angry render', - 'Boundary render', - 'Boundary componentDidMount', + 'ErrorBoundary render', + 'BrokenRender render [!]', + 'ErrorBoundary render', + 'ErrorBoundary componentDidMount', ]); }); - it('will catch exceptions in componentWillUnmount initial render', () => { + it('catches errors in componentWillUnmount during mounting rollback', () => { class ErrorBoundary extends React.Component { constructor() { super(); this.state = {error: false}; } - render() { - if (!this.state.error) { - return
{this.props.children}
; + if (this.state.error) { + return
Caught an error.
; } - return
Error has been caught
; + return
{this.props.children}
; } - unstable_handleError() { this.setState({error: true}); } @@ -155,28 +157,29 @@ describe('ReactErrorBoundaries', () => {
, container ); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); }); - it('expect uneventful render to succeed', () => { + it('successfully mounts if no error occurs', () => { var log = []; - class Boundary extends React.Component { + class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {error: false}; } render() { - log.push('Boundary render'); - return
; - } - onClick() { - /* do nothing */ + log.push('ErrorBoundary render'); + if (this.state.error) { + return
Caught an error.
; + } + return
Mounted successfully.
; } componentDidMount() { - log.push('Boundary componentDidMount'); + log.push('ErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('Boundary componentWillUnmount'); + log.push('ErrorBoundary componentWillUnmount'); } unstable_handleError() { this.setState({error: true}); @@ -184,69 +187,72 @@ describe('ReactErrorBoundaries', () => { } var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render(, container); + expect(container.firstChild.innerHTML).toBe('Mounted successfully.'); expect(log).toEqual([ - 'Boundary render', - 'Boundary componentDidMount', + 'ErrorBoundary render', + 'ErrorBoundary componentDidMount', ]); }); - it('correctly handles composite siblings', () => { + it('rolls back mounting composite siblings if one of them throws', () => { class ErrorBoundary extends React.Component { constructor() { super(); this.state = {error: false}; } - render() { - if (!this.state.error) { - return
{this.props.children}
; + if (this.state.error) { + return
Caught an error.
; } - return
Error has been caught
; + return
{this.props.children}
; } - unstable_handleError() { this.setState({error: true}); } } - function Broken() { + function BrokenRender() { throw new Error('Always broken.'); } - function Composite() { + function Normal() { return
; } var container = document.createElement('div'); ReactDOM.render( - , + + + + , container ); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); }); - it('catches errors from children', () => { + it('resets refs to composite siblings if one of them throws', () => { var log = []; - class Box extends React.Component { + class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {errorMessage: null}; } render() { if (this.state.errorMessage != null) { - log.push('Box renderError'); - return
Error: {this.state.errorMessage}
; + log.push('ErrorBoundary renderError'); + return
Caught an error: {this.state.errorMessage}
; } - log.push('Box render'); + log.push('ErrorBoundary render'); var ref = function(x) { - log.push('Inquisitive ref ' + x); + log.push('ErrorBoundary ref to Normal is set to ' + x); }; return (
- - + +
); } @@ -254,139 +260,143 @@ describe('ReactErrorBoundaries', () => { this.setState({errorMessage: e.message}); } componentDidMount() { - log.push('Box componentDidMount'); + log.push('ErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('Box componentWillUnmount'); + log.push('ErrorBoundary componentWillUnmount'); } } - class Inquisitive extends React.Component { + class Normal extends React.Component { render() { - log.push('Inquisitive render'); + log.push('Normal render'); return
What is love?
; } componentDidMount() { - log.push('Inquisitive componentDidMount'); + log.push('Normal componentDidMount'); } componentWillUnmount() { - log.push('Inquisitive componentWillUnmount'); + log.push('Normal componentWillUnmount'); } } - class Angry extends React.Component { + class BrokenRender extends React.Component { render() { - log.push('Angry render'); + log.push('BrokenRender render [!]'); throw new Error('Please, do not render me.'); } componentDidMount() { - log.push('Angry componentDidMount'); + log.push('BrokenRender componentDidMount'); } componentWillUnmount() { - log.push('Angry componentWillUnmount'); + log.push('BrokenRender componentWillUnmount'); } } var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.textContent).toBe('Error: Please, do not render me.'); + ReactDOM.render(, container); + expect(container.textContent).toBe('Caught an error: Please, do not render me.'); ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ - 'Box render', - 'Inquisitive render', - 'Angry render', - 'Inquisitive ref null', - 'Inquisitive componentWillUnmount', - 'Box renderError', - 'Box componentDidMount', - 'Box componentWillUnmount', + 'ErrorBoundary render', + 'Normal render', + 'BrokenRender render [!]', + 'ErrorBoundary ref to Normal is set to null', + 'Normal componentWillUnmount', + 'ErrorBoundary renderError', + 'ErrorBoundary componentDidMount', + 'ErrorBoundary componentWillUnmount', ]); }); - it('catches errors on update', () => { + it('catches if child throws in render during update', () => { var log = []; - class Box extends React.Component { + class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {errorMessage: null}; } render() { if (this.state.errorMessage != null) { - log.push('Box renderError'); - return
Error: {this.state.errorMessage}
; + log.push('ErrorBoundary renderError'); + return
Caught an error: {this.state.errorMessage}
; } - log.push('Box render'); + log.push('ErrorBoundary render'); var ref = function(x) { - log.push('Inquisitive ref ' + x); + log.push('ErrorBoundary ref to Normal is set to ' + x); }; return (
- - {this.props.angry ? :
} + + {this.props.renderBrokenChild ? :
}
); } unstable_handleError(e) { - log.push('error handled'); + log.push('ErrorBoundary unstable_handleError'); this.setState({errorMessage: e.message}); } componentDidMount() { - log.push('Box componentDidMount'); + log.push('ErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('Box componentWillUnmount'); + log.push('ErrorBoundary componentWillUnmount'); } } - class Inquisitive extends React.Component { + class Normal extends React.Component { render() { - log.push('Inquisitive render'); + log.push('Normal render'); return
What is love?
; } componentDidMount() { - log.push('Inquisitive componentDidMount'); + log.push('Normal componentDidMount'); } componentWillUnmount() { - log.push('Inquisitive componentWillUnmount'); + log.push('Normal componentWillUnmount'); } } - class Angry extends React.Component { + class BrokenRender extends React.Component { render() { - log.push('Angry render'); + log.push('BrokenRender render [!]'); throw new Error('Please, do not render me.'); } componentDidMount() { - log.push('Angry componentDidMount'); + log.push('BrokenRender componentDidMount'); } componentWillUnmount() { - log.push('Angry componentWillUnmount'); + log.push('BrokenRender componentWillUnmount'); } } var container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render(, container); - expect(container.textContent).toBe('Error: Please, do not render me.'); + ReactDOM.render(, container); expect(log).toEqual([ - 'Box render', - 'Inquisitive render', - 'Inquisitive componentDidMount', - 'Inquisitive ref [object Object]', - 'Box componentDidMount', - 'Box render', - 'Inquisitive ref null', - 'Inquisitive render', - 'Angry render', - 'error handled', - 'Inquisitive ref null', - 'Inquisitive componentWillUnmount', - 'Box renderError', + 'ErrorBoundary render', + 'Normal render', + 'Normal componentDidMount', + 'ErrorBoundary ref to Normal is set to [object Object]', + 'ErrorBoundary componentDidMount', + ]); + + log.length = 0; + ReactDOM.render(, container); + expect(container.textContent).toBe('Caught an error: Please, do not render me.'); + expect(log).toEqual([ + 'ErrorBoundary render', + 'ErrorBoundary ref to Normal is set to null', + 'Normal render', + 'BrokenRender render [!]', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary ref to Normal is set to null', + 'Normal componentWillUnmount', + 'ErrorBoundary renderError', ]); }); - it('catches componentWillUnmount errors on update', () => { + it('recovers from componentWillUnmount errors on update', () => { var log = []; class ErrorBoundary extends React.Component { @@ -396,28 +406,29 @@ describe('ReactErrorBoundaries', () => { } render() { if (this.state.errorMessage != null) { - log.push('Box renderError'); - return
Error: I am now a sad component :(
; + log.push('ErrorBoundary renderError'); + return
Caught an error.
; } - log.push('Box render'); - + log.push('ErrorBoundary render'); return (
- {this.props.angry ? null : } + {this.props.renderChildWithBrokenUnmount && + + }
); } unstable_handleError(e) { - log.push('error handled'); + log.push('ErrorBoundary unstable_handleError'); this.setState({errorMessage: e.message}); } componentDidMount() { - log.push('Box componentDidMount'); + log.push('ErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('Box componentWillUnmount'); + log.push('ErrorBoundary componentWillUnmount'); } } @@ -426,34 +437,40 @@ describe('ReactErrorBoundaries', () => { return
text
; } componentWillUnmount() { - log.push('BrokenUnmount is attempting to unmount'); + log.push('BrokenUnmount componentWillUnmount [!]'); throw new Error('Always broken.'); } } var container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render(, container); - expect(container.textContent).toBe('Error: I am now a sad component :('); + ReactDOM.render( + , + container + ); + ReactDOM.render( + , + container + ); + expect(container.textContent).toBe('Caught an error.'); expect(log).toEqual([ - 'Box render', - 'Box componentDidMount', - 'Box render', - 'BrokenUnmount is attempting to unmount', - 'error handled', - 'BrokenUnmount is attempting to unmount', - 'BrokenUnmount is attempting to unmount', - 'Box renderError', + 'ErrorBoundary render', + 'ErrorBoundary componentDidMount', + 'ErrorBoundary render', + 'BrokenUnmount componentWillUnmount [!]', + 'ErrorBoundary unstable_handleError', + 'BrokenUnmount componentWillUnmount [!]', + 'BrokenUnmount componentWillUnmount [!]', + 'ErrorBoundary renderError', ]); log.length = 0; ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ - 'Box componentWillUnmount', + 'ErrorBoundary componentWillUnmount', ]); }); - it('catches componentWillUnmount errors nested children', () => { + it('recovers from nested componentWillUnmount errors on update', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); @@ -461,13 +478,14 @@ describe('ReactErrorBoundaries', () => { } render() { if (this.state.errorMessage != null) { - return
Error: I am now a sad component :(
; + return
Caught an error: {this.state.errorMessage}
; } - return (
- - {this.props.angry ? null : } + + {this.props.renderChildWithBrokenUnmount && + + }
); } @@ -476,7 +494,7 @@ describe('ReactErrorBoundaries', () => { } } - class InnocentParent extends React.Component { + class RenderBrokenUnmount extends React.Component { render() { return ; } @@ -487,14 +505,22 @@ describe('ReactErrorBoundaries', () => { return
text
; } componentWillUnmount() { - throw new Error('Always broken.'); + throw new Error('I am broken.'); } } var container = document.createElement('div'); - ReactDOM.render(, container); - ReactDOM.render(, container); - expect(container.textContent).toBe('Error: I am now a sad component :('); + ReactDOM.render( + , + container + ); + ReactDOM.render( + , + container + ); + expect(container.textContent).toBe( + 'Caught an error: I am broken.' + ); ReactDOM.unmountComponentAtNode(container); }); @@ -506,9 +532,8 @@ describe('ReactErrorBoundaries', () => { } render() { if (this.state.errorMessage != null) { - return
Error: I am now a sad component :(
; + return
Caught an error.
; } - return (
{this.props.children}
); @@ -518,13 +543,13 @@ describe('ReactErrorBoundaries', () => { } } - class InnocentComponent extends React.Component { + class Normal extends React.Component { render() { return
text
; } } - class GuiltyComponent extends React.Component { + class BrokenUnmount extends React.Component { render() { return
text
; } @@ -536,14 +561,14 @@ describe('ReactErrorBoundaries', () => { var container = document.createElement('div'); ReactDOM.render( - - - + + + , container ); ReactDOM.render(, container); - expect(container.textContent).toBe('Error: I am now a sad component :('); + expect(container.textContent).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); }); @@ -555,9 +580,8 @@ describe('ReactErrorBoundaries', () => { } render() { if (this.state.errorMessage != null) { - return
Error: I am now a sad component :(
; + return
Caught an error.
; } - return (
{this.props.children}
); @@ -567,13 +591,13 @@ describe('ReactErrorBoundaries', () => { } } - class InnocentComponent extends React.Component { + class Normal extends React.Component { render() { return
text
; } } - class GuiltyComponent extends React.Component { + class BrokenRender extends React.Component { render() { throw new Error('Always broken.'); } @@ -583,23 +607,23 @@ describe('ReactErrorBoundaries', () => { ReactDOM.render(, container); ReactDOM.render( - - - + + + , container ); - expect(container.textContent).toBe('Error: I am now a sad component :('); + expect(container.textContent).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); }); it('doesn\'t get into inconsistent state during reorders', () => { - function generateElements() { + function getAMixOfNormalAndBrokenRenderElements() { var elements = []; for (var i = 0; i < 100; i++) { - elements.push(); + elements.push(); } - elements.push(); + elements.push(); var currentIndex = elements.length; while (0 !== currentIndex) { @@ -619,7 +643,7 @@ describe('ReactErrorBoundaries', () => { } render() { if (this.state.errorMessage != null) { - return
Error: I am now a sad component :(
; + return
Caught an error.
; } return ( @@ -631,13 +655,13 @@ describe('ReactErrorBoundaries', () => { } } - class InnocentComponent extends React.Component { + class Normal extends React.Component { render() { return
text
; } } - class GuiltyComponent extends React.Component { + class BrokenRender extends React.Component { render() { if (fail) { throw new Error('Always broken.'); @@ -647,23 +671,23 @@ describe('ReactErrorBoundaries', () => { } var fail = false; - var container = document.createElement('div'); ReactDOM.render( - {generateElements()} + {getAMixOfNormalAndBrokenRenderElements()} , container ); + expect(container.textContent).not.toBe('Caught an error.'); + fail = true; ReactDOM.render( - {generateElements()} + {getAMixOfNormalAndBrokenRenderElements()} , container ); - - expect(container.textContent).toBe('Error: I am now a sad component :('); + expect(container.textContent).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); }); }); From aedd80d2690b4ba63fbc544c062c05625d56642e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 12 Oct 2016 16:50:07 +0100 Subject: [PATCH 06/23] receiveComponent() does not accept safely argument --- .../shared/stack/reconciler/ReactCompositeComponent.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 56fb6c4fc80d..e77408993254 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -1024,8 +1024,7 @@ var ReactCompositeComponent = { prevComponentInstance, nextRenderedElement, transaction, - this._processChildContext(context), - safely + this._processChildContext(context) ); } else { var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance); From 5f869d24bbd6a5ebb87e763f17cf3014f7ddce55 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 12 Oct 2016 16:56:49 +0100 Subject: [PATCH 07/23] Assert that lifecycle and refs fire for error message --- .../__tests__/ReactErrorBoundaries-test.js | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 3d62633d0af2..981458b77fdd 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -320,15 +320,20 @@ describe('ReactErrorBoundaries', () => { render() { if (this.state.errorMessage != null) { log.push('ErrorBoundary renderError'); - return
Caught an error: {this.state.errorMessage}
; + return ( + { + log.push('ErrorBoundary ref to ErrorMessage is set to ' + inst); + }} /> + ); } log.push('ErrorBoundary render'); - var ref = function(x) { - log.push('ErrorBoundary ref to Normal is set to ' + x); - }; return (
- + { + log.push('ErrorBoundary ref to Normal is set to ' + inst); + }} /> {this.props.renderBrokenChild ? :
}
); @@ -345,6 +350,19 @@ describe('ReactErrorBoundaries', () => { } } + class ErrorMessage extends React.Component { + componentWillMount() { + log.push('ErrorMessage componentWillMount'); + } + componentDidMount() { + log.push('ErrorMessage componentDidMount'); + } + render() { + log.push('ErrorMessage render'); + return
Caught an error: {this.props.message}
; + } + } + class Normal extends React.Component { render() { log.push('Normal render'); @@ -393,6 +411,10 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary ref to Normal is set to null', 'Normal componentWillUnmount', 'ErrorBoundary renderError', + 'ErrorMessage componentWillMount', + 'ErrorMessage render', + 'ErrorMessage componentDidMount', + 'ErrorBoundary ref to ErrorMessage is set to [object Object]', ]); }); From 3890a3a90633ed52da4997e6e13d3e523f7eb966 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 13 Oct 2016 21:03:59 +0100 Subject: [PATCH 08/23] Add more tests for mounting --- .../__tests__/ReactErrorBoundaries-test.js | 487 +++++++++++++++++- 1 file changed, 460 insertions(+), 27 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 981458b77fdd..d3bc0ecbc8c1 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -21,51 +21,301 @@ describe('ReactErrorBoundaries', () => { React = require('React'); }); - it('does not register event handlers for unmounted children', () => { + it('renders an error state if child throws in render', () => { + var log = []; class BrokenRender extends React.Component { + constructor(props) { + super(props); + log.push('BrokenRender constructor'); + } render() { + log.push('BrokenRender render [!]'); throw new Error('Please, do not render me.'); } + componentWillMount() { + log.push('BrokenRender componentWillMount'); + } + componentDidMount() { + log.push('BrokenRender componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenRender componentWillUnmount'); + } } class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {error: false}; + log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { + log.push('ErrorBoundary render error'); return
Caught an error.
; } - return ( -
- - -
- ); + log.push('ErrorBoundary render success'); + return ; } - handleClick() { - /* do nothing */ + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorBoundary componentWillUnmount'); } unstable_handleError() { + log.push('ErrorBoundary unstable_handleError'); this.setState({error: true}); } } - var EventPluginHub = require('EventPluginHub'); var container = document.createElement('div'); - EventPluginHub.putListener = jest.fn(); ReactDOM.render(, container); - expect(EventPluginHub.putListener).not.toBeCalled(); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + // FIXME: we should not be callling componentWillMount() twice + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + }); + + it('renders an error state if child throws in constructor', () => { + var log = []; + class BrokenConstructor extends React.Component { + constructor(props) { + super(props); + log.push('BrokenConstructor constructor [!]'); + throw new Error('Bad constructor'); + } + render() { + log.push('BrokenConstructor render'); + } + componentWillMount() { + log.push('BrokenConstructor componentWillMount'); + } + componentDidMount() { + log.push('BrokenConstructor componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenConstructor componentWillUnmount'); + } + } + + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {error: false}; + log.push('ErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('ErrorBoundary render error'); + return
Caught an error.
; + } + log.push('ErrorBoundary render success'); + return ; + } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('ErrorBoundary unstable_handleError'); + this.setState({error: true}); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenConstructor constructor [!]', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + // FIXME: we should not be callling componentWillMount() twice + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + }); + + it('renders an error state if child throws in componentWillMount', () => { + var log = []; + class BrokenComponentWillMount extends React.Component { + constructor(props) { + super(props); + log.push('BrokenComponentWillMount constructor'); + } + componentWillMount() { + log.push('BrokenComponentWillMount componentWillMount [!]'); + throw new Error('Bad componentWillMount'); + } + render() { + log.push('BrokenComponentWillMount render'); + } + componentDidMount() { + log.push('BrokenComponentWillMount componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenComponentWillMount componentWillUnmount'); + } + } + + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {error: false}; + log.push('ErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('ErrorBoundary render error'); + return
Caught an error.
; + } + log.push('ErrorBoundary render success'); + return ; + } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('ErrorBoundary unstable_handleError'); + this.setState({error: true}); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentWillMount constructor', + 'BrokenComponentWillMount componentWillMount [!]', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + // FIXME: we should not be callling componentWillMount() twice + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + }); + + // Known limitation because componentDidMount() does not occur on the stack. + // We could either hardcode searching for parent boundary, or wait for Fiber. + it('currently does not catch errors in componentDidMount', () => { + var log = []; + class BrokenComponentDidMount extends React.Component { + constructor(props) { + super(props); + log.push('BrokenComponentDidMount constructor'); + } + componentWillMount() { + log.push('BrokenComponentDidMount componentWillMount'); + } + render() { + log.push('BrokenComponentDidMount render'); + return
; + } + componentDidMount() { + log.push('BrokenComponentDidMount componentDidMount [!]'); + throw new Error('Bad componentDidMount.'); + } + componentWillUnmount() { + log.push('BrokenComponentDidMount componentWillUnmount'); + } + } + + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {error: false}; + log.push('ErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('ErrorBoundary render error'); + return
Caught an error.
; + } + log.push('ErrorBoundary render success'); + return ; + } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('ErrorBoundary unstable_handleError'); + this.setState({error: true}); + } + } + + var container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow(); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentDidMount constructor', + 'BrokenComponentDidMount componentWillMount', + 'BrokenComponentDidMount render', + 'BrokenComponentDidMount componentDidMount [!]', + // FIXME: uncomment when componentDidMount() gets caught. + // Catch and render an error message + // 'ErrorBoundary unstable_handleError', + // 'ErrorBoundary render error', + // 'ErrorBoundary componentDidMount', + ]); }); - it('renders an error state if child throws rendering', () => { + it('propagates errors on retry on mounting', () => { var log = []; class BrokenRender extends React.Component { + constructor(props) { + super(props); + log.push('BrokenRender constructor'); + } render() { log.push('BrokenRender render [!]'); throw new Error('Please, do not render me.'); } + componentWillMount() { + log.push('BrokenRender componentWillMount'); + } componentDidMount() { log.push('BrokenRender componentDidMount'); } @@ -74,13 +324,191 @@ describe('ReactErrorBoundaries', () => { } } + class UncatchingErrorBoundary extends React.Component { + constructor(props) { + super(props); + log.push('UncatchingErrorBoundary constructor'); + } + render() { + log.push('UncatchingErrorBoundary render'); + return ; + } + componentWillMount() { + log.push('UncatchingErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('UncatchingErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('UncatchingErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('UncatchingErrorBoundary unstable_handleError'); + } + } + + class ParentErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {error: false}; + log.push('ParentErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('ParentErrorBoundary render error'); + return
Caught an error.
; + } + log.push('ParentErrorBoundary render success'); + return ; + } + componentWillMount() { + log.push('ParentErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ParentErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ParentErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('ParentErrorBoundary unstable_handleError'); + this.setState({error: true}); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(log).toEqual([ + 'ParentErrorBoundary constructor', + 'ParentErrorBoundary componentWillMount', + 'ParentErrorBoundary render success', + 'UncatchingErrorBoundary constructor', + 'UncatchingErrorBoundary componentWillMount', + 'UncatchingErrorBoundary render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // The first error boundary catches the error + // However, it doesn't adjust its state so next render also fails + 'UncatchingErrorBoundary unstable_handleError', + // FIXME: we should not be callling componentWillMount() twice + 'UncatchingErrorBoundary componentWillMount', + 'UncatchingErrorBoundary render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // This time, the error propagates to the higher boundary + 'ParentErrorBoundary unstable_handleError', + // Clean up the broken tree + 'UncatchingErrorBoundary componentWillUnmount', + // Render the error + // FIXME: we should not be callling componentWillMount() twice + 'ParentErrorBoundary componentWillMount', + 'ParentErrorBoundary render error', + 'ParentErrorBoundary componentDidMount', + ]); + }); + + it('propagates errors inside boundary itself on mounting', () => { + var log = []; + class BrokenRender extends React.Component { + render() { + log.push('BrokenRender render [!]'); + throw new Error('Please, do not render me.'); + } + componentDidMount() { + log.push('BrokenRender componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenRender componentWillUnmount'); + } + } + + class BrokenErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {error: false}; + } + render() { + if (this.state.error) { + log.push('BrokenErrorBoundary render error [!]'); + throw new Error('You shall not pass.'); + } + log.push('BrokenErrorBoundary render success'); + return ; + } + componentDidMount() { + log.push('BrokenErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('BrokenErrorBoundary unstable_handleError'); + this.setState({error: true}); + } + } + + class ParentErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {error: false}; + } + render() { + if (this.state.error) { + log.push('ParentErrorBoundary render error'); + return
Caught an error.
; + } + log.push('ParentErrorBoundary render success'); + return ; + } + componentDidMount() { + log.push('ParentErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ParentErrorBoundary componentWillUnmount'); + } + unstable_handleError() { + log.push('ParentErrorBoundary unstable_handleError'); + this.setState({error: true}); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(log).toEqual([ + 'ParentErrorBoundary render success', + 'BrokenErrorBoundary render success', + 'BrokenRender render [!]', + // The first error boundary catches the error + // It adjusts state but throws displaying the message + 'BrokenErrorBoundary unstable_handleError', + 'BrokenErrorBoundary render error [!]', + // The error propagates to the higher boundary + 'ParentErrorBoundary unstable_handleError', + // Clean up the broken tree + 'BrokenErrorBoundary componentWillUnmount', + // Render the error + 'ParentErrorBoundary render error', + 'ParentErrorBoundary componentDidMount', + ]); + }); + + it('does not register event handlers for unmounted children', () => { + class BrokenRender extends React.Component { + render() { + throw new Error('Please, do not render me.'); + } + } + class ErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {error: false}; } render() { - log.push('ErrorBoundary render'); if (this.state.error) { return
Caught an error.
; } @@ -91,12 +519,6 @@ describe('ReactErrorBoundaries', () => {
); } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } handleClick() { /* do nothing */ } @@ -105,18 +527,15 @@ describe('ReactErrorBoundaries', () => { } } + var EventPluginHub = require('EventPluginHub'); var container = document.createElement('div'); + EventPluginHub.putListener = jest.fn(); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); - expect(log).toEqual([ - 'ErrorBoundary render', - 'BrokenRender render [!]', - 'ErrorBoundary render', - 'ErrorBoundary componentDidMount', - ]); + expect(EventPluginHub.putListener).not.toBeCalled(); }); it('catches errors in componentWillUnmount during mounting rollback', () => { + var log = []; class ErrorBoundary extends React.Component { constructor() { super(); @@ -124,26 +543,32 @@ describe('ReactErrorBoundaries', () => { } render() { if (this.state.error) { + log.push('ErrorBoundary render error'); return
Caught an error.
; } + log.push('ErrorBoundary render success'); return
{this.props.children}
; } unstable_handleError() { + log.push('ErrorBoundary unstable_handleError'); this.setState({error: true}); } } class BrokenRender extends React.Component { render() { + log.push('BrokenRender render [!]'); throw new Error('Always broken.'); } } class BrokenUnmount extends React.Component { render() { + log.push('BrokenUnmount render'); return
; } componentWillUnmount() { + log.push('BrokenUnmount componentWillUnmount [!]'); throw new Error('Always broken.'); } } @@ -159,6 +584,14 @@ describe('ReactErrorBoundaries', () => { ); expect(container.firstChild.innerHTML).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary render success', + 'BrokenUnmount render', + 'BrokenRender render [!]', + 'ErrorBoundary unstable_handleError', + 'BrokenUnmount componentWillUnmount [!]', + 'ErrorBoundary render error', + ]); }); it('successfully mounts if no error occurs', () => { From 045dcb56ff77f35051f73adc26c7b10d18f9f810 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 13 Oct 2016 21:09:11 +0100 Subject: [PATCH 09/23] Do not call componentWillMount twice on error boundary --- .../reconciler/ReactCompositeComponent.js | 67 ++++++++++++++----- .../__tests__/ReactErrorBoundaries-test.js | 10 --- 2 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index e77408993254..fb26939736bb 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -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) { @@ -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); @@ -448,12 +462,27 @@ var ReactCompositeComponent = { // 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; @@ -461,20 +490,22 @@ var ReactCompositeComponent = { 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) { + 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); + } } } diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index d3bc0ecbc8c1..75b5ffd5b424 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -84,8 +84,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // Catch and render an error message 'ErrorBoundary unstable_handleError', - // FIXME: we should not be callling componentWillMount() twice - 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -152,8 +150,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenConstructor constructor [!]', // Catch and render an error message 'ErrorBoundary unstable_handleError', - // FIXME: we should not be callling componentWillMount() twice - 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -221,8 +217,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillMount componentWillMount [!]', // Catch and render an error message 'ErrorBoundary unstable_handleError', - // FIXME: we should not be callling componentWillMount() twice - 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -392,8 +386,6 @@ describe('ReactErrorBoundaries', () => { // The first error boundary catches the error // However, it doesn't adjust its state so next render also fails 'UncatchingErrorBoundary unstable_handleError', - // FIXME: we should not be callling componentWillMount() twice - 'UncatchingErrorBoundary componentWillMount', 'UncatchingErrorBoundary render', 'BrokenRender constructor', 'BrokenRender componentWillMount', @@ -403,8 +395,6 @@ describe('ReactErrorBoundaries', () => { // Clean up the broken tree 'UncatchingErrorBoundary componentWillUnmount', // Render the error - // FIXME: we should not be callling componentWillMount() twice - 'ParentErrorBoundary componentWillMount', 'ParentErrorBoundary render error', 'ParentErrorBoundary componentDidMount', ]); From b69d428d9e1888ee4dd00a0e2f0ef61b39b53734 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 13 Oct 2016 21:32:11 +0100 Subject: [PATCH 10/23] Document more of existing behavior in tests --- .../__tests__/ReactErrorBoundaries-test.js | 101 +++++++++++++++++- 1 file changed, 99 insertions(+), 2 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 75b5ffd5b424..7e88a16c2427 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -403,10 +403,17 @@ describe('ReactErrorBoundaries', () => { it('propagates errors inside boundary itself on mounting', () => { var log = []; class BrokenRender extends React.Component { + constructor(props) { + super(props); + log.push('BrokenRender constructor'); + } render() { log.push('BrokenRender render [!]'); throw new Error('Please, do not render me.'); } + componentWillMount() { + log.push('BrokenRender componentWillMount'); + } componentDidMount() { log.push('BrokenRender componentDidMount'); } @@ -419,6 +426,7 @@ describe('ReactErrorBoundaries', () => { constructor(props) { super(props); this.state = {error: false}; + log.push('BrokenErrorBoundary constructor'); } render() { if (this.state.error) { @@ -428,6 +436,9 @@ describe('ReactErrorBoundaries', () => { log.push('BrokenErrorBoundary render success'); return ; } + componentWillMount() { + log.push('BrokenErrorBoundary componentWillMount'); + } componentDidMount() { log.push('BrokenErrorBoundary componentDidMount'); } @@ -444,6 +455,7 @@ describe('ReactErrorBoundaries', () => { constructor(props) { super(props); this.state = {error: false}; + log.push('ParentErrorBoundary constructor'); } render() { if (this.state.error) { @@ -453,6 +465,9 @@ describe('ReactErrorBoundaries', () => { log.push('ParentErrorBoundary render success'); return ; } + componentWillMount() { + log.push('ParentErrorBoundary componentWillMount'); + } componentDidMount() { log.push('ParentErrorBoundary componentDidMount'); } @@ -469,8 +484,14 @@ describe('ReactErrorBoundaries', () => { ReactDOM.render(, container); expect(container.firstChild.innerHTML).toBe('Caught an error.'); expect(log).toEqual([ + 'ParentErrorBoundary constructor', + 'ParentErrorBoundary componentWillMount', 'ParentErrorBoundary render success', + 'BrokenErrorBoundary constructor', + 'BrokenErrorBoundary componentWillMount', 'BrokenErrorBoundary render success', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', 'BrokenRender render [!]', // The first error boundary catches the error // It adjusts state but throws displaying the message @@ -530,6 +551,7 @@ describe('ReactErrorBoundaries', () => { constructor() { super(); this.state = {error: false}; + log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { @@ -543,30 +565,81 @@ describe('ReactErrorBoundaries', () => { log.push('ErrorBoundary unstable_handleError'); this.setState({error: true}); } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } } class BrokenRender extends React.Component { + constructor(props) { + super(props); + log.push('BrokenRender constructor'); + } render() { log.push('BrokenRender render [!]'); - throw new Error('Always broken.'); + throw new Error('Please, do not render me.'); + } + componentWillMount() { + log.push('BrokenRender componentWillMount'); + } + componentDidMount() { + log.push('BrokenRender componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenRender componentWillUnmount'); } } class BrokenUnmount extends React.Component { + constructor(props) { + super(props); + log.push('BrokenUnmount constructor'); + } render() { log.push('BrokenUnmount render'); return
; } + componentWillMount() { + log.push('BrokenUnmount componentWillMount'); + } + componentDidMount() { + log.push('BrokenUnmount componentDidMount'); + } componentWillUnmount() { log.push('BrokenUnmount componentWillUnmount [!]'); - throw new Error('Always broken.'); + throw new Error('Please, do not unmount me.'); + } + } + + class Normal extends React.Component { + constructor(props) { + super(props); + log.push('Normal constructor'); + } + render() { + log.push('Normal render'); + return
; + } + componentWillMount() { + log.push('Normal componentWillMount'); + } + componentDidMount() { + log.push('Normal componentDidMount'); + } + componentWillUnmount() { + log.push('Normal componentWillUnmount'); } } var container = document.createElement('div'); ReactDOM.render( + + , @@ -575,12 +648,36 @@ describe('ReactErrorBoundaries', () => { expect(container.firstChild.innerHTML).toBe('Caught an error.'); ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', + // Render first child + 'Normal constructor', + 'Normal componentWillMount', + 'Normal render', + // Render second child (it will throw later) + 'BrokenUnmount constructor', + 'BrokenUnmount componentWillMount', 'BrokenUnmount render', + // Render third child + 'Normal constructor', + 'Normal componentWillMount', + 'Normal render', + // Render fourth child (it throws) + 'BrokenRender constructor', + 'BrokenRender componentWillMount', 'BrokenRender render [!]', + // Error boundary catches the error 'ErrorBoundary unstable_handleError', + // Try to clean up already mounted children + // even if their siblings throw during cleanup + // FIXME: why are we doing this? They never got componentDidMount(): + 'Normal componentWillUnmount', 'BrokenUnmount componentWillUnmount [!]', + 'Normal componentWillUnmount', + // Render the error message 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', ]); }); From e374fc793356e901e99474c23629e14184abaca5 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Thu, 13 Oct 2016 23:55:49 +0100 Subject: [PATCH 11/23] 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. --- src/renderers/dom/client/ReactMount.js | 11 +- src/renderers/dom/shared/ReactDOMComponent.js | 4 +- .../native/ReactNativeBaseComponent.js | 4 +- .../stack/reconciler/ReactChildReconciler.js | 16 ++- .../reconciler/ReactCompositeComponent.js | 47 ++++-- .../stack/reconciler/ReactMultiChild.js | 20 ++- .../stack/reconciler/ReactReconciler.js | 4 +- .../reconciler/ReactSimpleEmptyComponent.js | 8 +- .../__tests__/ReactErrorBoundaries-test.js | 136 ++++++++++++++++-- src/renderers/testing/ReactTestMount.js | 3 +- src/test/ReactShallowRenderer.js | 6 +- 11 files changed, 217 insertions(+), 42 deletions(-) diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index c0e3c9ec00e2..4c9a77f8c7db 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -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(); } @@ -618,8 +622,7 @@ var ReactMount = { ReactUpdates.batchedUpdates( unmountComponentFromNode, prevComponent, - container, - false + container ); return true; }, diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index ef75df0c42b8..0da695492847 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -1155,7 +1155,7 @@ ReactDOMComponent.Mixin = { * * @internal */ - unmountComponent: function(safely) { + unmountComponent: function(safely, skipLifecycle) { switch (this._tag) { case 'audio': case 'form': @@ -1197,7 +1197,7 @@ ReactDOMComponent.Mixin = { break; } - this.unmountChildren(safely); + this.unmountChildren(safely, skipLifecycle); ReactDOMComponentTree.uncacheNode(this); EventPluginHub.deleteAllListeners(this); this._rootNodeID = 0; diff --git a/src/renderers/native/ReactNativeBaseComponent.js b/src/renderers/native/ReactNativeBaseComponent.js index 2192b019fc4a..1d47e9dbdfde 100644 --- a/src/renderers/native/ReactNativeBaseComponent.js +++ b/src/renderers/native/ReactNativeBaseComponent.js @@ -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; }, diff --git a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js index d487e70999f0..dc58ff648e06 100644 --- a/src/renderers/shared/stack/reconciler/ReactChildReconciler.js +++ b/src/renderers/shared/stack/reconciler/ReactChildReconciler.js @@ -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); @@ -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 */ + ); } } }, @@ -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); } } }, diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index fb26939736bb..4e54a8439a38 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -457,7 +457,11 @@ 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. @@ -551,7 +555,7 @@ var ReactCompositeComponent = { * @final * @internal */ - unmountComponent: function(safely) { + unmountComponent: function(safely, skipLifecycle) { if (!this._renderedComponent) { return; } @@ -562,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( @@ -578,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; @@ -1016,7 +1026,12 @@ var ReactCompositeComponent = { checkpoint = transaction.checkpoint(); // Gracefully update to a clean state - this._updateRenderedComponentWithNextElement(transaction, context, null, true); + 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). @@ -1032,7 +1047,12 @@ var ReactCompositeComponent = { */ _updateRenderedComponent: function(transaction, context) { var nextRenderedElement = this._renderValidatedComponent(); - this._updateRenderedComponentWithNextElement(transaction, context, nextRenderedElement, false); + this._updateRenderedComponentWithNextElement( + transaction, + context, + nextRenderedElement, + false /* safely */ + ); }, /** @@ -1041,7 +1061,12 @@ var ReactCompositeComponent = { * @param {ReactReconcileTransaction} transaction * @internal */ - _updateRenderedComponentWithNextElement: function(transaction, context, nextRenderedElement, safely) { + _updateRenderedComponentWithNextElement: function( + transaction, + context, + nextRenderedElement, + safely + ) { var prevComponentInstance = this._renderedComponent; var prevRenderedElement = prevComponentInstance._currentElement; @@ -1059,7 +1084,11 @@ var ReactCompositeComponent = { ); } else { var oldHostNode = ReactReconciler.getHostNode(prevComponentInstance); - ReactReconciler.unmountComponent(prevComponentInstance, safely); + ReactReconciler.unmountComponent( + prevComponentInstance, + safely, + false /* skipLifecycle */ + ); var nodeType = ReactNodeTypes.getType(nextRenderedElement); this._renderedNodeType = nodeType; diff --git a/src/renderers/shared/stack/reconciler/ReactMultiChild.js b/src/renderers/shared/stack/reconciler/ReactMultiChild.js index 220de49f3e6b..18fa1e8d0bd9 100644 --- a/src/renderers/shared/stack/reconciler/ReactMultiChild.js +++ b/src/renderers/shared/stack/reconciler/ReactMultiChild.js @@ -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.'); @@ -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.'); @@ -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; }, diff --git a/src/renderers/shared/stack/reconciler/ReactReconciler.js b/src/renderers/shared/stack/reconciler/ReactReconciler.js index 3e5f1658ca8c..f55bf339ee7a 100644 --- a/src/renderers/shared/stack/reconciler/ReactReconciler.js +++ b/src/renderers/shared/stack/reconciler/ReactReconciler.js @@ -93,7 +93,7 @@ var ReactReconciler = { * @final * @internal */ - unmountComponent: function(internalInstance, safely) { + unmountComponent: function(internalInstance, safely, skipLifecycle) { if (__DEV__) { if (internalInstance._debugID !== 0) { ReactInstrumentation.debugTool.onBeforeUnmountComponent( @@ -102,7 +102,7 @@ var ReactReconciler = { } } ReactRef.detachRefs(internalInstance, internalInstance._currentElement); - internalInstance.unmountComponent(safely); + internalInstance.unmountComponent(safely, skipLifecycle); if (__DEV__) { if (internalInstance._debugID !== 0) { ReactInstrumentation.debugTool.onUnmountComponent( diff --git a/src/renderers/shared/stack/reconciler/ReactSimpleEmptyComponent.js b/src/renderers/shared/stack/reconciler/ReactSimpleEmptyComponent.js index a81793eab56c..dc3c82077f4f 100644 --- a/src/renderers/shared/stack/reconciler/ReactSimpleEmptyComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactSimpleEmptyComponent.js @@ -40,8 +40,12 @@ Object.assign(ReactSimpleEmptyComponent.prototype, { getHostNode: function() { return ReactReconciler.getHostNode(this._renderedComponent); }, - unmountComponent: function() { - ReactReconciler.unmountComponent(this._renderedComponent); + unmountComponent: function(safely, skipLifecycle) { + ReactReconciler.unmountComponent( + this._renderedComponent, + safely, + skipLifecycle + ); this._renderedComponent = null; }, }); diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 7e88a16c2427..5ea0b35f5632 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -392,8 +392,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // This time, the error propagates to the higher boundary 'ParentErrorBoundary unstable_handleError', - // Clean up the broken tree - 'UncatchingErrorBoundary componentWillUnmount', // Render the error 'ParentErrorBoundary render error', 'ParentErrorBoundary componentDidMount', @@ -499,8 +497,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenErrorBoundary render error [!]', // The error propagates to the higher boundary 'ParentErrorBoundary unstable_handleError', - // Clean up the broken tree - 'BrokenErrorBoundary componentWillUnmount', // Render the error 'ParentErrorBoundary render error', 'ParentErrorBoundary componentDidMount', @@ -669,12 +665,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // Error boundary catches the error 'ErrorBoundary unstable_handleError', - // Try to clean up already mounted children - // even if their siblings throw during cleanup - // FIXME: why are we doing this? They never got componentDidMount(): - 'Normal componentWillUnmount', - 'BrokenUnmount componentWillUnmount [!]', - 'Normal componentWillUnmount', // Render the error message 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', @@ -822,7 +812,6 @@ describe('ReactErrorBoundaries', () => { 'Normal render', 'BrokenRender render [!]', 'ErrorBoundary ref to Normal is set to null', - 'Normal componentWillUnmount', 'ErrorBoundary renderError', 'ErrorBoundary componentDidMount', 'ErrorBoundary componentWillUnmount', @@ -938,6 +927,131 @@ describe('ReactErrorBoundaries', () => { ]); }); + it('skips hook for not-yet-mounted child if sibling throws in update', () => { + var log = []; + + class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {errorMessage: null}; + } + render() { + if (this.state.errorMessage != null) { + log.push('ErrorBoundary renderError'); + return ( + { + log.push('ErrorBoundary ref to ErrorMessage is set to ' + inst); + }} /> + ); + } + log.push('ErrorBoundary render'); + return ( +
+ { + log.push('ErrorBoundary ref to Normal is set to ' + inst); + }} /> + {this.props.renderBrokenChild && } + {this.props.renderBrokenChild ? :
} +
+ ); + } + unstable_handleError(e) { + log.push('ErrorBoundary unstable_handleError'); + this.setState({errorMessage: e.message}); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorBoundary componentWillUnmount'); + } + } + + class ErrorMessage extends React.Component { + componentWillMount() { + log.push('ErrorMessage componentWillMount'); + } + componentDidMount() { + log.push('ErrorMessage componentDidMount'); + } + render() { + log.push('ErrorMessage render'); + return
Caught an error: {this.props.message}
; + } + } + + class Normal extends React.Component { + render() { + log.push('Normal render'); + return
What is love?
; + } + componentDidMount() { + log.push('Normal componentDidMount'); + } + componentWillUnmount() { + log.push('Normal componentWillUnmount'); + } + } + + class Normal2 extends React.Component { + render() { + log.push('Normal2 render'); + return
What is love?
; + } + componentDidMount() { + log.push('Normal2 componentDidMount'); + } + componentWillUnmount() { + log.push('Normal2 componentWillUnmount'); + } + } + + class BrokenRender extends React.Component { + render() { + log.push('BrokenRender render [!]'); + throw new Error('Please, do not render me.'); + } + componentDidMount() { + log.push('BrokenRender componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenRender componentWillUnmount'); + } + } + + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(log).toEqual([ + 'ErrorBoundary render', + 'Normal render', + 'Normal componentDidMount', + 'ErrorBoundary ref to Normal is set to [object Object]', + 'ErrorBoundary componentDidMount', + ]); + + log.length = 0; + ReactDOM.render(, container); + expect(container.textContent).toBe('Caught an error: Please, do not render me.'); + expect(log).toEqual([ + 'ErrorBoundary render', + 'ErrorBoundary ref to Normal is set to null', + 'Normal render', + 'Normal2 render', + 'BrokenRender render [!]', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary ref to Normal is set to null', + 'Normal componentWillUnmount', + // Normal2 doesn't get componentWillUnmount() since it never fully mounted + 'ErrorBoundary renderError', + 'ErrorMessage componentWillMount', + 'ErrorMessage render', + 'ErrorMessage componentDidMount', + 'ErrorBoundary ref to ErrorMessage is set to [object Object]', + ]); + }); + it('recovers from componentWillUnmount errors on update', () => { var log = []; diff --git a/src/renderers/testing/ReactTestMount.js b/src/renderers/testing/ReactTestMount.js index f13e5d20f6d7..bed525c4315e 100644 --- a/src/renderers/testing/ReactTestMount.js +++ b/src/renderers/testing/ReactTestMount.js @@ -128,7 +128,8 @@ ReactTestInstance.prototype.unmount = function(nextElement) { transaction.perform(function() { ReactReconciler.unmountComponent( component, - false + false, /* safely */ + false /* skipLifecycle */ ); }); ReactUpdates.ReactReconcileTransaction.release(transaction); diff --git a/src/test/ReactShallowRenderer.js b/src/test/ReactShallowRenderer.js index e16f8476117e..ca2e245b1f99 100644 --- a/src/test/ReactShallowRenderer.js +++ b/src/test/ReactShallowRenderer.js @@ -117,7 +117,11 @@ class ReactShallowRenderer { } unmount() { if (this._instance) { - ReactReconciler.unmountComponent(this._instance, false); + ReactReconciler.unmountComponent( + this._instance, + false, /* safely */ + false /* skipLifecycle */ + ); } } _render(element, transaction, context) { From 1fa0b364e42cbff5d3f66d84f2f1476e4682ea75 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 14:35:30 +0100 Subject: [PATCH 12/23] Consistently display error messages in tests --- .../__tests__/ReactErrorBoundaries-test.js | 252 ++++++++---------- 1 file changed, 113 insertions(+), 139 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 5ea0b35f5632..3977672f01fc 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -30,7 +30,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentWillMount() { log.push('BrokenRender componentWillMount'); @@ -46,13 +46,13 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {error: false}; + this.state = {error: null}; log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { log.push('ErrorBoundary render error'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ErrorBoundary render success'); return ; @@ -66,15 +66,17 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('ErrorBoundary componentWillUnmount'); } - unstable_handleError() { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({error: true}); + this.setState({error}); } } var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe( + 'Caught an error: Hello.' + ); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', @@ -95,7 +97,7 @@ describe('ReactErrorBoundaries', () => { constructor(props) { super(props); log.push('BrokenConstructor constructor [!]'); - throw new Error('Bad constructor'); + throw new Error('Hello'); } render() { log.push('BrokenConstructor render'); @@ -114,13 +116,13 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {error: false}; + this.state = {error: null}; log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { log.push('ErrorBoundary render error'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ErrorBoundary render success'); return ; @@ -134,15 +136,15 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('ErrorBoundary componentWillUnmount'); } - unstable_handleError() { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({error: true}); + this.setState({error}); } } var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', @@ -164,7 +166,7 @@ describe('ReactErrorBoundaries', () => { } componentWillMount() { log.push('BrokenComponentWillMount componentWillMount [!]'); - throw new Error('Bad componentWillMount'); + throw new Error('Hello'); } render() { log.push('BrokenComponentWillMount render'); @@ -180,13 +182,13 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {error: false}; + this.state = {error: null}; log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { log.push('ErrorBoundary render error'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ErrorBoundary render success'); return ; @@ -200,15 +202,15 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('ErrorBoundary componentWillUnmount'); } - unstable_handleError() { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({error: true}); + this.setState({error}); } } var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', @@ -240,7 +242,7 @@ describe('ReactErrorBoundaries', () => { } componentDidMount() { log.push('BrokenComponentDidMount componentDidMount [!]'); - throw new Error('Bad componentDidMount.'); + throw new Error('Hello'); } componentWillUnmount() { log.push('BrokenComponentDidMount componentWillUnmount'); @@ -305,7 +307,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentWillMount() { log.push('BrokenRender componentWillMount'); @@ -344,13 +346,13 @@ describe('ReactErrorBoundaries', () => { class ParentErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {error: false}; + this.state = {error: null}; log.push('ParentErrorBoundary constructor'); } render() { if (this.state.error) { log.push('ParentErrorBoundary render error'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ParentErrorBoundary render success'); return ; @@ -364,15 +366,15 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('ParentErrorBoundary componentWillUnmount'); } - unstable_handleError() { + unstable_handleError(error) { log.push('ParentErrorBoundary unstable_handleError'); - this.setState({error: true}); + this.setState({error}); } } var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ParentErrorBoundary constructor', 'ParentErrorBoundary componentWillMount', @@ -407,7 +409,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentWillMount() { log.push('BrokenRender componentWillMount'); @@ -429,7 +431,7 @@ describe('ReactErrorBoundaries', () => { render() { if (this.state.error) { log.push('BrokenErrorBoundary render error [!]'); - throw new Error('You shall not pass.'); + throw new Error('Hello'); } log.push('BrokenErrorBoundary render success'); return ; @@ -458,7 +460,7 @@ describe('ReactErrorBoundaries', () => { render() { if (this.state.error) { log.push('ParentErrorBoundary render error'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ParentErrorBoundary render success'); return ; @@ -472,15 +474,15 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('ParentErrorBoundary componentWillUnmount'); } - unstable_handleError() { + unstable_handleError(error) { log.push('ParentErrorBoundary unstable_handleError'); - this.setState({error: true}); + this.setState({error}); } } var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ParentErrorBoundary constructor', 'ParentErrorBoundary componentWillMount', @@ -506,7 +508,7 @@ describe('ReactErrorBoundaries', () => { it('does not register event handlers for unmounted children', () => { class BrokenRender extends React.Component { render() { - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } } @@ -541,25 +543,25 @@ describe('ReactErrorBoundaries', () => { expect(EventPluginHub.putListener).not.toBeCalled(); }); - it('catches errors in componentWillUnmount during mounting rollback', () => { + it('does not call componentWillUnmount when aborting initial mount', () => { var log = []; class ErrorBoundary extends React.Component { constructor() { super(); - this.state = {error: false}; + this.state = {error: null}; log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { log.push('ErrorBoundary render error'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ErrorBoundary render success'); return
{this.props.children}
; } - unstable_handleError() { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({error: true}); + this.setState({error}); } componentWillMount() { log.push('ErrorBoundary componentWillMount'); @@ -576,7 +578,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentWillMount() { log.push('BrokenRender componentWillMount'); @@ -589,27 +591,6 @@ describe('ReactErrorBoundaries', () => { } } - class BrokenUnmount extends React.Component { - constructor(props) { - super(props); - log.push('BrokenUnmount constructor'); - } - render() { - log.push('BrokenUnmount render'); - return
; - } - componentWillMount() { - log.push('BrokenUnmount componentWillMount'); - } - componentDidMount() { - log.push('BrokenUnmount componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenUnmount componentWillUnmount [!]'); - throw new Error('Please, do not unmount me.'); - } - } - class Normal extends React.Component { constructor(props) { super(props); @@ -633,15 +614,12 @@ describe('ReactErrorBoundaries', () => { var container = document.createElement('div'); ReactDOM.render( - - - , container ); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ 'ErrorBoundary constructor', @@ -651,15 +629,7 @@ describe('ReactErrorBoundaries', () => { 'Normal constructor', 'Normal componentWillMount', 'Normal render', - // Render second child (it will throw later) - 'BrokenUnmount constructor', - 'BrokenUnmount componentWillMount', - 'BrokenUnmount render', - // Render third child - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', - // Render fourth child (it throws) + // Render second child (it throws) 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', @@ -677,6 +647,7 @@ describe('ReactErrorBoundaries', () => { constructor(props) { super(props); this.state = {error: false}; + log.push('ErrorBoundary constructor'); } render() { log.push('ErrorBoundary render'); @@ -685,6 +656,9 @@ describe('ReactErrorBoundaries', () => { } return
Mounted successfully.
; } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } componentDidMount() { log.push('ErrorBoundary componentDidMount'); } @@ -698,8 +672,10 @@ describe('ReactErrorBoundaries', () => { var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.firstChild.innerHTML).toBe('Mounted successfully.'); + expect(container.firstChild.textContent).toBe('Mounted successfully.'); expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render', 'ErrorBoundary componentDidMount', ]); @@ -709,21 +685,21 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor() { super(); - this.state = {error: false}; + this.state = {error: null}; } render() { if (this.state.error) { - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } return
{this.props.children}
; } - unstable_handleError() { - this.setState({error: true}); + unstable_handleError(error) { + this.setState({error}); } } function BrokenRender() { - throw new Error('Always broken.'); + throw new Error('Hello'); } function Normal() { @@ -738,7 +714,7 @@ describe('ReactErrorBoundaries', () => {
, container ); - expect(container.firstChild.innerHTML).toBe('Caught an error.'); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); }); @@ -748,12 +724,12 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { + if (this.state.error) { log.push('ErrorBoundary renderError'); - return
Caught an error: {this.state.errorMessage}
; + return
Caught an error: {this.state.error.message}.
; } log.push('ErrorBoundary render'); var ref = function(x) { @@ -766,8 +742,8 @@ describe('ReactErrorBoundaries', () => {
); } - unstable_handleError(e) { - this.setState({errorMessage: e.message}); + unstable_handleError(error) { + this.setState({error}); } componentDidMount() { log.push('ErrorBoundary componentDidMount'); @@ -793,7 +769,7 @@ describe('ReactErrorBoundaries', () => { class BrokenRender extends React.Component { render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentDidMount() { log.push('BrokenRender componentDidMount'); @@ -805,7 +781,7 @@ describe('ReactErrorBoundaries', () => { var container = document.createElement('div'); ReactDOM.render(, container); - expect(container.textContent).toBe('Caught an error: Please, do not render me.'); + expect(container.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ 'ErrorBoundary render', @@ -824,14 +800,14 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { + if (this.state.error) { log.push('ErrorBoundary renderError'); return ( { log.push('ErrorBoundary ref to ErrorMessage is set to ' + inst); }} /> @@ -847,9 +823,9 @@ describe('ReactErrorBoundaries', () => {
); } - unstable_handleError(e) { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({errorMessage: e.message}); + this.setState({error}); } componentDidMount() { log.push('ErrorBoundary componentDidMount'); @@ -868,7 +844,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('ErrorMessage render'); - return
Caught an error: {this.props.message}
; + return
Caught an error: {this.props.message}.
; } } @@ -888,7 +864,7 @@ describe('ReactErrorBoundaries', () => { class BrokenRender extends React.Component { render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentDidMount() { log.push('BrokenRender componentDidMount'); @@ -910,7 +886,7 @@ describe('ReactErrorBoundaries', () => { log.length = 0; ReactDOM.render(, container); - expect(container.textContent).toBe('Caught an error: Please, do not render me.'); + expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary render', 'ErrorBoundary ref to Normal is set to null', @@ -933,14 +909,14 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { + if (this.state.error) { log.push('ErrorBoundary renderError'); return ( { log.push('ErrorBoundary ref to ErrorMessage is set to ' + inst); }} /> @@ -957,9 +933,9 @@ describe('ReactErrorBoundaries', () => {
); } - unstable_handleError(e) { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({errorMessage: e.message}); + this.setState({error}); } componentDidMount() { log.push('ErrorBoundary componentDidMount'); @@ -978,7 +954,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('ErrorMessage render'); - return
Caught an error: {this.props.message}
; + return
Caught an error: {this.props.message}.
; } } @@ -1011,7 +987,7 @@ describe('ReactErrorBoundaries', () => { class BrokenRender extends React.Component { render() { log.push('BrokenRender render [!]'); - throw new Error('Please, do not render me.'); + throw new Error('Hello'); } componentDidMount() { log.push('BrokenRender componentDidMount'); @@ -1033,7 +1009,7 @@ describe('ReactErrorBoundaries', () => { log.length = 0; ReactDOM.render(, container); - expect(container.textContent).toBe('Caught an error: Please, do not render me.'); + expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary render', 'ErrorBoundary ref to Normal is set to null', @@ -1058,12 +1034,12 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { + if (this.state.error) { log.push('ErrorBoundary renderError'); - return
Caught an error.
; + return
Caught an error: {this.state.error.message}.
; } log.push('ErrorBoundary render'); return ( @@ -1076,9 +1052,9 @@ describe('ReactErrorBoundaries', () => {
); } - unstable_handleError(e) { + unstable_handleError(error) { log.push('ErrorBoundary unstable_handleError'); - this.setState({errorMessage: e.message}); + this.setState({error}); } componentDidMount() { log.push('ErrorBoundary componentDidMount'); @@ -1094,7 +1070,7 @@ describe('ReactErrorBoundaries', () => { } componentWillUnmount() { log.push('BrokenUnmount componentWillUnmount [!]'); - throw new Error('Always broken.'); + throw new Error('Hello'); } } @@ -1107,7 +1083,7 @@ describe('ReactErrorBoundaries', () => { , container ); - expect(container.textContent).toBe('Caught an error.'); + expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary render', 'ErrorBoundary componentDidMount', @@ -1130,11 +1106,11 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { - return
Caught an error: {this.state.errorMessage}
; + if (this.state.error) { + return
Caught an error: {this.state.error.message}.
; } return (
@@ -1145,8 +1121,8 @@ describe('ReactErrorBoundaries', () => {
); } - unstable_handleError(e) { - this.setState({errorMessage: e.message}); + unstable_handleError(error) { + this.setState({error}); } } @@ -1161,7 +1137,7 @@ describe('ReactErrorBoundaries', () => { return
text
; } componentWillUnmount() { - throw new Error('I am broken.'); + throw new Error('Hello'); } } @@ -1174,9 +1150,7 @@ describe('ReactErrorBoundaries', () => { , container ); - expect(container.textContent).toBe( - 'Caught an error: I am broken.' - ); + expect(container.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); }); @@ -1184,18 +1158,18 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { - return
Caught an error.
; + if (this.state.error) { + return
Caught an error: {this.state.error.message}.
; } return (
{this.props.children}
); } - unstable_handleError(e) { - this.setState({errorMessage: e.message}); + unstable_handleError(error) { + this.setState({error}); } } @@ -1210,7 +1184,7 @@ describe('ReactErrorBoundaries', () => { return
text
; } componentWillUnmount() { - throw new Error('Always broken.'); + throw new Error('Hello'); } } @@ -1224,7 +1198,7 @@ describe('ReactErrorBoundaries', () => { container ); ReactDOM.render(, container); - expect(container.textContent).toBe('Caught an error.'); + expect(container.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); }); @@ -1232,18 +1206,18 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { - return
Caught an error.
; + if (this.state.error) { + return
Caught an error: {this.state.error.message}.
; } return (
{this.props.children}
); } - unstable_handleError(e) { - this.setState({errorMessage: e.message}); + unstable_handleError(error) { + this.setState({error}); } } @@ -1255,7 +1229,7 @@ describe('ReactErrorBoundaries', () => { class BrokenRender extends React.Component { render() { - throw new Error('Always broken.'); + throw new Error('Hello'); } } @@ -1269,7 +1243,7 @@ describe('ReactErrorBoundaries', () => {
, container ); - expect(container.textContent).toBe('Caught an error.'); + expect(container.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); }); @@ -1295,19 +1269,19 @@ describe('ReactErrorBoundaries', () => { class ErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {errorMessage: null}; + this.state = {error: null}; } render() { - if (this.state.errorMessage != null) { - return
Caught an error.
; + if (this.state.error) { + return
Caught an error: {this.state.error.message}.
; } return (
{this.props.children}
); } - unstable_handleError(e) { - this.setState({errorMessage: e.message}); + unstable_handleError(error) { + this.setState({error}); } } @@ -1320,7 +1294,7 @@ describe('ReactErrorBoundaries', () => { class BrokenRender extends React.Component { render() { if (fail) { - throw new Error('Always broken.'); + throw new Error('Hello'); } return
text
; } @@ -1343,7 +1317,7 @@ describe('ReactErrorBoundaries', () => {
, container ); - expect(container.textContent).toBe('Caught an error.'); + expect(container.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); }); }); From 8066a5031f9dc26b46d55d21a3520959b8051feb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 14:47:02 +0100 Subject: [PATCH 13/23] Add more logging to tests and remove redundant one --- .../__tests__/ReactErrorBoundaries-test.js | 62 ++++++++----------- 1 file changed, 25 insertions(+), 37 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 3977672f01fc..54839d64a240 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -616,6 +616,7 @@ describe('ReactErrorBoundaries', () => { + , container ); @@ -681,43 +682,6 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('rolls back mounting composite siblings if one of them throws', () => { - class ErrorBoundary extends React.Component { - constructor() { - super(); - this.state = {error: null}; - } - render() { - if (this.state.error) { - return
Caught an error: {this.state.error.message}.
; - } - return
{this.props.children}
; - } - unstable_handleError(error) { - this.setState({error}); - } - } - - function BrokenRender() { - throw new Error('Hello'); - } - - function Normal() { - return
; - } - - var container = document.createElement('div'); - ReactDOM.render( - - - - , - container - ); - expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); - ReactDOM.unmountComponentAtNode(container); - }); - it('resets refs to composite siblings if one of them throws', () => { var log = []; @@ -725,6 +689,7 @@ describe('ReactErrorBoundaries', () => { constructor(props) { super(props); this.state = {error: null}; + log.push('ErrorBoundary constructor'); } render() { if (this.state.error) { @@ -745,6 +710,9 @@ describe('ReactErrorBoundaries', () => { unstable_handleError(error) { this.setState({error}); } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } componentDidMount() { log.push('ErrorBoundary componentDidMount'); } @@ -754,10 +722,17 @@ describe('ReactErrorBoundaries', () => { } class Normal extends React.Component { + constructor(props) { + super(props); + log.push('Normal constructor'); + } render() { log.push('Normal render'); return
What is love?
; } + componentWillMount() { + log.push('Normal componentWillMount'); + } componentDidMount() { log.push('Normal componentDidMount'); } @@ -767,10 +742,17 @@ describe('ReactErrorBoundaries', () => { } class BrokenRender extends React.Component { + constructor(props) { + super(props); + log.push('BrokenRender constructor'); + } render() { log.push('BrokenRender render [!]'); throw new Error('Hello'); } + componentWillMount() { + log.push('BrokenRender componentWillMount'); + } componentDidMount() { log.push('BrokenRender componentDidMount'); } @@ -784,8 +766,14 @@ describe('ReactErrorBoundaries', () => { expect(container.textContent).toBe('Caught an error: Hello.'); ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render', + 'Normal constructor', + 'Normal componentWillMount', 'Normal render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', 'BrokenRender render [!]', 'ErrorBoundary ref to Normal is set to null', 'ErrorBoundary renderError', From b0e6a081b6ae82f4ee1f91730e4fbf013bc0f322 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 16:00:13 +0100 Subject: [PATCH 14/23] Refactor tests --- .../__tests__/ReactErrorBoundaries-test.js | 1371 ++++++----------- 1 file changed, 439 insertions(+), 932 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 54839d64a240..f49c3cc4fdd4 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -16,84 +16,25 @@ var ReactDOM; describe('ReactErrorBoundaries', () => { + var log; + + var BrokenConstructor; + var BrokenComponentWillMount; + var BrokenComponentDidMount; + var BrokenComponentWillUnmount; + var BrokenErrorBoundary; + var BrokenRender; + var ErrorBoundary; + var NoopErrorBoundary; + var Normal; + beforeEach(() => { ReactDOM = require('ReactDOM'); React = require('React'); - }); - - it('renders an error state if child throws in render', () => { - var log = []; - class BrokenRender extends React.Component { - constructor(props) { - super(props); - log.push('BrokenRender constructor'); - } - render() { - log.push('BrokenRender render [!]'); - throw new Error('Hello'); - } - componentWillMount() { - log.push('BrokenRender componentWillMount'); - } - componentDidMount() { - log.push('BrokenRender componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenRender componentWillUnmount'); - } - } - - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - log.push('ErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('ErrorBoundary render error'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ErrorBoundary render success'); - return ; - } - componentWillMount() { - log.push('ErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); - } - } - var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.firstChild.textContent).toBe( - 'Caught an error: Hello.' - ); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenRender constructor', - 'BrokenRender componentWillMount', - 'BrokenRender render [!]', - // Catch and render an error message - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', - ]); - }); + log = []; - it('renders an error state if child throws in constructor', () => { - var log = []; - class BrokenConstructor extends React.Component { + BrokenConstructor = class BrokenConstructor extends React.Component { constructor(props) { super(props); log.push('BrokenConstructor constructor [!]'); @@ -101,6 +42,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('BrokenConstructor render'); + return
; } componentWillMount() { log.push('BrokenConstructor componentWillMount'); @@ -111,55 +53,9 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('BrokenConstructor componentWillUnmount'); } - } - - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - log.push('ErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('ErrorBoundary render error'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ErrorBoundary render success'); - return ; - } - componentWillMount() { - log.push('ErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); - } - } + }; - var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenConstructor constructor [!]', - // Catch and render an error message - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', - ]); - }); - - it('renders an error state if child throws in componentWillMount', () => { - var log = []; - class BrokenComponentWillMount extends React.Component { + BrokenComponentWillMount = class BrokenComponentWillMount extends React.Component { constructor(props) { super(props); log.push('BrokenComponentWillMount constructor'); @@ -170,6 +66,7 @@ describe('ReactErrorBoundaries', () => { } render() { log.push('BrokenComponentWillMount render'); + return
; } componentDidMount() { log.push('BrokenComponentWillMount componentDidMount'); @@ -177,58 +74,30 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('BrokenComponentWillMount componentWillUnmount'); } - } + }; - class ErrorBoundary extends React.Component { + BrokenComponentWillUnmount = class BrokenComponentWillUnmount extends React.Component { constructor(props) { super(props); - this.state = {error: null}; - log.push('ErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('ErrorBoundary render error'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ErrorBoundary render success'); - return ; + log.push('BrokenComponentWillUnmount constructor'); } componentWillMount() { - log.push('ErrorBoundary componentWillMount'); + log.push('BrokenComponentWillUnmount componentWillMount'); + } + render() { + log.push('BrokenComponentWillUnmount render'); + return
; } componentDidMount() { - log.push('ErrorBoundary componentDidMount'); + log.push('BrokenComponentWillUnmount componentDidMount'); } componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); + log.push('BrokenComponentWillUnmount componentWillUnmount [!]'); + throw new Error('Hello'); } - } - - var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenComponentWillMount constructor', - 'BrokenComponentWillMount componentWillMount [!]', - // Catch and render an error message - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', - ]); - }); + }; - // Known limitation because componentDidMount() does not occur on the stack. - // We could either hardcode searching for parent boundary, or wait for Fiber. - it('currently does not catch errors in componentDidMount', () => { - var log = []; - class BrokenComponentDidMount extends React.Component { + BrokenComponentDidMount = class BrokenComponentDidMount extends React.Component { constructor(props) { super(props); log.push('BrokenComponentDidMount constructor'); @@ -247,60 +116,38 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('BrokenComponentDidMount componentWillUnmount'); } - } + }; - class ErrorBoundary extends React.Component { + BrokenErrorBoundary = class BrokenErrorBoundary extends React.Component { constructor(props) { super(props); - this.state = {error: false}; - log.push('ErrorBoundary constructor'); + this.state = {error: null}; + log.push('BrokenErrorBoundary constructor'); } render() { if (this.state.error) { - log.push('ErrorBoundary render error'); - return
Caught an error.
; + log.push('BrokenErrorBoundary render error [!]'); + throw new Error('Hello'); } - log.push('ErrorBoundary render success'); - return ; + log.push('BrokenErrorBoundary render success'); + return
{this.props.children}
; } componentWillMount() { - log.push('ErrorBoundary componentWillMount'); + log.push('BrokenErrorBoundary componentWillMount'); } componentDidMount() { - log.push('ErrorBoundary componentDidMount'); + log.push('BrokenErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); + log.push('BrokenErrorBoundary componentWillUnmount'); } - unstable_handleError() { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error: true}); + unstable_handleError(error) { + log.push('BrokenErrorBoundary unstable_handleError'); + this.setState({error}); } - } - - var container = document.createElement('div'); - expect(() => { - ReactDOM.render(, container); - }).toThrow(); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenComponentDidMount constructor', - 'BrokenComponentDidMount componentWillMount', - 'BrokenComponentDidMount render', - 'BrokenComponentDidMount componentDidMount [!]', - // FIXME: uncomment when componentDidMount() gets caught. - // Catch and render an error message - // 'ErrorBoundary unstable_handleError', - // 'ErrorBoundary render error', - // 'ErrorBoundary componentDidMount', - ]); - }); + }; - it('propagates errors on retry on mounting', () => { - var log = []; - class BrokenRender extends React.Component { + BrokenRender = class BrokenRender extends React.Component { constructor(props) { super(props); log.push('BrokenRender constructor'); @@ -318,175 +165,237 @@ describe('ReactErrorBoundaries', () => { componentWillUnmount() { log.push('BrokenRender componentWillUnmount'); } - } + }; - class UncatchingErrorBoundary extends React.Component { + NoopErrorBoundary = class NoopErrorBoundary extends React.Component { constructor(props) { super(props); - log.push('UncatchingErrorBoundary constructor'); + log.push('NoopErrorBoundary constructor'); } render() { - log.push('UncatchingErrorBoundary render'); + log.push('NoopErrorBoundary render'); return ; } componentWillMount() { - log.push('UncatchingErrorBoundary componentWillMount'); + log.push('NoopErrorBoundary componentWillMount'); } componentDidMount() { - log.push('UncatchingErrorBoundary componentDidMount'); + log.push('NoopErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('UncatchingErrorBoundary componentWillUnmount'); + log.push('NoopErrorBoundary componentWillUnmount'); } unstable_handleError() { - log.push('UncatchingErrorBoundary unstable_handleError'); + log.push('NoopErrorBoundary unstable_handleError'); } - } + }; - class ParentErrorBoundary extends React.Component { + Normal = class Normal extends React.Component { + static defaultProps = { + logName: 'Normal', + }; constructor(props) { super(props); - this.state = {error: null}; - log.push('ParentErrorBoundary constructor'); + log.push(`${this.props.logName} constructor`); } render() { - if (this.state.error) { - log.push('ParentErrorBoundary render error'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ParentErrorBoundary render success'); - return ; + log.push(`${this.props.logName} render`); + return
{this.props.children}
; } componentWillMount() { - log.push('ParentErrorBoundary componentWillMount'); + log.push(`${this.props.logName} componentWillMount`); } componentDidMount() { - log.push('ParentErrorBoundary componentDidMount'); + log.push(`${this.props.logName} componentDidMount`); } componentWillUnmount() { - log.push('ParentErrorBoundary componentWillUnmount'); + log.push(`${this.props.logName} componentWillUnmount`); + } + }; + + ErrorBoundary = class ErrorBoundary extends React.Component { + constructor() { + super(); + this.state = {error: null}; + log.push('ErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('ErrorBoundary render error'); + return this.props.renderError(this.state.error); + } + log.push('ErrorBoundary render success'); + return
{this.props.children}
; } unstable_handleError(error) { - log.push('ParentErrorBoundary unstable_handleError'); + log.push('ErrorBoundary unstable_handleError'); this.setState({error}); } - } + componentWillMount() { + log.push('ErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('ErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorBoundary componentWillUnmount'); + } + }; + + ErrorBoundary.defaultProps = { + renderError(error) { + return
Caught an error: {error.message}.
; + }, + }; + }); + it('renders an error state if child throws in render', () => { var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + ReactDOM.render( + + + , + container + ); + expect(container.firstChild.textContent).toBe( + 'Caught an error: Hello.' + ); expect(log).toEqual([ - 'ParentErrorBoundary constructor', - 'ParentErrorBoundary componentWillMount', - 'ParentErrorBoundary render success', - 'UncatchingErrorBoundary constructor', - 'UncatchingErrorBoundary componentWillMount', - 'UncatchingErrorBoundary render', - 'BrokenRender constructor', - 'BrokenRender componentWillMount', - 'BrokenRender render [!]', - // The first error boundary catches the error - // However, it doesn't adjust its state so next render also fails - 'UncatchingErrorBoundary unstable_handleError', - 'UncatchingErrorBoundary render', + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // This time, the error propagates to the higher boundary - 'ParentErrorBoundary unstable_handleError', - // Render the error - 'ParentErrorBoundary render error', - 'ParentErrorBoundary componentDidMount', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', ]); }); - it('propagates errors inside boundary itself on mounting', () => { - var log = []; - class BrokenRender extends React.Component { - constructor(props) { - super(props); - log.push('BrokenRender constructor'); - } - render() { - log.push('BrokenRender render [!]'); - throw new Error('Hello'); - } - componentWillMount() { - log.push('BrokenRender componentWillMount'); - } - componentDidMount() { - log.push('BrokenRender componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenRender componentWillUnmount'); - } - } + it('renders an error state if child throws in constructor', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenConstructor constructor [!]', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + }); - class BrokenErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: false}; - log.push('BrokenErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('BrokenErrorBoundary render error [!]'); - throw new Error('Hello'); - } - log.push('BrokenErrorBoundary render success'); - return ; - } - componentWillMount() { - log.push('BrokenErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('BrokenErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenErrorBoundary componentWillUnmount'); - } - unstable_handleError() { - log.push('BrokenErrorBoundary unstable_handleError'); - this.setState({error: true}); - } - } + it('renders an error state if child throws in componentWillMount', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentWillMount constructor', + 'BrokenComponentWillMount componentWillMount [!]', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + }); - class ParentErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: false}; - log.push('ParentErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('ParentErrorBoundary render error'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ParentErrorBoundary render success'); - return ; - } - componentWillMount() { - log.push('ParentErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('ParentErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ParentErrorBoundary componentWillUnmount'); - } - unstable_handleError(error) { - log.push('ParentErrorBoundary unstable_handleError'); - this.setState({error}); - } - } + // Known limitation because componentDidMount() does not occur on the stack. + // We could either hardcode searching for parent boundary, or wait for Fiber. + it('currently does not catch errors in componentDidMount', () => { + var container = document.createElement('div'); + expect(() => { + ReactDOM.render( + + + , + container + ); + }).toThrow(); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentDidMount constructor', + 'BrokenComponentDidMount componentWillMount', + 'BrokenComponentDidMount render', + 'BrokenComponentDidMount componentDidMount [!]', + // FIXME: uncomment when componentDidMount() gets caught. + // Catch and render an error message + // 'ErrorBoundary unstable_handleError', + // 'ErrorBoundary render error', + // 'ErrorBoundary componentDidMount', + ]); + }); + it('propagates errors on retry on mounting', () => { var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render( + + + + + , + container + ); expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ - 'ParentErrorBoundary constructor', - 'ParentErrorBoundary componentWillMount', - 'ParentErrorBoundary render success', + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'NoopErrorBoundary constructor', + 'NoopErrorBoundary componentWillMount', + 'NoopErrorBoundary render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // The first error boundary catches the error + // However, it doesn't adjust its state so next render also fails + 'NoopErrorBoundary unstable_handleError', + 'NoopErrorBoundary render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // This time, the error propagates to the higher boundary + 'ErrorBoundary unstable_handleError', + // Render the error + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + }); + + it('propagates errors inside boundary itself on mounting', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + + + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', 'BrokenErrorBoundary constructor', 'BrokenErrorBoundary componentWillMount', 'BrokenErrorBoundary render success', @@ -498,119 +407,28 @@ describe('ReactErrorBoundaries', () => { 'BrokenErrorBoundary unstable_handleError', 'BrokenErrorBoundary render error [!]', // The error propagates to the higher boundary - 'ParentErrorBoundary unstable_handleError', + 'ErrorBoundary unstable_handleError', // Render the error - 'ParentErrorBoundary render error', - 'ParentErrorBoundary componentDidMount', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', ]); }); it('does not register event handlers for unmounted children', () => { - class BrokenRender extends React.Component { - render() { - throw new Error('Hello'); - } - } - - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: false}; - } - render() { - if (this.state.error) { - return
Caught an error.
; - } - return ( -
- - -
- ); - } - handleClick() { - /* do nothing */ - } - unstable_handleError() { - this.setState({error: true}); - } - } - var EventPluginHub = require('EventPluginHub'); var container = document.createElement('div'); EventPluginHub.putListener = jest.fn(); - ReactDOM.render(, container); + ReactDOM.render( + + + + , + container + ); expect(EventPluginHub.putListener).not.toBeCalled(); }); it('does not call componentWillUnmount when aborting initial mount', () => { - var log = []; - class ErrorBoundary extends React.Component { - constructor() { - super(); - this.state = {error: null}; - log.push('ErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('ErrorBoundary render error'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ErrorBoundary render success'); - return
{this.props.children}
; - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); - } - componentWillMount() { - log.push('ErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - } - - class BrokenRender extends React.Component { - constructor(props) { - super(props); - log.push('BrokenRender constructor'); - } - render() { - log.push('BrokenRender render [!]'); - throw new Error('Hello'); - } - componentWillMount() { - log.push('BrokenRender componentWillMount'); - } - componentDidMount() { - log.push('BrokenRender componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenRender componentWillUnmount'); - } - } - - class Normal extends React.Component { - constructor(props) { - super(props); - log.push('Normal constructor'); - } - render() { - log.push('Normal render'); - return
; - } - componentWillMount() { - log.push('Normal componentWillMount'); - } - componentDidMount() { - log.push('Normal componentDidMount'); - } - componentWillUnmount() { - log.push('Normal componentWillUnmount'); - } - } - var container = document.createElement('div'); ReactDOM.render( @@ -639,448 +457,208 @@ describe('ReactErrorBoundaries', () => { // Render the error message 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', + 'ErrorBoundary componentWillUnmount' ]); }); it('successfully mounts if no error occurs', () => { - var log = []; - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: false}; - log.push('ErrorBoundary constructor'); - } - render() { - log.push('ErrorBoundary render'); - if (this.state.error) { - return
Caught an error.
; - } - return
Mounted successfully.
; - } - componentWillMount() { - log.push('ErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - unstable_handleError() { - this.setState({error: true}); - } - } - var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.firstChild.textContent).toBe('Mounted successfully.'); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render', - 'ErrorBoundary componentDidMount', - ]); - }); - - it('resets refs to composite siblings if one of them throws', () => { - var log = []; - - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - log.push('ErrorBoundary constructor'); - } - render() { - if (this.state.error) { - log.push('ErrorBoundary renderError'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ErrorBoundary render'); - var ref = function(x) { - log.push('ErrorBoundary ref to Normal is set to ' + x); - }; - return ( -
- - -
- ); - } - unstable_handleError(error) { - this.setState({error}); - } - componentWillMount() { - log.push('ErrorBoundary componentWillMount'); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - } - - class Normal extends React.Component { - constructor(props) { - super(props); - log.push('Normal constructor'); - } - render() { - log.push('Normal render'); - return
What is love?
; - } - componentWillMount() { - log.push('Normal componentWillMount'); - } - componentDidMount() { - log.push('Normal componentDidMount'); - } - componentWillUnmount() { - log.push('Normal componentWillUnmount'); - } - } - - class BrokenRender extends React.Component { - constructor(props) { - super(props); - log.push('BrokenRender constructor'); - } - render() { - log.push('BrokenRender render [!]'); - throw new Error('Hello'); - } - componentWillMount() { - log.push('BrokenRender componentWillMount'); - } - componentDidMount() { - log.push('BrokenRender componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenRender componentWillUnmount'); - } - } - - var container = document.createElement('div'); - ReactDOM.render(, container); - expect(container.textContent).toBe('Caught an error: Hello.'); - ReactDOM.unmountComponentAtNode(container); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render', - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', - 'BrokenRender constructor', - 'BrokenRender componentWillMount', - 'BrokenRender render [!]', - 'ErrorBoundary ref to Normal is set to null', - 'ErrorBoundary renderError', - 'ErrorBoundary componentDidMount', - 'ErrorBoundary componentWillUnmount', - ]); - }); - - it('catches if child throws in render during update', () => { - var log = []; - - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - log.push('ErrorBoundary renderError'); - return ( - { - log.push('ErrorBoundary ref to ErrorMessage is set to ' + inst); - }} /> - ); - } - log.push('ErrorBoundary render'); - return ( -
- { - log.push('ErrorBoundary ref to Normal is set to ' + inst); - }} /> - {this.props.renderBrokenChild ? :
} -
- ); - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - } - - class ErrorMessage extends React.Component { - componentWillMount() { - log.push('ErrorMessage componentWillMount'); - } - componentDidMount() { - log.push('ErrorMessage componentDidMount'); - } - render() { - log.push('ErrorMessage render'); - return
Caught an error: {this.props.message}.
; - } - } - - class Normal extends React.Component { - render() { - log.push('Normal render'); - return
What is love?
; - } - componentDidMount() { - log.push('Normal componentDidMount'); - } - componentWillUnmount() { - log.push('Normal componentWillUnmount'); - } - } - - class BrokenRender extends React.Component { - render() { - log.push('BrokenRender render [!]'); - throw new Error('Hello'); - } - componentDidMount() { - log.push('BrokenRender componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenRender componentWillUnmount'); - } - } - - var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render( + +
Mounted successfully.
+
, + container + ); + expect(container.firstChild.textContent).toBe('Mounted successfully.'); expect(log).toEqual([ - 'ErrorBoundary render', - 'Normal render', - 'Normal componentDidMount', - 'ErrorBoundary ref to Normal is set to [object Object]', + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', 'ErrorBoundary componentDidMount', ]); + }); - log.length = 0; - ReactDOM.render(, container); + it('resets refs to composite siblings if one of them throws', () => { + function ref(x) { + log.push('Normal ref is set to ' + x); + }; + + var container = document.createElement('div'); + ReactDOM.render( + + + + , + container + ); expect(container.textContent).toBe('Caught an error: Hello.'); + ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ - 'ErrorBoundary render', - 'ErrorBoundary ref to Normal is set to null', + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'Normal constructor', + 'Normal componentWillMount', 'Normal render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', 'BrokenRender render [!]', 'ErrorBoundary unstable_handleError', - 'ErrorBoundary ref to Normal is set to null', - 'Normal componentWillUnmount', - 'ErrorBoundary renderError', - 'ErrorMessage componentWillMount', - 'ErrorMessage render', - 'ErrorMessage componentDidMount', - 'ErrorBoundary ref to ErrorMessage is set to [object Object]', + 'Normal ref is set to null', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + 'ErrorBoundary componentWillUnmount', ]); }); - it('skips hook for not-yet-mounted child if sibling throws in update', () => { - var log = []; - - class ErrorBoundary extends React.Component { + it('catches if child throws in render during update', () => { + class ErrorMessage extends React.Component { constructor(props) { super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - log.push('ErrorBoundary renderError'); - return ( - { - log.push('ErrorBoundary ref to ErrorMessage is set to ' + inst); - }} /> - ); - } - log.push('ErrorBoundary render'); - return ( -
- { - log.push('ErrorBoundary ref to Normal is set to ' + inst); - }} /> - {this.props.renderBrokenChild && } - {this.props.renderBrokenChild ? :
} -
- ); - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); + log.push('ErrorMessage constructor'); } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - } - - class ErrorMessage extends React.Component { componentWillMount() { log.push('ErrorMessage componentWillMount'); } componentDidMount() { log.push('ErrorMessage componentDidMount'); } - render() { - log.push('ErrorMessage render'); - return
Caught an error: {this.props.message}.
; - } - } - - class Normal extends React.Component { - render() { - log.push('Normal render'); - return
What is love?
; - } - componentDidMount() { - log.push('Normal componentDidMount'); - } componentWillUnmount() { - log.push('Normal componentWillUnmount'); + log.push('ErrorMessage componentWillUnmount'); } - } - - class Normal2 extends React.Component { render() { - log.push('Normal2 render'); - return
What is love?
; - } - componentDidMount() { - log.push('Normal2 componentDidMount'); - } - componentWillUnmount() { - log.push('Normal2 componentWillUnmount'); + log.push('ErrorMessage render'); + return
Caught an error: {this.props.message}.
; } } - class BrokenRender extends React.Component { - render() { - log.push('BrokenRender render [!]'); - throw new Error('Hello'); - } - componentDidMount() { - log.push('BrokenRender componentDidMount'); - } - componentWillUnmount() { - log.push('BrokenRender componentWillUnmount'); - } + function renderError(error) { + return ( + { + log.push('ErrorMessage ref is set to ' + inst); + }} /> + ); } var container = document.createElement('div'); - ReactDOM.render(, container); + ReactDOM.render( + + { + log.push('Normal ref is set to ' + inst); + }} /> + , + container + ); expect(log).toEqual([ - 'ErrorBoundary render', + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'Normal constructor', + 'Normal componentWillMount', 'Normal render', 'Normal componentDidMount', - 'ErrorBoundary ref to Normal is set to [object Object]', + 'Normal ref is set to [object Object]', 'ErrorBoundary componentDidMount', ]); log.length = 0; - ReactDOM.render(, container); + ReactDOM.render( + + { + log.push('Normal ref is set to ' + inst); + }} /> + { + log.push('Normal ref is set to ' + inst); + }} /> + + , + container + ); expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ - 'ErrorBoundary render', - 'ErrorBoundary ref to Normal is set to null', + 'ErrorBoundary render success', + 'Normal ref is set to null', 'Normal render', + // Normal2 will attempt to mount: + 'Normal2 constructor', + 'Normal2 componentWillMount', 'Normal2 render', + // BrokenRender will abort rendering: + 'BrokenRender constructor', + 'BrokenRender componentWillMount', 'BrokenRender render [!]', 'ErrorBoundary unstable_handleError', - 'ErrorBoundary ref to Normal is set to null', + // Unmount the previously mounted components: + 'Normal ref is set to null', 'Normal componentWillUnmount', - // Normal2 doesn't get componentWillUnmount() since it never fully mounted - 'ErrorBoundary renderError', + // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary render error', + 'ErrorMessage constructor', 'ErrorMessage componentWillMount', 'ErrorMessage render', 'ErrorMessage componentDidMount', - 'ErrorBoundary ref to ErrorMessage is set to [object Object]', + 'ErrorMessage ref is set to [object Object]', ]); }); it('recovers from componentWillUnmount errors on update', () => { - var log = []; - - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - log.push('ErrorBoundary renderError'); - return
Caught an error: {this.state.error.message}.
; - } - log.push('ErrorBoundary render'); - return ( -
- - - {this.props.renderChildWithBrokenUnmount && - - } -
- ); - } - unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); - this.setState({error}); - } - componentDidMount() { - log.push('ErrorBoundary componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); - } - } - - class BrokenUnmount extends React.Component { - render() { - return
text
; - } - componentWillUnmount() { - log.push('BrokenUnmount componentWillUnmount [!]'); - throw new Error('Hello'); - } - } - var container = document.createElement('div'); ReactDOM.render( - , + + + + + , container ); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + // Mount three children: + 'BrokenComponentWillUnmount constructor', + 'BrokenComponentWillUnmount componentWillMount', + 'BrokenComponentWillUnmount render', + 'BrokenComponentWillUnmount constructor', + 'BrokenComponentWillUnmount componentWillMount', + 'BrokenComponentWillUnmount render', + 'BrokenComponentWillUnmount constructor', + 'BrokenComponentWillUnmount componentWillMount', + 'BrokenComponentWillUnmount render', + // Finalize mounting + 'BrokenComponentWillUnmount componentDidMount', + 'BrokenComponentWillUnmount componentDidMount', + 'BrokenComponentWillUnmount componentDidMount', + 'ErrorBoundary componentDidMount' + ]); + + log.length = 0; ReactDOM.render( - , + + + + , container ); expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ - 'ErrorBoundary render', - 'ErrorBoundary componentDidMount', - 'ErrorBoundary render', - 'BrokenUnmount componentWillUnmount [!]', + 'ErrorBoundary render success', + // Update existing children: + 'BrokenComponentWillUnmount render', + 'BrokenComponentWillUnmount render', + // Unmounting throws: + 'BrokenComponentWillUnmount componentWillUnmount [!]', 'ErrorBoundary unstable_handleError', - 'BrokenUnmount componentWillUnmount [!]', - 'BrokenUnmount componentWillUnmount [!]', - 'ErrorBoundary renderError', + // Attempt to unmount previous children: + 'BrokenComponentWillUnmount componentWillUnmount [!]', + 'BrokenComponentWillUnmount componentWillUnmount [!]', + 'ErrorBoundary render error', ]); log.length = 0; @@ -1091,96 +669,80 @@ describe('ReactErrorBoundaries', () => { }); it('recovers from nested componentWillUnmount errors on update', () => { - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - return
Caught an error: {this.state.error.message}.
; - } - return ( -
- - {this.props.renderChildWithBrokenUnmount && - - } -
- ); - } - unstable_handleError(error) { - this.setState({error}); - } - } - - class RenderBrokenUnmount extends React.Component { - render() { - return ; - } - } - - class BrokenUnmount extends React.Component { - render() { - return
text
; - } - componentWillUnmount() { - throw new Error('Hello'); - } + function ParentOfBrokenComponentWillUnmount() { + return ; } var container = document.createElement('div'); ReactDOM.render( - , + + + + + + , container ); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + // Mount first child: + 'Normal constructor', + 'Normal componentWillMount', + 'Normal render', + 'BrokenComponentWillUnmount constructor', + 'BrokenComponentWillUnmount componentWillMount', + 'BrokenComponentWillUnmount render', + // Mount second child: + 'BrokenComponentWillUnmount constructor', + 'BrokenComponentWillUnmount componentWillMount', + 'BrokenComponentWillUnmount render', + // Finalize mounting + 'BrokenComponentWillUnmount componentDidMount', + 'Normal componentDidMount', + 'BrokenComponentWillUnmount componentDidMount', + 'ErrorBoundary componentDidMount' + ]); + + log.length = 0; ReactDOM.render( - , + + + + + , container ); expect(container.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary render success', + // Update existing children: + 'Normal render', + 'BrokenComponentWillUnmount render', + // Unmounting throws: + 'BrokenComponentWillUnmount componentWillUnmount [!]', + 'ErrorBoundary unstable_handleError', + // Attempt to unmount previous children: + 'Normal componentWillUnmount', + 'BrokenComponentWillUnmount componentWillUnmount [!]', + // Render error: + 'ErrorBoundary render error', + ]); + + log.length = 0; ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('doesn\'t get into inconsistent state during removals', () => { - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - return
Caught an error: {this.state.error.message}.
; - } - return ( -
{this.props.children}
- ); - } - unstable_handleError(error) { - this.setState({error}); - } - } - - class Normal extends React.Component { - render() { - return
text
; - } - } - - class BrokenUnmount extends React.Component { - render() { - return
text
; - } - componentWillUnmount() { - throw new Error('Hello'); - } - } - var container = document.createElement('div'); ReactDOM.render( - + , container @@ -1191,36 +753,6 @@ describe('ReactErrorBoundaries', () => { }); it('doesn\'t get into inconsistent state during additions', () => { - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - return
Caught an error: {this.state.error.message}.
; - } - return ( -
{this.props.children}
- ); - } - unstable_handleError(error) { - this.setState({error}); - } - } - - class Normal extends React.Component { - render() { - return
text
; - } - } - - class BrokenRender extends React.Component { - render() { - throw new Error('Hello'); - } - } - var container = document.createElement('div'); ReactDOM.render(, container); ReactDOM.render( @@ -1241,7 +773,7 @@ describe('ReactErrorBoundaries', () => { for (var i = 0; i < 100; i++) { elements.push(); } - elements.push(); + elements.push(); var currentIndex = elements.length; while (0 !== currentIndex) { @@ -1254,37 +786,12 @@ describe('ReactErrorBoundaries', () => { return elements; } - class ErrorBoundary extends React.Component { - constructor(props) { - super(props); - this.state = {error: null}; - } - render() { - if (this.state.error) { - return
Caught an error: {this.state.error.message}.
; - } - - return ( -
{this.props.children}
- ); - } - unstable_handleError(error) { - this.setState({error}); - } - } - - class Normal extends React.Component { - render() { - return
text
; - } - } - - class BrokenRender extends React.Component { + class MaybeBrokenRender extends React.Component { render() { if (fail) { throw new Error('Hello'); } - return
text
; + return
; } } @@ -1296,7 +803,7 @@ describe('ReactErrorBoundaries', () => { , container ); - expect(container.textContent).not.toBe('Caught an error.'); + expect(container.textContent).not.toContain('Caught an error'); fail = true; ReactDOM.render( From 842bd75669caf700f3fd555f19f912fdbae7c1c1 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 17:23:45 +0100 Subject: [PATCH 15/23] Split complicated tests into smaller ones --- .../__tests__/ReactErrorBoundaries-test.js | 258 ++++++++++++------ 1 file changed, 177 insertions(+), 81 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index f49c3cc4fdd4..b620cf260fef 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -15,7 +15,6 @@ var React; var ReactDOM; describe('ReactErrorBoundaries', () => { - var log; var BrokenConstructor; @@ -25,6 +24,7 @@ describe('ReactErrorBoundaries', () => { var BrokenErrorBoundary; var BrokenRender; var ErrorBoundary; + var ErrorMessage; var NoopErrorBoundary; var Normal; @@ -222,7 +222,7 @@ describe('ReactErrorBoundaries', () => { render() { if (this.state.error) { log.push('ErrorBoundary render error'); - return this.props.renderError(this.state.error); + return this.props.renderError(this.state.error, this.props); } log.push('ErrorBoundary render success'); return
{this.props.children}
; @@ -241,12 +241,35 @@ describe('ReactErrorBoundaries', () => { log.push('ErrorBoundary componentWillUnmount'); } }; - ErrorBoundary.defaultProps = { - renderError(error) { - return
Caught an error: {error.message}.
; + renderError(error, props) { + return ( +
+ Caught an error: {error.message}. +
+ ); }, }; + + ErrorMessage = class ErrorMessage extends React.Component { + constructor(props) { + super(props); + log.push('ErrorMessage constructor'); + } + componentWillMount() { + log.push('ErrorMessage componentWillMount'); + } + componentDidMount() { + log.push('ErrorMessage componentDidMount'); + } + componentWillUnmount() { + log.push('ErrorMessage componentWillUnmount'); + } + render() { + log.push('ErrorMessage render'); + return
Caught an error: {this.props.message}.
; + } + }; }); it('renders an error state if child throws in render', () => { @@ -317,6 +340,39 @@ describe('ReactErrorBoundaries', () => { ]); }); + it('mounts the error message if mounting fails', () => { + function renderError(error) { + return ( + + ); + } + + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // Handle the error: + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + // Mount the error message: + 'ErrorMessage constructor', + 'ErrorMessage componentWillMount', + 'ErrorMessage render', + 'ErrorMessage componentDidMount', + 'ErrorBoundary componentDidMount', + ]); + }); + // Known limitation because componentDidMount() does not occur on the stack. // We could either hardcode searching for parent boundary, or wait for Fiber. it('currently does not catch errors in componentDidMount', () => { @@ -461,94 +517,70 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('successfully mounts if no error occurs', () => { + it('resets refs if mounting aborts', () => { + function childRef(x) { + log.push('Child ref is set to ' + x); + }; + function errorMessageRef(x) { + log.push('Error message ref is set to ' + x); + }; + var container = document.createElement('div'); ReactDOM.render( - -
Mounted successfully.
+ +
+ , container ); - expect(container.firstChild.textContent).toBe('Mounted successfully.'); + expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // Handle error: + 'ErrorBoundary unstable_handleError', + // Ref to child should not get set: + 'Child ref is set to null', + 'ErrorBoundary render error', + // Ref to error message should get set: + 'Error message ref is set to [object HTMLDivElement]', 'ErrorBoundary componentDidMount', ]); - }); - it('resets refs to composite siblings if one of them throws', () => { - function ref(x) { - log.push('Normal ref is set to ' + x); - }; + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + 'Error message ref is set to null', + ]); + }); + it('successfully mounts if no error occurs', () => { var container = document.createElement('div'); ReactDOM.render( - - +
Mounted successfully.
, container ); - expect(container.textContent).toBe('Caught an error: Hello.'); - ReactDOM.unmountComponentAtNode(container); + expect(container.firstChild.textContent).toBe('Mounted successfully.'); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', - 'BrokenRender constructor', - 'BrokenRender componentWillMount', - 'BrokenRender render [!]', - 'ErrorBoundary unstable_handleError', - 'Normal ref is set to null', - 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', - 'ErrorBoundary componentWillUnmount', ]); }); it('catches if child throws in render during update', () => { - class ErrorMessage extends React.Component { - constructor(props) { - super(props); - log.push('ErrorMessage constructor'); - } - componentWillMount() { - log.push('ErrorMessage componentWillMount'); - } - componentDidMount() { - log.push('ErrorMessage componentDidMount'); - } - componentWillUnmount() { - log.push('ErrorMessage componentWillUnmount'); - } - render() { - log.push('ErrorMessage render'); - return
Caught an error: {this.props.message}.
; - } - } - - function renderError(error) { - return ( - { - log.push('ErrorMessage ref is set to ' + inst); - }} /> - ); - } - var container = document.createElement('div'); ReactDOM.render( - - { - log.push('Normal ref is set to ' + inst); - }} /> + + , container ); @@ -560,22 +592,14 @@ describe('ReactErrorBoundaries', () => { 'Normal componentWillMount', 'Normal render', 'Normal componentDidMount', - 'Normal ref is set to [object Object]', 'ErrorBoundary componentDidMount', ]); log.length = 0; ReactDOM.render( - - { - log.push('Normal ref is set to ' + inst); - }} /> - { - log.push('Normal ref is set to ' + inst); - }} /> + + + , container @@ -583,7 +607,6 @@ describe('ReactErrorBoundaries', () => { expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary render success', - 'Normal ref is set to null', 'Normal render', // Normal2 will attempt to mount: 'Normal2 constructor', @@ -595,15 +618,67 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', 'ErrorBoundary unstable_handleError', // Unmount the previously mounted components: - 'Normal ref is set to null', 'Normal componentWillUnmount', // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary render error' + ]); + }); + + it('keeps refs up-to-date during updates', () => { + function child1Ref(x) { + log.push('Child1 ref is set to ' + x); + }; + function child2Ref(x) { + log.push('Child2 ref is set to ' + x); + }; + function errorMessageRef(x) { + log.push('Error message ref is set to ' + x); + }; + + var container = document.createElement('div'); + ReactDOM.render( + +
+ , + container + ); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'Child1 ref is set to [object HTMLDivElement]', + 'ErrorBoundary componentDidMount', + ]); + + log.length = 0; + ReactDOM.render( + +
+
+ + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary render success', + // BrokenRender will abort rendering: + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + 'ErrorBoundary unstable_handleError', + // Unmount the previously mounted components: + 'Child1 ref is set to null', 'ErrorBoundary render error', - 'ErrorMessage constructor', - 'ErrorMessage componentWillMount', - 'ErrorMessage render', - 'ErrorMessage componentDidMount', - 'ErrorMessage ref is set to [object Object]', + 'Error message ref is set to [object HTMLDivElement]', + // Child2 ref is never set because its mounting aborted + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + 'Error message ref is set to null', ]); }); @@ -737,6 +812,27 @@ describe('ReactErrorBoundaries', () => { ]); }); + it('can update multiple times in error state', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); + ReactDOM.render( + + + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); + ReactDOM.render(
Other screen
, container); + expect(container.textContent).toBe('Other screen'); + ReactDOM.unmountComponentAtNode(container); + }); + it('doesn\'t get into inconsistent state during removals', () => { var container = document.createElement('div'); ReactDOM.render( From 0783dee8810381dfa62f99cfabf7f6f93d5f5c40 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 17:30:14 +0100 Subject: [PATCH 16/23] Assert clean unmounting --- .../__tests__/ReactErrorBoundaries-test.js | 91 ++++++++++++++++++- 1 file changed, 88 insertions(+), 3 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index b620cf260fef..5c86153bdd7c 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -295,6 +295,12 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('renders an error state if child throws in constructor', () => { @@ -316,6 +322,12 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('renders an error state if child throws in componentWillMount', () => { @@ -338,6 +350,12 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('mounts the error message if mounting fails', () => { @@ -371,6 +389,13 @@ describe('ReactErrorBoundaries', () => { 'ErrorMessage componentDidMount', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + 'ErrorMessage componentWillUnmount', + ]); }); // Known limitation because componentDidMount() does not occur on the stack. @@ -399,6 +424,13 @@ describe('ReactErrorBoundaries', () => { // 'ErrorBoundary render error', // 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + 'BrokenComponentDidMount componentWillUnmount', + ]); }); it('propagates errors on retry on mounting', () => { @@ -435,6 +467,12 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('propagates errors inside boundary itself on mounting', () => { @@ -468,6 +506,12 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('does not register event handlers for unmounted children', () => { @@ -482,6 +526,12 @@ describe('ReactErrorBoundaries', () => { container ); expect(EventPluginHub.putListener).not.toBeCalled(); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('does not call componentWillUnmount when aborting initial mount', () => { @@ -495,7 +545,6 @@ describe('ReactErrorBoundaries', () => { container ); expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); - ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', @@ -512,8 +561,13 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary unstable_handleError', // Render the error message 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', - 'ErrorBoundary componentWillUnmount' + 'ErrorBoundary componentDidMount' + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', ]); }); @@ -574,6 +628,12 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render success', 'ErrorBoundary componentDidMount', ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('catches if child throws in render during update', () => { @@ -622,6 +682,12 @@ describe('ReactErrorBoundaries', () => { // Normal2 does not get lifefycle because it was never mounted 'ErrorBoundary render error' ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('keeps refs up-to-date during updates', () => { @@ -821,6 +887,7 @@ describe('ReactErrorBoundaries', () => { container ); expect(container.textContent).toBe('Caught an error: Hello.'); + ReactDOM.render( @@ -828,8 +895,10 @@ describe('ReactErrorBoundaries', () => { container ); expect(container.textContent).toBe('Caught an error: Hello.'); + ReactDOM.render(
Other screen
, container); expect(container.textContent).toBe('Other screen'); + ReactDOM.unmountComponentAtNode(container); }); @@ -843,9 +912,15 @@ describe('ReactErrorBoundaries', () => {
, container ); + ReactDOM.render(, container); expect(container.textContent).toBe('Caught an error: Hello.'); + + log.length = 0; ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('doesn\'t get into inconsistent state during additions', () => { @@ -860,7 +935,12 @@ describe('ReactErrorBoundaries', () => { container ); expect(container.textContent).toBe('Caught an error: Hello.'); + + log.length = 0; ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); it('doesn\'t get into inconsistent state during reorders', () => { @@ -909,6 +989,11 @@ describe('ReactErrorBoundaries', () => { container ); expect(container.textContent).toBe('Caught an error: Hello.'); + + log.length = 0; ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); }); }); From b36526e8ffcd6358b44080e1128b0af7c8080369 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 17:58:04 +0100 Subject: [PATCH 17/23] Add assertions about update hooks --- .../__tests__/ReactErrorBoundaries-test.js | 111 ++++++++++++++++-- 1 file changed, 99 insertions(+), 12 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 5c86153bdd7c..a3f2931282ff 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -50,6 +50,15 @@ describe('ReactErrorBoundaries', () => { componentDidMount() { log.push('BrokenConstructor componentDidMount'); } + componentWillReceiveProps() { + log.push('BrokenConstructor componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenConstructor componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenConstructor componentDidUpdate'); + } componentWillUnmount() { log.push('BrokenConstructor componentWillUnmount'); } @@ -60,17 +69,26 @@ describe('ReactErrorBoundaries', () => { super(props); log.push('BrokenComponentWillMount constructor'); } - componentWillMount() { - log.push('BrokenComponentWillMount componentWillMount [!]'); - throw new Error('Hello'); - } render() { log.push('BrokenComponentWillMount render'); return
; } + componentWillMount() { + log.push('BrokenComponentWillMount componentWillMount [!]'); + throw new Error('Hello'); + } componentDidMount() { log.push('BrokenComponentWillMount componentDidMount'); } + componentWillReceiveProps() { + log.push('BrokenComponentWillMount componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenComponentWillMount componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenComponentWillMount componentDidUpdate'); + } componentWillUnmount() { log.push('BrokenComponentWillMount componentWillUnmount'); } @@ -81,16 +99,25 @@ describe('ReactErrorBoundaries', () => { super(props); log.push('BrokenComponentWillUnmount constructor'); } - componentWillMount() { - log.push('BrokenComponentWillUnmount componentWillMount'); - } render() { log.push('BrokenComponentWillUnmount render'); return
; } + componentWillMount() { + log.push('BrokenComponentWillUnmount componentWillMount'); + } componentDidMount() { log.push('BrokenComponentWillUnmount componentDidMount'); } + componentWillReceiveProps() { + log.push('BrokenComponentWillUnmount componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenComponentWillUnmount componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenComponentWillUnmount componentDidUpdate'); + } componentWillUnmount() { log.push('BrokenComponentWillUnmount componentWillUnmount [!]'); throw new Error('Hello'); @@ -102,17 +129,26 @@ describe('ReactErrorBoundaries', () => { super(props); log.push('BrokenComponentDidMount constructor'); } - componentWillMount() { - log.push('BrokenComponentDidMount componentWillMount'); - } render() { log.push('BrokenComponentDidMount render'); return
; } + componentWillMount() { + log.push('BrokenComponentDidMount componentWillMount'); + } componentDidMount() { log.push('BrokenComponentDidMount componentDidMount [!]'); throw new Error('Hello'); } + componentWillReceiveProps() { + log.push('BrokenComponentDidMount componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenComponentDidMount componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenComponentDidMount componentDidUpdate'); + } componentWillUnmount() { log.push('BrokenComponentDidMount componentWillUnmount'); } @@ -162,6 +198,15 @@ describe('ReactErrorBoundaries', () => { componentDidMount() { log.push('BrokenRender componentDidMount'); } + componentWillReceiveProps() { + log.push('BrokenRender componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenRender componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenRender componentDidUpdate'); + } componentWillUnmount() { log.push('BrokenRender componentWillUnmount'); } @@ -208,6 +253,15 @@ describe('ReactErrorBoundaries', () => { componentDidMount() { log.push(`${this.props.logName} componentDidMount`); } + componentWillReceiveProps() { + log.push(`${this.props.logName} componentWillReceiveProps`); + } + componentWillUpdate() { + log.push(`${this.props.logName} componentWillUpdate`); + } + componentDidUpdate() { + log.push(`${this.props.logName} componentDidUpdate`); + } componentWillUnmount() { log.push(`${this.props.logName} componentWillUnmount`); } @@ -237,6 +291,15 @@ describe('ReactErrorBoundaries', () => { componentDidMount() { log.push('ErrorBoundary componentDidMount'); } + componentWillReceiveProps() { + log.push('ErrorBoundary componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('ErrorBoundary componentWillUpdate'); + } + componentDidUpdate() { + log.push('ErrorBoundary componentDidUpdate'); + } componentWillUnmount() { log.push('ErrorBoundary componentWillUnmount'); } @@ -597,7 +660,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // Handle error: 'ErrorBoundary unstable_handleError', - // Ref to child should not get set: + // Child ref wasn't (and won't be) set but there's no harm in clearing: 'Child ref is set to null', 'ErrorBoundary render error', // Ref to error message should get set: @@ -666,7 +729,11 @@ describe('ReactErrorBoundaries', () => { ); expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render success', + 'Normal componentWillReceiveProps', + 'Normal componentWillUpdate', 'Normal render', // Normal2 will attempt to mount: 'Normal2 constructor', @@ -680,7 +747,8 @@ describe('ReactErrorBoundaries', () => { // Unmount the previously mounted components: 'Normal componentWillUnmount', // Normal2 does not get lifefycle because it was never mounted - 'ErrorBoundary render error' + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -727,6 +795,8 @@ describe('ReactErrorBoundaries', () => { ); expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render success', // BrokenRender will abort rendering: 'BrokenRender constructor', @@ -738,6 +808,7 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render error', 'Error message ref is set to [object HTMLDivElement]', // Child2 ref is never set because its mounting aborted + 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -789,9 +860,15 @@ describe('ReactErrorBoundaries', () => { ); expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render success', // Update existing children: + 'BrokenComponentWillUnmount componentWillReceiveProps', + 'BrokenComponentWillUnmount componentWillUpdate', 'BrokenComponentWillUnmount render', + 'BrokenComponentWillUnmount componentWillReceiveProps', + 'BrokenComponentWillUnmount componentWillUpdate', 'BrokenComponentWillUnmount render', // Unmounting throws: 'BrokenComponentWillUnmount componentWillUnmount [!]', @@ -799,7 +876,10 @@ describe('ReactErrorBoundaries', () => { // Attempt to unmount previous children: 'BrokenComponentWillUnmount componentWillUnmount [!]', 'BrokenComponentWillUnmount componentWillUnmount [!]', + // Render error: 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + // Children don't get componentDidUpdate() since update was aborted ]); log.length = 0; @@ -857,9 +937,15 @@ describe('ReactErrorBoundaries', () => { ); expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render success', // Update existing children: + 'Normal componentWillReceiveProps', + 'Normal componentWillUpdate', 'Normal render', + 'BrokenComponentWillUnmount componentWillReceiveProps', + 'BrokenComponentWillUnmount componentWillUpdate', 'BrokenComponentWillUnmount render', // Unmounting throws: 'BrokenComponentWillUnmount componentWillUnmount [!]', @@ -869,6 +955,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillUnmount componentWillUnmount [!]', // Render error: 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ]); log.length = 0; From db4c3069a0ebcd297d7f455cf62dd9e13edc5ac9 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 18:19:09 +0100 Subject: [PATCH 18/23] Add more tests to document existing behavior and remove irrelevant details --- .../__tests__/ReactErrorBoundaries-test.js | 448 +++++++++++++++--- 1 file changed, 382 insertions(+), 66 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index a3f2931282ff..e13d5e42bea9 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -20,6 +20,9 @@ describe('ReactErrorBoundaries', () => { var BrokenConstructor; var BrokenComponentWillMount; var BrokenComponentDidMount; + var BrokenComponentWillReceiveProps; + var BrokenComponentWillUpdate; + var BrokenComponentDidUpdate; var BrokenComponentWillUnmount; var BrokenErrorBoundary; var BrokenRender; @@ -94,63 +97,153 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentWillUnmount = class BrokenComponentWillUnmount extends React.Component { + BrokenComponentDidMount = class BrokenComponentDidMount extends React.Component { constructor(props) { super(props); - log.push('BrokenComponentWillUnmount constructor'); + log.push('BrokenComponentDidMount constructor'); } render() { - log.push('BrokenComponentWillUnmount render'); + log.push('BrokenComponentDidMount render'); return
; } componentWillMount() { - log.push('BrokenComponentWillUnmount componentWillMount'); + log.push('BrokenComponentDidMount componentWillMount'); } componentDidMount() { - log.push('BrokenComponentWillUnmount componentDidMount'); + log.push('BrokenComponentDidMount componentDidMount [!]'); + throw new Error('Hello'); } componentWillReceiveProps() { - log.push('BrokenComponentWillUnmount componentWillReceiveProps'); + log.push('BrokenComponentDidMount componentWillReceiveProps'); } componentWillUpdate() { - log.push('BrokenComponentWillUnmount componentWillUpdate'); + log.push('BrokenComponentDidMount componentWillUpdate'); } componentDidUpdate() { - log.push('BrokenComponentWillUnmount componentDidUpdate'); + log.push('BrokenComponentDidMount componentDidUpdate'); } componentWillUnmount() { - log.push('BrokenComponentWillUnmount componentWillUnmount [!]'); + log.push('BrokenComponentDidMount componentWillUnmount'); + } + }; + + BrokenComponentWillReceiveProps = class BrokenComponentWillReceiveProps extends React.Component { + constructor(props) { + super(props); + log.push('BrokenComponentWillReceiveProps constructor'); + } + render() { + log.push('BrokenComponentWillReceiveProps render'); + return
; + } + componentWillMount() { + log.push('BrokenComponentWillReceiveProps componentWillMount'); + } + componentDidMount() { + log.push('BrokenComponentWillReceiveProps componentDidMount'); + } + componentWillReceiveProps() { + log.push('BrokenComponentWillReceiveProps componentWillReceiveProps [!]'); throw new Error('Hello'); } + componentWillUpdate() { + log.push('BrokenComponentWillReceiveProps componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenComponentWillReceiveProps componentDidUpdate'); + } + componentWillUnmount() { + log.push('BrokenComponentWillReceiveProps componentWillUnmount'); + } }; - BrokenComponentDidMount = class BrokenComponentDidMount extends React.Component { + BrokenComponentWillUpdate = class BrokenComponentWillUpdate extends React.Component { constructor(props) { super(props); - log.push('BrokenComponentDidMount constructor'); + log.push('BrokenComponentWillUpdate constructor'); } render() { - log.push('BrokenComponentDidMount render'); + log.push('BrokenComponentWillUpdate render'); return
; } componentWillMount() { - log.push('BrokenComponentDidMount componentWillMount'); + log.push('BrokenComponentWillUpdate componentWillMount'); } componentDidMount() { - log.push('BrokenComponentDidMount componentDidMount [!]'); + log.push('BrokenComponentWillUpdate componentDidMount'); + } + componentWillReceiveProps() { + log.push('BrokenComponentWillUpdate componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenComponentWillUpdate componentWillUpdate [!]'); throw new Error('Hello'); } + componentDidUpdate() { + log.push('BrokenComponentWillUpdate componentDidUpdate'); + } + componentWillUnmount() { + log.push('BrokenComponentWillUpdate componentWillUnmount'); + } + }; + + BrokenComponentDidUpdate = class BrokenComponentDidUpdate extends React.Component { + constructor(props) { + super(props); + log.push('BrokenComponentDidUpdate constructor'); + } + render() { + log.push('BrokenComponentDidUpdate render'); + return
; + } + componentWillMount() { + log.push('BrokenComponentDidUpdate componentWillMount'); + } + componentDidMount() { + log.push('BrokenComponentDidUpdate componentDidMount'); + } componentWillReceiveProps() { - log.push('BrokenComponentDidMount componentWillReceiveProps'); + log.push('BrokenComponentDidUpdate componentWillReceiveProps'); } componentWillUpdate() { - log.push('BrokenComponentDidMount componentWillUpdate'); + log.push('BrokenComponentDidUpdate componentWillUpdate'); } componentDidUpdate() { - log.push('BrokenComponentDidMount componentDidUpdate'); + log.push('BrokenComponentDidUpdate componentDidUpdate [!]'); + throw new Error('Hello'); } componentWillUnmount() { - log.push('BrokenComponentDidMount componentWillUnmount'); + log.push('BrokenComponentDidUpdate componentWillUnmount'); + } + }; + + BrokenComponentWillUnmount = class BrokenComponentWillUnmount extends React.Component { + constructor(props) { + super(props); + log.push('BrokenComponentWillUnmount constructor'); + } + render() { + log.push('BrokenComponentWillUnmount render'); + return
; + } + componentWillMount() { + log.push('BrokenComponentWillUnmount componentWillMount'); + } + componentDidMount() { + log.push('BrokenComponentWillUnmount componentDidMount'); + } + componentWillReceiveProps() { + log.push('BrokenComponentWillUnmount componentWillReceiveProps'); + } + componentWillUpdate() { + log.push('BrokenComponentWillUnmount componentWillUpdate'); + } + componentDidUpdate() { + log.push('BrokenComponentWillUnmount componentDidUpdate'); + } + componentWillUnmount() { + log.push('BrokenComponentWillUnmount componentWillUnmount [!]'); + throw new Error('Hello'); } }; @@ -335,6 +428,53 @@ describe('ReactErrorBoundaries', () => { }; }); + // Known limitation: error boundary only "sees" errors caused by updates + // flowing through it. This might be easier to fix in Fiber. + it('currently does not catch errors originating downstream', () => { + var fail = false; + class Stateful extends React.Component { + state = {shouldThrow: false}; + + render() { + if (fail) { + log.push('Stateful render [!]'); + throw new Error('Hello'); + } + return
; + } + } + + var statefulInst; + var container = document.createElement('div'); + ReactDOM.render( + + statefulInst = inst} /> + , + container + ); + + log.length = 0; + expect(() => { + fail = true; + statefulInst.forceUpdate(); + }).toThrow(); + + expect(log).toEqual([ + 'Stateful render [!]', + // FIXME: uncomment when downstream errors get caught. + // Catch and render an error message + // 'ErrorBoundary unstable_handleError', + // 'ErrorBoundary render error', + // 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + it('renders an error state if child throws in render', () => { var container = document.createElement('div'); ReactDOM.render( @@ -699,7 +839,7 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('catches if child throws in render during update', () => { + it('catches if child throws in constructor during update', () => { var container = document.createElement('div'); ReactDOM.render( @@ -707,17 +847,191 @@ describe('ReactErrorBoundaries', () => { , container ); + + log.length = 0; + ReactDOM.render( + + + + + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render success', - 'Normal constructor', - 'Normal componentWillMount', + 'Normal componentWillReceiveProps', + 'Normal componentWillUpdate', 'Normal render', - 'Normal componentDidMount', - 'ErrorBoundary componentDidMount', + // Normal2 will attempt to mount: + 'Normal2 constructor', + 'Normal2 componentWillMount', + 'Normal2 render', + // BrokenConstructor will abort rendering: + 'BrokenConstructor constructor [!]', + 'ErrorBoundary unstable_handleError', + // Unmount the previously mounted components: + 'Normal componentWillUnmount', + // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ]); + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('catches if child throws in componentWillMount during update', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + + log.length = 0; + ReactDOM.render( + + + + + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render success', + 'Normal componentWillReceiveProps', + 'Normal componentWillUpdate', + 'Normal render', + // Normal2 will attempt to mount: + 'Normal2 constructor', + 'Normal2 componentWillMount', + 'Normal2 render', + // BrokenComponentWillMount will abort rendering: + 'BrokenComponentWillMount constructor', + 'BrokenComponentWillMount componentWillMount [!]', + 'ErrorBoundary unstable_handleError', + // Unmount the previously mounted components: + 'Normal componentWillUnmount', + // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('catches if child throws in componentWillReceiveProps during update', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + + , + container + ); + + log.length = 0; + ReactDOM.render( + + + + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render success', + 'Normal componentWillReceiveProps', + 'Normal componentWillUpdate', + 'Normal render', + // BrokenComponentWillReceiveProps will abort rendering: + 'BrokenComponentWillReceiveProps componentWillReceiveProps [!]', + 'ErrorBoundary unstable_handleError', + // Unmount the previously mounted components: + 'Normal componentWillUnmount', + 'BrokenComponentWillReceiveProps componentWillUnmount', + // Render error: + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('catches if child throws in componentWillUpdate during update', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + + , + container + ); + + log.length = 0; + ReactDOM.render( + + + + , + container + ); + expect(container.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render success', + 'Normal componentWillReceiveProps', + 'Normal componentWillUpdate', + 'Normal render', + // BrokenComponentWillUpdate will abort rendering: + 'BrokenComponentWillUpdate componentWillReceiveProps', + 'BrokenComponentWillUpdate componentWillUpdate [!]', + 'ErrorBoundary unstable_handleError', + // Unmount the previously mounted components: + 'Normal componentWillUnmount', + 'BrokenComponentWillUpdate componentWillUnmount', + // Render error: + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('catches if child throws in render during update', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + log.length = 0; ReactDOM.render( @@ -819,6 +1133,49 @@ describe('ReactErrorBoundaries', () => { ]); }); + // Known limitation because componentDidUpdate() does not occur on the stack. + // We could either hardcode searching for parent boundary, or wait for Fiber. + it('currently does not catch errors in componentDidUpdate', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + + log.length = 0; + expect(() => { + ReactDOM.render( + + + , + container + ); + }).toThrow(); + expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render success', + 'BrokenComponentDidUpdate componentWillReceiveProps', + 'BrokenComponentDidUpdate componentWillUpdate', + 'BrokenComponentDidUpdate render', + 'BrokenComponentDidUpdate componentDidUpdate [!]', + // FIXME: uncomment when componentDidUpdate() gets caught. + // Catch and render an error message + // 'ErrorBoundary unstable_handleError', + // 'ErrorBoundary render error', + // 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + 'BrokenComponentDidUpdate componentWillUnmount', + ]); + }); + it('recovers from componentWillUnmount errors on update', () => { var container = document.createElement('div'); ReactDOM.render( @@ -829,26 +1186,6 @@ describe('ReactErrorBoundaries', () => { , container ); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - // Mount three children: - 'BrokenComponentWillUnmount constructor', - 'BrokenComponentWillUnmount componentWillMount', - 'BrokenComponentWillUnmount render', - 'BrokenComponentWillUnmount constructor', - 'BrokenComponentWillUnmount componentWillMount', - 'BrokenComponentWillUnmount render', - 'BrokenComponentWillUnmount constructor', - 'BrokenComponentWillUnmount componentWillMount', - 'BrokenComponentWillUnmount render', - // Finalize mounting - 'BrokenComponentWillUnmount componentDidMount', - 'BrokenComponentWillUnmount componentDidMount', - 'BrokenComponentWillUnmount componentDidMount', - 'ErrorBoundary componentDidMount' - ]); log.length = 0; ReactDOM.render( @@ -904,27 +1241,6 @@ describe('ReactErrorBoundaries', () => { , container ); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - // Mount first child: - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', - 'BrokenComponentWillUnmount constructor', - 'BrokenComponentWillUnmount componentWillMount', - 'BrokenComponentWillUnmount render', - // Mount second child: - 'BrokenComponentWillUnmount constructor', - 'BrokenComponentWillUnmount componentWillMount', - 'BrokenComponentWillUnmount render', - // Finalize mounting - 'BrokenComponentWillUnmount componentDidMount', - 'Normal componentDidMount', - 'BrokenComponentWillUnmount componentDidMount', - 'ErrorBoundary componentDidMount' - ]); log.length = 0; ReactDOM.render( From 99dad51723470f02a161dd6d4466c0cc87a4dce6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 18:26:27 +0100 Subject: [PATCH 19/23] Verify we can recover from error state --- .../__tests__/ReactErrorBoundaries-test.js | 50 ++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index e13d5e42bea9..6011f93ef8cf 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -367,7 +367,7 @@ describe('ReactErrorBoundaries', () => { log.push('ErrorBoundary constructor'); } render() { - if (this.state.error) { + if (this.state.error && !this.props.forceRetry) { log.push('ErrorBoundary render error'); return this.props.renderError(this.state.error, this.props); } @@ -1281,6 +1281,54 @@ describe('ReactErrorBoundaries', () => { ]); }); + it('can recover from error state', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + + ReactDOM.render( + + + , + container + ); + // Error boundary doesn't retry by itself: + expect(container.textContent).toBe('Caught an error: Hello.'); + + // Force the success path: + log.length = 0; + ReactDOM.render( + + + , + container + ); + expect(container.textContent).not.toContain('Caught an error'); + expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render success', + // Mount children: + 'Normal constructor', + 'Normal componentWillMount', + 'Normal render', + // Finalize updates: + 'Normal componentDidMount', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + 'Normal componentWillUnmount', + ]); + }); + it('can update multiple times in error state', () => { var container = document.createElement('div'); ReactDOM.render( From de5bdf154babc242cc1bb1b57ebcad38dfd2dc9a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 14 Oct 2016 18:29:07 +0100 Subject: [PATCH 20/23] Fix lint --- .../__tests__/ReactErrorBoundaries-test.js | 44 +++++++++---------- 1 file changed, 20 insertions(+), 24 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 6011f93ef8cf..3b1b369434e4 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -37,7 +37,7 @@ describe('ReactErrorBoundaries', () => { log = []; - BrokenConstructor = class BrokenConstructor extends React.Component { + BrokenConstructor = class extends React.Component { constructor(props) { super(props); log.push('BrokenConstructor constructor [!]'); @@ -67,7 +67,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentWillMount = class BrokenComponentWillMount extends React.Component { + BrokenComponentWillMount = class extends React.Component { constructor(props) { super(props); log.push('BrokenComponentWillMount constructor'); @@ -97,7 +97,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentDidMount = class BrokenComponentDidMount extends React.Component { + BrokenComponentDidMount = class extends React.Component { constructor(props) { super(props); log.push('BrokenComponentDidMount constructor'); @@ -127,7 +127,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentWillReceiveProps = class BrokenComponentWillReceiveProps extends React.Component { + BrokenComponentWillReceiveProps = class extends React.Component { constructor(props) { super(props); log.push('BrokenComponentWillReceiveProps constructor'); @@ -157,7 +157,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentWillUpdate = class BrokenComponentWillUpdate extends React.Component { + BrokenComponentWillUpdate = class extends React.Component { constructor(props) { super(props); log.push('BrokenComponentWillUpdate constructor'); @@ -187,7 +187,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentDidUpdate = class BrokenComponentDidUpdate extends React.Component { + BrokenComponentDidUpdate = class extends React.Component { constructor(props) { super(props); log.push('BrokenComponentDidUpdate constructor'); @@ -217,7 +217,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenComponentWillUnmount = class BrokenComponentWillUnmount extends React.Component { + BrokenComponentWillUnmount = class extends React.Component { constructor(props) { super(props); log.push('BrokenComponentWillUnmount constructor'); @@ -247,7 +247,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenErrorBoundary = class BrokenErrorBoundary extends React.Component { + BrokenErrorBoundary = class extends React.Component { constructor(props) { super(props); this.state = {error: null}; @@ -276,7 +276,7 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenRender = class BrokenRender extends React.Component { + BrokenRender = class extends React.Component { constructor(props) { super(props); log.push('BrokenRender constructor'); @@ -305,7 +305,7 @@ describe('ReactErrorBoundaries', () => { } }; - NoopErrorBoundary = class NoopErrorBoundary extends React.Component { + NoopErrorBoundary = class extends React.Component { constructor(props) { super(props); log.push('NoopErrorBoundary constructor'); @@ -328,7 +328,7 @@ describe('ReactErrorBoundaries', () => { } }; - Normal = class Normal extends React.Component { + Normal = class extends React.Component { static defaultProps = { logName: 'Normal', }; @@ -360,7 +360,7 @@ describe('ReactErrorBoundaries', () => { } }; - ErrorBoundary = class ErrorBoundary extends React.Component { + ErrorBoundary = class extends React.Component { constructor() { super(); this.state = {error: null}; @@ -407,7 +407,7 @@ describe('ReactErrorBoundaries', () => { }, }; - ErrorMessage = class ErrorMessage extends React.Component { + ErrorMessage = class extends React.Component { constructor(props) { super(props); log.push('ErrorMessage constructor'); @@ -764,7 +764,7 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary unstable_handleError', // Render the error message 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount' + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -777,10 +777,10 @@ describe('ReactErrorBoundaries', () => { it('resets refs if mounting aborts', () => { function childRef(x) { log.push('Child ref is set to ' + x); - }; + } function errorMessageRef(x) { log.push('Error message ref is set to ' + x); - }; + } var container = document.createElement('div'); ReactDOM.render( @@ -1075,13 +1075,13 @@ describe('ReactErrorBoundaries', () => { it('keeps refs up-to-date during updates', () => { function child1Ref(x) { log.push('Child1 ref is set to ' + x); - }; + } function child2Ref(x) { log.push('Child2 ref is set to ' + x); - }; + } function errorMessageRef(x) { log.push('Error message ref is set to ' + x); - }; + } var container = document.createElement('div'); ReactDOM.render( @@ -1227,10 +1227,6 @@ describe('ReactErrorBoundaries', () => { }); it('recovers from nested componentWillUnmount errors on update', () => { - function ParentOfBrokenComponentWillUnmount() { - return ; - } - var container = document.createElement('div'); ReactDOM.render( @@ -1302,7 +1298,7 @@ describe('ReactErrorBoundaries', () => { // Force the success path: log.length = 0; ReactDOM.render( - + , container From c64dbfd64678a9f01ba9584624ab63227ca25c36 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 15 Oct 2016 00:34:04 +0100 Subject: [PATCH 21/23] =?UTF-8?q?Error=20in=20boundary=E2=80=99s=20compone?= =?UTF-8?q?ntWillMount=20should=20propagate=20up?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This test is currently failing. --- .../__tests__/ReactErrorBoundaries-test.js | 95 +++++++++++++++---- 1 file changed, 78 insertions(+), 17 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 3b1b369434e4..ec3ae5029f86 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -24,7 +24,8 @@ describe('ReactErrorBoundaries', () => { var BrokenComponentWillUpdate; var BrokenComponentDidUpdate; var BrokenComponentWillUnmount; - var BrokenErrorBoundary; + var BrokenRenderErrorBoundary; + var BrokenComponentWillMountErrorBoundary; var BrokenRender; var ErrorBoundary; var ErrorMessage; @@ -247,31 +248,61 @@ describe('ReactErrorBoundaries', () => { } }; - BrokenErrorBoundary = class extends React.Component { + BrokenComponentWillMountErrorBoundary = class extends React.Component { constructor(props) { super(props); this.state = {error: null}; - log.push('BrokenErrorBoundary constructor'); + log.push('BrokenComponentWillMountErrorBoundary constructor'); } render() { if (this.state.error) { - log.push('BrokenErrorBoundary render error [!]'); + log.push('BrokenComponentWillMountErrorBoundary render error'); + return
Caught an error: {this.state.error.message}.
; + } + log.push('BrokenComponentWillMountErrorBoundary render success'); + return
{this.props.children}
; + } + componentWillMount() { + log.push('BrokenComponentWillMountErrorBoundary componentWillMount [!]'); + throw new Error('Hello'); + } + componentDidMount() { + log.push('BrokenComponentWillMountErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('BrokenComponentWillMountErrorBoundary componentWillUnmount'); + } + unstable_handleError(error) { + log.push('BrokenComponentWillMountErrorBoundary unstable_handleError'); + this.setState({error}); + } + }; + + BrokenRenderErrorBoundary = class extends React.Component { + constructor(props) { + super(props); + this.state = {error: null}; + log.push('BrokenRenderErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('BrokenRenderErrorBoundary render error [!]'); throw new Error('Hello'); } - log.push('BrokenErrorBoundary render success'); + log.push('BrokenRenderErrorBoundary render success'); return
{this.props.children}
; } componentWillMount() { - log.push('BrokenErrorBoundary componentWillMount'); + log.push('BrokenRenderErrorBoundary componentWillMount'); } componentDidMount() { - log.push('BrokenErrorBoundary componentDidMount'); + log.push('BrokenRenderErrorBoundary componentDidMount'); } componentWillUnmount() { - log.push('BrokenErrorBoundary componentWillUnmount'); + log.push('BrokenRenderErrorBoundary componentWillUnmount'); } unstable_handleError(error) { - log.push('BrokenErrorBoundary unstable_handleError'); + log.push('BrokenRenderErrorBoundary unstable_handleError'); this.setState({error}); } }; @@ -678,13 +709,43 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('propagates errors inside boundary itself on mounting', () => { + it('propagates errors inside boundary during componentWillMount', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentWillMountErrorBoundary constructor', + 'BrokenComponentWillMountErrorBoundary componentWillMount [!]', + // The error propagates to the higher boundary + 'ErrorBoundary unstable_handleError', + // Render the error + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + + it('propagates errors inside boundary while rendering error state', () => { var container = document.createElement('div'); ReactDOM.render( - + - + , container ); @@ -693,16 +754,16 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', - 'BrokenErrorBoundary constructor', - 'BrokenErrorBoundary componentWillMount', - 'BrokenErrorBoundary render success', + 'BrokenRenderErrorBoundary constructor', + 'BrokenRenderErrorBoundary componentWillMount', + 'BrokenRenderErrorBoundary render success', 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', // The first error boundary catches the error // It adjusts state but throws displaying the message - 'BrokenErrorBoundary unstable_handleError', - 'BrokenErrorBoundary render error [!]', + 'BrokenRenderErrorBoundary unstable_handleError', + 'BrokenRenderErrorBoundary render error [!]', // The error propagates to the higher boundary 'ErrorBoundary unstable_handleError', // Render the error From 7026adc0259e58422e3acb3ee798d4232a74bd8e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 15 Oct 2016 00:34:55 +0100 Subject: [PATCH 22/23] 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. --- .../reconciler/ReactCompositeComponent.js | 61 ++++++++----------- 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 4e54a8439a38..3c49fc464df1 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -333,6 +333,23 @@ var ReactCompositeComponent = { this._pendingReplaceState = false; this._pendingForceUpdate = false; + if (inst.componentWillMount) { + if (__DEV__) { + measureLifeCyclePerf( + () => inst.componentWillMount(), + this._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); + } + } + var markup; if (inst.unstable_handleError) { markup = this.performInitialMountWithErrorHandling( @@ -348,8 +365,7 @@ var ReactCompositeComponent = { hostParent, hostContainerInfo, transaction, - context, - false /* skipLifecyle */ + context ); } @@ -446,8 +462,7 @@ var ReactCompositeComponent = { hostParent, hostContainerInfo, transaction, - context, - false /* skipLifecyle */ + context ); } catch (e) { // Roll back to checkpoint, handle error (which may add items to the transaction), and take a new checkpoint @@ -471,9 +486,7 @@ var ReactCompositeComponent = { hostParent, hostContainerInfo, transaction, - context, - // We have already called componentWillMount() before retrying: - true /* skipLifecyle */ + context ); } return markup; @@ -484,35 +497,8 @@ var ReactCompositeComponent = { hostParent, hostContainerInfo, transaction, - context, - skipLifecyle + context ) { - var inst = this._instance; - - var debugID = 0; - if (__DEV__) { - debugID = this._debugID; - } - - if (!skipLifecyle) { - 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 not a stateless component, we now render if (renderedElement === undefined) { renderedElement = this._renderValidatedComponent(); @@ -526,6 +512,11 @@ var ReactCompositeComponent = { ); this._renderedComponent = child; + var debugID = 0; + if (__DEV__) { + debugID = this._debugID; + } + var markup = ReactReconciler.mountComponent( child, transaction, From 9f66fa4a6fd38a30b020715dbe6bbf2e5d370641 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 15 Oct 2016 00:37:26 +0100 Subject: [PATCH 23/23] Remove extra whitespace --- .../stack/reconciler/__tests__/ReactErrorBoundaries-test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index ec3ae5029f86..ac5e410cad91 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -738,7 +738,6 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('propagates errors inside boundary while rendering error state', () => { var container = document.createElement('div'); ReactDOM.render(