From bd3c90ac6f9b601e0c0b53ae38b0e8d4bbc5ac03 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 28 Oct 2016 15:29:19 +0100 Subject: [PATCH 1/5] Add more tests for error boundaries --- .../__tests__/ReactErrorBoundaries-test.js | 461 ++++++++++++------ 1 file changed, 299 insertions(+), 162 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 78fa5fe6ce..63762a5fcb 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -28,6 +28,7 @@ describe('ReactErrorBoundaries', () => { var BrokenComponentWillUnmount; var BrokenRenderErrorBoundary; var BrokenComponentWillMountErrorBoundary; + var BrokenComponentDidMountErrorBoundary; var BrokenRender; var ErrorBoundary; var ErrorMessage; @@ -283,6 +284,36 @@ describe('ReactErrorBoundaries', () => { } }; + BrokenComponentDidMountErrorBoundary = class extends React.Component { + constructor(props) { + super(props); + this.state = {error: null}; + log.push('BrokenComponentDidMountErrorBoundary constructor'); + } + render() { + if (this.state.error) { + log.push('BrokenComponentDidMountErrorBoundary render error'); + return
Caught an error: {this.state.error.message}.
; + } + log.push('BrokenComponentDidMountErrorBoundary render success'); + return
{this.props.children}
; + } + componentWillMount() { + log.push('BrokenComponentDidMountErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('BrokenComponentDidMountErrorBoundary componentDidMount [!]'); + throw new Error('Hello'); + } + componentWillUnmount() { + log.push('BrokenComponentDidMountErrorBoundary componentWillUnmount'); + } + unstable_handleError(error) { + log.push('BrokenComponentDidMountErrorBoundary unstable_handleError'); + this.setState({error}); + } + }; + BrokenRenderErrorBoundary = class extends React.Component { constructor(props) { super(props); @@ -397,43 +428,44 @@ describe('ReactErrorBoundaries', () => { }; ErrorBoundary = class extends React.Component { - constructor() { - super(); + constructor(props) { + super(props); this.state = {error: null}; - log.push('ErrorBoundary constructor'); + log.push(`${this.props.logName} constructor`); } render() { if (this.state.error && !this.props.forceRetry) { - log.push('ErrorBoundary render error'); + log.push(`${this.props.logName} render error`); return this.props.renderError(this.state.error, this.props); } - log.push('ErrorBoundary render success'); + log.push(`${this.props.logName} render success`); return
{this.props.children}
; } unstable_handleError(error) { - log.push('ErrorBoundary unstable_handleError'); + log.push(`${this.props.logName} unstable_handleError`); this.setState({error}); } componentWillMount() { - log.push('ErrorBoundary componentWillMount'); + log.push(`${this.props.logName} componentWillMount`); } componentDidMount() { - log.push('ErrorBoundary componentDidMount'); + log.push(`${this.props.logName} componentDidMount`); } componentWillReceiveProps() { - log.push('ErrorBoundary componentWillReceiveProps'); + log.push(`${this.props.logName} componentWillReceiveProps`); } componentWillUpdate() { - log.push('ErrorBoundary componentWillUpdate'); + log.push(`${this.props.logName} componentWillUpdate`); } componentDidUpdate() { - log.push('ErrorBoundary componentDidUpdate'); + log.push(`${this.props.logName} componentDidUpdate`); } componentWillUnmount() { - log.push('ErrorBoundary componentWillUnmount'); + log.push(`${this.props.logName} componentWillUnmount`); } }; ErrorBoundary.defaultProps = { + logName: 'ErrorBoundary', renderError(error, props) { return (
@@ -464,53 +496,6 @@ describe('ReactErrorBoundaries', () => { }; }); - if (ReactDOMFeatureFlags.useFiber) { - // This test implements a new feature in Fiber. - it('catches 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
{this.props.children}
; - } - } - - var statefulInst; - var container = document.createElement('div'); - ReactDOM.render( - - statefulInst = inst} /> - , - container - ); - - log.length = 0; - expect(() => { - fail = true; - statefulInst.forceUpdate(); - }).not.toThrow(); - - expect(log).toEqual([ - 'Stateful render [!]', - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary componentWillUpdate', - '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( @@ -661,65 +646,6 @@ describe('ReactErrorBoundaries', () => { ]); }); - if (ReactDOMFeatureFlags.useFiber) { - // This test implements a new feature in Fiber. - it('catches errors in componentDidMount', () => { - var container = document.createElement('div'); - ReactDOM.render( - - - - - - - , - container - ); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenComponentWillUnmount constructor', - 'BrokenComponentWillUnmount componentWillMount', - 'BrokenComponentWillUnmount render', - 'Normal constructor', - 'Normal componentWillMount', - 'Normal render', - 'BrokenComponentDidMount constructor', - 'BrokenComponentDidMount componentWillMount', - 'BrokenComponentDidMount render', - 'LastChild constructor', - 'LastChild componentWillMount', - 'LastChild render', - // Start flushing didMount queue - 'Normal componentDidMount', - 'BrokenComponentWillUnmount componentDidMount', - 'BrokenComponentDidMount componentDidMount [!]', - // Continue despite the error - 'LastChild componentDidMount', - 'ErrorBoundary componentDidMount', - // Now we are ready to handle the error - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary componentWillUpdate', - 'ErrorBoundary render error', - // Safely unmount every child - 'BrokenComponentWillUnmount componentWillUnmount [!]', - // Continue unmounting safely despite any errors - 'Normal componentWillUnmount', - 'BrokenComponentDidMount componentWillUnmount', - 'LastChild componentWillUnmount', - // The update has finished - 'ErrorBoundary componentDidUpdate', - ]); - - log.length = 0; - ReactDOM.unmountComponentAtNode(container); - expect(log).toEqual([ - 'ErrorBoundary componentWillUnmount', - ]); - }); - } - it('propagates errors on retry on mounting', () => { var container = document.createElement('div'); ReactDOM.render( @@ -1356,50 +1282,6 @@ describe('ReactErrorBoundaries', () => { ]); }); - if (ReactDOMFeatureFlags.useFiber) { - // This test implements a new feature in Fiber. - it('catches errors in componentDidUpdate', () => { - var container = document.createElement('div'); - ReactDOM.render( - - - , - container - ); - - log.length = 0; - ReactDOM.render( - - - , - container - ); - expect(log).toEqual([ - 'ErrorBoundary componentWillReceiveProps', - 'ErrorBoundary componentWillUpdate', - 'ErrorBoundary render success', - 'BrokenComponentDidUpdate componentWillReceiveProps', - 'BrokenComponentDidUpdate componentWillUpdate', - 'BrokenComponentDidUpdate render', - // All lifecycles run - 'BrokenComponentDidUpdate componentDidUpdate [!]', - 'ErrorBoundary componentDidUpdate', - // Then, error is handled - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary componentWillUpdate', - 'ErrorBoundary render error', - 'BrokenComponentDidUpdate componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - ]); - - log.length = 0; - ReactDOM.unmountComponentAtNode(container); - expect(log).toEqual([ - 'ErrorBoundary componentWillUnmount', - ]); - }); - } - it('recovers from componentWillUnmount errors on update', () => { var container = document.createElement('div'); ReactDOM.render( @@ -1535,6 +1417,76 @@ describe('ReactErrorBoundaries', () => { ]); }); + it('picks the right boundary when handling unmounting errors', () => { + function renderInnerError(error, props) { + return
Caught an inner error: {error.message}.
; + } + function renderOuterError(error, props) { + return
Caught an outer error: {error.message}.
; + } + + var container = document.createElement('div'); + ReactDOM.render( + + + + + , + container + ); + + log.length = 0; + ReactDOM.render( + + + , + container + ); + expect(container.textContent).toBe('Caught an inner error: Hello.'); + expect(log).toEqual([ + // Update outer boundary + 'OuterErrorBoundary componentWillReceiveProps', + 'OuterErrorBoundary componentWillUpdate', + 'OuterErrorBoundary render success', + // Update inner boundary + 'InnerErrorBoundary componentWillReceiveProps', + 'InnerErrorBoundary componentWillUpdate', + 'InnerErrorBoundary render success', + // Try unmounting child + 'BrokenComponentWillUnmount componentWillUnmount [!]', + ...(ReactDOMFeatureFlags.useFiber ? [ + // Fiber proceeds with lifecycles despite errors + // Inner and outer boundaries have updated in this phase + 'InnerErrorBoundary componentDidUpdate', + 'OuterErrorBoundary componentDidUpdate', + // Now that commit phase is done, Fiber handles errors + // Only inner boundary receives the error: + 'InnerErrorBoundary unstable_handleError', + 'InnerErrorBoundary componentWillUpdate', + // Render an error now + 'InnerErrorBoundary render error', + // In Fiber, this was a local update to the + // inner boundary so only its hook fires + 'InnerErrorBoundary componentDidUpdate', + ] : [ + // Stack will handle error immediately + 'InnerErrorBoundary unstable_handleError', + 'InnerErrorBoundary render error', + // In stack, this was a part of the update to the + // outer boundary so both lifecycles fire + 'InnerErrorBoundary componentDidUpdate', + 'OuterErrorBoundary componentDidUpdate', + ]), + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'OuterErrorBoundary componentWillUnmount', + 'InnerErrorBoundary componentWillUnmount', + ]); + }); + it('can recover from error state', () => { var container = document.createElement('div'); ReactDOM.render( @@ -1701,4 +1653,189 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary componentWillUnmount', ]); }); + + // The tests below implement new features in Fiber. + if (ReactDOMFeatureFlags.useFiber) { + it('catches 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
{this.props.children}
; + } + } + + var statefulInst; + var container = document.createElement('div'); + ReactDOM.render( + + statefulInst = inst} /> + , + container + ); + + log.length = 0; + expect(() => { + fail = true; + statefulInst.forceUpdate(); + }).not.toThrow(); + + expect(log).toEqual([ + 'Stateful render [!]', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('catches errors in componentDidMount', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + + + + + , + container + ); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentWillUnmount constructor', + 'BrokenComponentWillUnmount componentWillMount', + 'BrokenComponentWillUnmount render', + 'Normal constructor', + 'Normal componentWillMount', + 'Normal render', + 'BrokenComponentDidMount constructor', + 'BrokenComponentDidMount componentWillMount', + 'BrokenComponentDidMount render', + 'LastChild constructor', + 'LastChild componentWillMount', + 'LastChild render', + // Start flushing didMount queue + 'Normal componentDidMount', + 'BrokenComponentWillUnmount componentDidMount', + 'BrokenComponentDidMount componentDidMount [!]', + // Continue despite the error + 'LastChild componentDidMount', + 'ErrorBoundary componentDidMount', + // Now we are ready to handle the error + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + // Safely unmount every child + 'BrokenComponentWillUnmount componentWillUnmount [!]', + // Continue unmounting safely despite any errors + 'Normal componentWillUnmount', + 'BrokenComponentDidMount componentWillUnmount', + 'LastChild componentWillUnmount', + // The update has finished + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('propagates errors inside boundary during componentDidMount', () => { + var container = document.createElement('div'); + ReactDOM.render( + + ( +
+ We should never catch our own error: {error.message}. +
+ )} /> + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentDidMountErrorBoundary constructor', + 'BrokenComponentDidMountErrorBoundary componentWillMount', + 'BrokenComponentDidMountErrorBoundary render success', + 'BrokenComponentDidMountErrorBoundary componentDidMount [!]', + // Fiber proceeds with the hooks + 'ErrorBoundary componentDidMount', + // The error propagates to the higher boundary + 'ErrorBoundary unstable_handleError', + // Fiber retries from the root + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'BrokenComponentDidMountErrorBoundary componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('catches errors in componentDidUpdate', () => { + var container = document.createElement('div'); + ReactDOM.render( + + + , + container + ); + + log.length = 0; + ReactDOM.render( + + + , + container + ); + expect(log).toEqual([ + 'ErrorBoundary componentWillReceiveProps', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render success', + 'BrokenComponentDidUpdate componentWillReceiveProps', + 'BrokenComponentDidUpdate componentWillUpdate', + 'BrokenComponentDidUpdate render', + // All lifecycles run + 'BrokenComponentDidUpdate componentDidUpdate [!]', + 'ErrorBoundary componentDidUpdate', + // Then, error is handled + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'BrokenComponentDidUpdate componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + } + }); From f51abe0b552ceabfa926957337d182d6f7173344 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 28 Oct 2016 15:42:06 +0100 Subject: [PATCH 2/5] Add a failing test for multiple independent boundaries Now that commits are treated as atomic, it is possible that componentDidMount, componentDidUpdate, or componentWillUnmount threw in multiple places during the commit. We need to make sure we notify all affected boundaries of the first errors in them. --- .../__tests__/ReactErrorBoundaries-test.js | 189 ++++++++++++++---- 1 file changed, 145 insertions(+), 44 deletions(-) diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js index 63762a5fcb..e7475d5eef 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -195,6 +195,9 @@ describe('ReactErrorBoundaries', () => { }; BrokenComponentDidUpdate = class extends React.Component { + static defaultProps = { + errorText: 'Hello' + }; constructor(props) { super(props); log.push('BrokenComponentDidUpdate constructor'); @@ -217,7 +220,7 @@ describe('ReactErrorBoundaries', () => { } componentDidUpdate() { log.push('BrokenComponentDidUpdate componentDidUpdate [!]'); - throw new Error('Hello'); + throw new Error(this.props.errorText); } componentWillUnmount() { log.push('BrokenComponentDidUpdate componentWillUnmount'); @@ -225,6 +228,9 @@ describe('ReactErrorBoundaries', () => { }; BrokenComponentWillUnmount = class extends React.Component { + static defaultProps = { + errorText: 'Hello' + }; constructor(props) { super(props); log.push('BrokenComponentWillUnmount constructor'); @@ -250,7 +256,7 @@ describe('ReactErrorBoundaries', () => { } componentWillUnmount() { log.push('BrokenComponentWillUnmount componentWillUnmount [!]'); - throw new Error('Hello'); + throw new Error(this.props.errorText); } }; @@ -1418,10 +1424,10 @@ describe('ReactErrorBoundaries', () => { }); it('picks the right boundary when handling unmounting errors', () => { - function renderInnerError(error, props) { + function renderInnerError(error) { return
Caught an inner error: {error.message}.
; } - function renderOuterError(error, props) { + function renderOuterError(error) { return
Caught an outer error: {error.message}.
; } @@ -1756,46 +1762,6 @@ describe('ReactErrorBoundaries', () => { ]); }); - it('propagates errors inside boundary during componentDidMount', () => { - var container = document.createElement('div'); - ReactDOM.render( - - ( -
- We should never catch our own error: {error.message}. -
- )} /> - , - container - ); - expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); - expect(log).toEqual([ - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenComponentDidMountErrorBoundary constructor', - 'BrokenComponentDidMountErrorBoundary componentWillMount', - 'BrokenComponentDidMountErrorBoundary render success', - 'BrokenComponentDidMountErrorBoundary componentDidMount [!]', - // Fiber proceeds with the hooks - 'ErrorBoundary componentDidMount', - // The error propagates to the higher boundary - 'ErrorBoundary unstable_handleError', - // Fiber retries from the root - 'ErrorBoundary componentWillUpdate', - 'ErrorBoundary render error', - 'BrokenComponentDidMountErrorBoundary componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - ]); - - log.length = 0; - ReactDOM.unmountComponentAtNode(container); - expect(log).toEqual([ - 'ErrorBoundary componentWillUnmount', - ]); - }); - it('catches errors in componentDidUpdate', () => { var container = document.createElement('div'); ReactDOM.render( @@ -1836,6 +1802,141 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary componentWillUnmount', ]); }); + + it('propagates errors inside boundary during componentDidMount', () => { + var container = document.createElement('div'); + ReactDOM.render( + + ( +
+ We should never catch our own error: {error.message}. +
+ )} /> + , + container + ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(log).toEqual([ + 'ErrorBoundary constructor', + 'ErrorBoundary componentWillMount', + 'ErrorBoundary render success', + 'BrokenComponentDidMountErrorBoundary constructor', + 'BrokenComponentDidMountErrorBoundary componentWillMount', + 'BrokenComponentDidMountErrorBoundary render success', + 'BrokenComponentDidMountErrorBoundary componentDidMount [!]', + // Fiber proceeds with the hooks + 'ErrorBoundary componentDidMount', + // The error propagates to the higher boundary + 'ErrorBoundary unstable_handleError', + // Fiber retries from the root + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'BrokenComponentDidMountErrorBoundary componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'ErrorBoundary componentWillUnmount', + ]); + }); + + it('lets different boundaries catch their own first errors', () => { + function renderUnmountError(error) { + return
Caught an unmounting error: {error.message}.
; + } + function renderUpdateError(error) { + return
Caught an updating error: {error.message}.
; + } + + var container = document.createElement('div'); + ReactDOM.render( + + + + + + + + + + , + container + ); + + log.length = 0; + ReactDOM.render( + + + + + + + , + container + ); + + expect(container.firstChild.textContent).toBe( + 'Caught an unmounting error: E1.' + + 'Caught an updating error: E3.' + ); + expect(log).toEqual([ + // Begin update phase + 'OuterErrorBoundary componentWillReceiveProps', + 'OuterErrorBoundary componentWillUpdate', + 'OuterErrorBoundary render success', + 'InnerUnmountBoundary componentWillReceiveProps', + 'InnerUnmountBoundary componentWillUpdate', + 'InnerUnmountBoundary render success', + 'InnerUpdateBoundary componentWillReceiveProps', + 'InnerUpdateBoundary componentWillUpdate', + 'InnerUpdateBoundary render success', + // First come the updates + 'BrokenComponentDidUpdate componentWillReceiveProps', + 'BrokenComponentDidUpdate componentWillUpdate', + 'BrokenComponentDidUpdate render', + 'BrokenComponentDidUpdate componentWillReceiveProps', + 'BrokenComponentDidUpdate componentWillUpdate', + 'BrokenComponentDidUpdate render', + // We're in commit phase now, deleting + 'BrokenComponentWillUnmount componentWillUnmount [!]', + 'BrokenComponentWillUnmount componentWillUnmount [!]', + // Continue despite errors, handle them after commit is done + 'InnerUnmountBoundary componentDidUpdate', + // We're still in commit phase, now calling update lifecycles + 'BrokenComponentDidUpdate componentDidUpdate [!]', + // Again, continue despite errors, we'll handle them later + 'BrokenComponentDidUpdate componentDidUpdate [!]', + 'InnerUpdateBoundary componentDidUpdate', + 'OuterErrorBoundary componentDidUpdate', + // The interesting part starts now. + // Acknowledge errors independently but don't update yet: + 'InnerUnmountBoundary unstable_handleError', + 'InnerUpdateBoundary unstable_handleError', + // Only two of four errors are acknowledged: one per boundary. + // The rest are likely cascading and we ignore them. + // Now update: + 'InnerUnmountBoundary componentWillUpdate', + 'InnerUnmountBoundary render error', + 'InnerUpdateBoundary componentWillUpdate', + 'InnerUpdateBoundary render error', + // Commit + 'BrokenComponentDidUpdate componentWillUnmount', + 'BrokenComponentDidUpdate componentWillUnmount', + 'InnerUnmountBoundary componentDidUpdate', + 'InnerUpdateBoundary componentDidUpdate', + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'OuterErrorBoundary componentWillUnmount', + 'InnerUnmountBoundary componentWillUnmount', + 'InnerUpdateBoundary componentWillUnmount', + ]); + }); } }); From c08469a7d6647fd1e4f0480713bdf2d9e49a719f Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 28 Oct 2016 19:23:35 +0100 Subject: [PATCH 3/5] Add tests verifying we don't swallow exceptions --- .../__tests__/ReactErrorBoundaries-test.js | 88 ++++++++++++++++++- 1 file changed, 86 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 e7475d5eef..658b76a1de 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -196,7 +196,7 @@ describe('ReactErrorBoundaries', () => { BrokenComponentDidUpdate = class extends React.Component { static defaultProps = { - errorText: 'Hello' + errorText: 'Hello', }; constructor(props) { super(props); @@ -229,7 +229,7 @@ describe('ReactErrorBoundaries', () => { BrokenComponentWillUnmount = class extends React.Component { static defaultProps = { - errorText: 'Hello' + errorText: 'Hello', }; constructor(props) { super(props); @@ -502,6 +502,90 @@ describe('ReactErrorBoundaries', () => { }; }); + it('does not swallow exceptions on mounting without boundaries', () => { + var container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Hello'); + + container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Hello'); + + container = document.createElement('div'); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Hello'); + }); + + it('does not swallow exceptions on updating without boundaries', () => { + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Hello'); + + container = document.createElement('div'); + ReactDOM.render(, container); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Hello'); + + container = document.createElement('div'); + ReactDOM.render(, container); + expect(() => { + ReactDOM.render(, container); + }).toThrow('Hello'); + }); + + it('does not swallow exceptions on unmounting without boundaries', () => { + var container = document.createElement('div'); + ReactDOM.render(, container); + expect(() => { + ReactDOM.unmountComponentAtNode(container); + }).toThrow('Hello'); + }); + + it('prevents errors from leaking into other roots', () => { + var container1 = document.createElement('div'); + var container2 = document.createElement('div'); + var container3 = document.createElement('div'); + + ReactDOM.render(Before 1, container1); + expect(() => { + ReactDOM.render(, container2); + }).toThrow('Hello'); + ReactDOM.render( + + + , + container3 + ); + expect(container1.firstChild.textContent).toBe('Before 1'); + expect(container2.firstChild).toBe(null); + expect(container3.firstChild.textContent).toBe('Caught an error: Hello.'); + + ReactDOM.render(After 1, container1); + ReactDOM.render(After 2, container2); + ReactDOM.render( + + After 3 + , + container3 + ); + expect(container1.firstChild.textContent).toBe('After 1'); + expect(container2.firstChild.textContent).toBe('After 2'); + expect(container3.firstChild.textContent).toBe('After 3'); + + ReactDOM.unmountComponentAtNode(container1); + ReactDOM.unmountComponentAtNode(container2); + ReactDOM.unmountComponentAtNode(container3); + expect(container1.firstChild).toBe(null); + expect(container2.firstChild).toBe(null); + expect(container3.firstChild).toBe(null); + }); + it('renders an error state if child throws in render', () => { var container = document.createElement('div'); ReactDOM.render( From d8441cd725bcc3bc4f5be7d21be0f3788fde9590 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 28 Oct 2016 21:22:44 +0100 Subject: [PATCH 4/5] Handle multiple errors in boundaries --- src/renderers/dom/fiber/ReactDOMFiber.js | 1 + .../shared/fiber/ReactFiberScheduler.js | 146 ++++++++++++------ 2 files changed, 96 insertions(+), 51 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index d12b1aca9d..8294883db5 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -131,6 +131,7 @@ var ReactDOM = { render(element : ReactElement, container : DOMContainerElement, callback: ?Function) { warnAboutUnstableUse(); let root; + if (!container._reactRootContainer) { root = container._reactRootContainer = DOMRenderer.mountContainer(element, container, callback); } else { diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 54ca2ef2cb..d861c81cd1 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -12,6 +12,7 @@ 'use strict'; +import type { TrappedError } from 'ReactFiberErrorBoundary'; import type { Fiber } from 'ReactFiber'; import type { FiberRoot } from 'ReactFiberRoot'; import type { HostConfig } from 'ReactFiberReconciler'; @@ -205,8 +206,7 @@ module.exports = function(config : HostConfig) { // Now that the tree has been committed, we can handle errors. if (allTrappedErrors) { - // TODO: handle multiple errors with distinct boundaries. - handleError(allTrappedErrors[0]); + handleErrors(allTrappedErrors); } } @@ -377,7 +377,7 @@ module.exports = function(config : HostConfig) { throw error; } const trappedError = trapError(failedUnitOfWork, error); - handleError(trappedError); + handleErrors([trappedError]); } } @@ -408,6 +408,7 @@ module.exports = function(config : HostConfig) { // We're the only work scheduled. nextScheduledRoot = root; lastScheduledRoot = root; + scheduleDeferredCallback(performDeferredWork); } } @@ -443,7 +444,7 @@ module.exports = function(config : HostConfig) { throw error; } const trappedError = trapError(failedUnitOfWork, error); - handleError(trappedError); + handleErrors([trappedError]); } } @@ -471,56 +472,99 @@ module.exports = function(config : HostConfig) { } } - function handleError(trappedError) { - const boundary = trappedError.boundary; - const error = trappedError.error; - if (!boundary) { - throw error; + function scheduleErrorBoundaryWork(boundary : Fiber, priority) : FiberRoot { + let root = null; + let fiber = boundary; + while (fiber) { + fiber.pendingWorkPriority = priority; + if (fiber.alternate) { + fiber.alternate.pendingWorkPriority = priority; + } + if (!fiber.return) { + if (fiber.tag === HostContainer) { + // We found the root. + // Remember it so we can update it. + root = ((fiber.stateNode : any) : FiberRoot); + break; + } else { + throw new Error('Invalid root'); + } + } + fiber = fiber.return; + } + if (!root) { + throw new Error('Could not find root from the boundary.'); + } + return root; + } + + function handleErrors(initialTrappedErrors : Array) : void { + let nextTrappedErrors = initialTrappedErrors; + let firstUncaughtError = null; + + // In each phase, we will attempt to pass errors to boundaries and re-render them. + // If we get more errors, we propagate them to higher boundaries in the next iterations. + while (nextTrappedErrors) { + const trappedErrors = nextTrappedErrors; + nextTrappedErrors = null; + + // Pass errors to all affected boundaries. + const affectedBoundaries : Set = new Set(); + trappedErrors.forEach(trappedError => { + const boundary = trappedError.boundary; + const error = trappedError.error; + if (!boundary) { + firstUncaughtError = firstUncaughtError || error; + return; + } + // Don't visit boundaries twice. + if (affectedBoundaries.has(boundary)) { + return; + } + // Give error boundary a chance to update its state. + try { + acknowledgeErrorInBoundary(boundary, error); + affectedBoundaries.add(boundary); + } catch (nextError) { + // If it throws, propagate the error. + nextTrappedErrors = nextTrappedErrors || []; + nextTrappedErrors.push(trapError(boundary, nextError)); + } + }); + + // We will process an update caused by each error boundary synchronously. + affectedBoundaries.forEach(boundary => { + // FIXME: We only specify LowPriority here so that setState() calls from the error + // boundaries are respected. Instead we should set default priority level or something + // like this. Reconsider this piece when synchronous scheduling is in place. + const priority = LowPriority; + const root = scheduleErrorBoundaryWork(boundary, priority); + // This should use findNextUnitOfWork() when synchronous scheduling is implemented. + let fiber = cloneFiber(root.current, priority); + try { + while (fiber) { + // TODO: this is the only place where we recurse and it's unfortunate. + // (This may potentially get us into handleErrors() again.) + fiber = performUnitOfWork(fiber, true); + } + } catch (nextError) { + // If it throws, propagate the error. + nextTrappedErrors = nextTrappedErrors || []; + nextTrappedErrors.push(trapError(boundary, nextError)); + } + }); } - try { - // Give error boundary a chance to update its state - acknowledgeErrorInBoundary(boundary, error); + // Surface the first error uncaught by the boundaries to the user. + if (firstUncaughtError) { + // We need to make sure any future root can get scheduled despite these errors. + // Currently after throwing, nothing gets scheduled because these fields are set. + // FIXME: this is likely a wrong fix! It's still better than ignoring updates though. + nextScheduledRoot = null; + lastScheduledRoot = null; - // We will process an update caused by an error boundary with synchronous priority. - // This leaves us free to not keep track of whether a boundary has errored. - // If it errors again, we will just catch the error and synchronously propagate it higher. - - // First, traverse upwards and set pending synchronous priority on the whole tree. - let fiber = boundary; - while (fiber) { - fiber.pendingWorkPriority = SynchronousPriority; - if (fiber.alternate) { - fiber.alternate.pendingWorkPriority = SynchronousPriority; - } - if (!fiber.return) { - if (fiber.tag === HostContainer) { - // We found the root. - // Now go to the second phase and update it synchronously. - break; - } else { - throw new Error('Invalid root'); - } - } - fiber = fiber.return; - } - - if (!fiber) { - throw new Error('Could not find an error boundary root.'); - } - - // Find the work in progress tree. - const root : FiberRoot = (fiber.stateNode : any); - fiber = root.current.alternate; - - // Perform all the work synchronously. - while (fiber) { - fiber = performUnitOfWork(fiber, true); - } - } catch (nextError) { - // Propagate error to the next boundary or rethrow. - const nextTrappedError = trapError(boundary, nextError); - handleError(nextTrappedError); + // Throw any unhandled errors. + throw firstUncaughtError; } } From ed11b35302a408f4ac5a2efc1a8c023b8658bdc7 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Fri, 28 Oct 2016 21:48:37 +0100 Subject: [PATCH 5/5] The issue with tests is solved now --- .../stack/reconciler/__tests__/ReactErrorBoundaries-test.js | 3 --- 1 file changed, 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 658b76a1de..405d6567c1 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js @@ -36,9 +36,6 @@ describe('ReactErrorBoundaries', () => { var Normal; beforeEach(() => { - // TODO: Fiber isn't error resilient and one test can bring down them all. - jest.resetModuleRegistry(); - ReactDOM = require('ReactDOM'); React = require('React');