From 5a9dc236e0c2c462940a626ae428c119b03fd73a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 21 Dec 2017 00:38:42 -0800 Subject: [PATCH 1/6] Capture errors within the same render phase A rewrite of error handling, with semantics that more closely match stack unwinding. Errors that are thrown during the render phase unwind to the nearest error boundary, like before. But rather than synchronously unmount the children before retrying, we restart the failed subtree within the same render phase. The failed children are still unmounted (as if all their keys changed) but without an extra commit. Commit phase errors are different. They work by scheduling an error on the update queue of the error boundary. When we enter the render phase, the error is popped off the queue. The rest of the algorithm is the same. This approach is designed to work for throwing non-errors, too, though that feature is not implemented yet. Non-errors will be handled slightly differently: rather than unwind directly to the boundary, we'll continue rendering the siblings. Unwinding will work using the normal complete phase algorithm. --- .../__tests__/ReactErrorBoundaries-test.js | 170 +++-- .../src/__tests__/ReactUpdates-test.js | 23 +- .../react-reconciler/src/ReactChildFiber.js | 6 + packages/react-reconciler/src/ReactFiber.js | 5 - .../src/ReactFiberBeginWork.js | 216 ++++-- .../src/ReactFiberClassComponent.js | 35 +- .../src/ReactFiberCompleteWork.js | 29 +- .../src/ReactFiberReconciler.js | 2 + .../src/ReactFiberScheduler.js | 656 ++++++------------ .../src/ReactFiberUpdateQueue.js | 19 +- .../src/__tests__/ReactIncremental-test.js | 4 +- .../ReactIncrementalErrorHandling-test.js | 17 +- ...ctIncrementalErrorLogging-test.internal.js | 21 +- ...ReactIncrementalPerf-test.internal.js.snap | 19 +- .../src/__tests__/ReactTestRenderer-test.js | 2 +- 15 files changed, 567 insertions(+), 657 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js index 04cfc5525e6..7cda0fee506 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js @@ -625,13 +625,11 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Fiber mounts with null children before capturing error - 'ErrorBoundary componentDidMount', // Catch and render an error message 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -653,13 +651,11 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', 'BrokenConstructor constructor [!]', - // Fiber mounts with null children before capturing error - 'ErrorBoundary componentDidMount', // Catch and render an error message 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -682,11 +678,10 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render success', 'BrokenComponentWillMount constructor', 'BrokenComponentWillMount componentWillMount [!]', - 'ErrorBoundary componentDidMount', 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -765,7 +760,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - 'ErrorBoundary componentDidMount', 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', @@ -773,7 +767,7 @@ describe('ReactErrorBoundaries', () => { 'ErrorMessage componentWillMount', 'ErrorMessage render', 'ErrorMessage componentDidMount', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -805,22 +799,18 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // In Fiber, failed error boundaries render null before attempting to recover - 'RetryErrorBoundary componentDidMount', 'RetryErrorBoundary componentDidCatch [!]', - 'ErrorBoundary componentDidMount', // Retry 'RetryErrorBoundary render', 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', // This time, the error propagates to the higher boundary - 'RetryErrorBoundary componentWillUnmount', 'ErrorBoundary componentDidCatch', // Render the error 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -844,11 +834,10 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillMountErrorBoundary constructor', 'BrokenComponentWillMountErrorBoundary componentWillMount [!]', // The error propagates to the higher boundary - 'ErrorBoundary componentDidMount', 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -879,19 +868,15 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // The first error boundary catches the error // It adjusts state but throws displaying the message - // Finish mounting with null children - 'BrokenRenderErrorBoundary componentDidMount', // Attempt to handle the error 'BrokenRenderErrorBoundary componentDidCatch', - 'ErrorBoundary componentDidMount', 'BrokenRenderErrorBoundary render error [!]', // Boundary fails with new error, propagate to next boundary - 'BrokenRenderErrorBoundary componentWillUnmount', // Attempt to handle the error again 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -922,14 +907,12 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Finish mounting with null children - 'ErrorBoundary componentDidMount', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -961,16 +944,13 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Handle error: - // Finish mounting with null children - 'ErrorBoundary componentDidMount', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', 'Error message ref is set to [object HTMLDivElement]', - 'ErrorBoundary componentDidUpdate', + 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -1034,14 +1014,12 @@ describe('ReactErrorBoundaries', () => { 'Normal2 render', // BrokenConstructor will abort rendering: 'BrokenConstructor constructor [!]', - // Finish updating with null children - 'Normal componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'Normal componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1083,14 +1061,12 @@ describe('ReactErrorBoundaries', () => { // BrokenComponentWillMount will abort rendering: 'BrokenComponentWillMount constructor', 'BrokenComponentWillMount componentWillMount [!]', - // Finish updating with null children - 'Normal componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'Normal componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1127,14 +1103,12 @@ describe('ReactErrorBoundaries', () => { 'Normal render', // BrokenComponentWillReceiveProps will abort rendering: 'BrokenComponentWillReceiveProps componentWillReceiveProps [!]', - // Finish updating with null children - 'Normal componentWillUnmount', - 'BrokenComponentWillReceiveProps componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'Normal componentWillUnmount', + 'BrokenComponentWillReceiveProps componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1172,14 +1146,12 @@ describe('ReactErrorBoundaries', () => { // BrokenComponentWillUpdate will abort rendering: 'BrokenComponentWillUpdate componentWillReceiveProps', 'BrokenComponentWillUpdate componentWillUpdate [!]', - // Finish updating with null children - 'Normal componentWillUnmount', - 'BrokenComponentWillUpdate componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'Normal componentWillUnmount', + 'BrokenComponentWillUpdate componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1222,13 +1194,11 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Finish updating with null children - 'Normal componentWillUnmount', - 'ErrorBoundary componentDidUpdate', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'Normal componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1281,15 +1251,13 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Finish updating with null children - 'Child1 ref is set to null', - 'ErrorBoundary componentDidUpdate', - // Handle the error + // Capture the error 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'Error message ref is set to [object HTMLDivElement]', + 'Child1 ref is set to null', // Child2 ref is never set because its mounting aborted + 'Error message ref is set to [object HTMLDivElement]', 'ErrorBoundary componentDidUpdate', ]); @@ -1307,7 +1275,7 @@ describe('ReactErrorBoundaries', () => { - + Normal , container, ); @@ -1328,15 +1296,25 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillUnmount componentWillReceiveProps', 'BrokenComponentWillUnmount componentWillUpdate', 'BrokenComponentWillUnmount render', - // Unmounting throws: + // The first error boundary is unmounted, which throws: 'BrokenComponentWillUnmount componentWillUnmount [!]', // Fiber proceeds with lifecycles despite errors 'Normal componentWillUnmount', // The components have updated in this phase 'BrokenComponentWillUnmount componentDidUpdate', 'ErrorBoundary componentDidUpdate', - // Now that commit phase is done, Fiber unmounts the boundary's children + // Now that commit phase is done, we update the error boundaries + // Capture the error + 'ErrorBoundary componentDidCatch', + // The error handler schedules an update + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + + // The second, sibling error boundary will be unmounted. This + // triggers *another* error. 'BrokenComponentWillUnmount componentWillUnmount [!]', + 'ErrorBoundary componentDidUpdate', + // Capture the error 'ErrorBoundary componentDidCatch', // The initial render was aborted, so // Fiber retries from the root. @@ -1386,20 +1364,23 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillUnmount componentWillReceiveProps', 'BrokenComponentWillUnmount componentWillUpdate', 'BrokenComponentWillUnmount render', - // Unmounting throws: + // Unmount the innermost BrokenComponentWillUnmount, which throws. 'BrokenComponentWillUnmount componentWillUnmount [!]', // Fiber proceeds with lifecycles despite errors 'BrokenComponentWillUnmount componentDidUpdate', 'Normal componentDidUpdate', 'ErrorBoundary componentDidUpdate', + // Now that commit phase is done, capture the error: + 'ErrorBoundary componentDidCatch', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', 'Normal componentWillUnmount', + // Now the second error boundary is unmounted. 'BrokenComponentWillUnmount componentWillUnmount [!]', - // Now that commit phase is done, Fiber handles errors + 'ErrorBoundary componentDidUpdate', + // Capture the second error 'ErrorBoundary componentDidCatch', - // The initial render was aborted, so - // Fiber retries from the root. 'ErrorBoundary componentWillUpdate', - // Render an error now (stack will do it later) 'ErrorBoundary render error', // Done 'ErrorBoundary componentDidUpdate', @@ -1714,13 +1695,19 @@ describe('ReactErrorBoundaries', () => { 'LastChild componentDidMount', 'ErrorBoundary componentDidMount', // Now we are ready to handle the error - // Safely unmount every child + // Capture the error + 'ErrorBoundary componentDidCatch', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + // The second BrokenComponentWillUnmount is unmounted, triggering + // another error. 'BrokenComponentWillUnmount componentWillUnmount [!]', // Continue unmounting safely despite any errors 'Normal componentWillUnmount', 'BrokenComponentDidMount componentWillUnmount', 'LastChild componentWillUnmount', - // Handle the error + 'ErrorBoundary componentDidUpdate', + // Capture the second error 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', @@ -1759,11 +1746,11 @@ describe('ReactErrorBoundaries', () => { // All lifecycles run 'BrokenComponentDidUpdate componentDidUpdate [!]', 'ErrorBoundary componentDidUpdate', - 'BrokenComponentDidUpdate componentWillUnmount', // Then, error is handled 'ErrorBoundary componentDidCatch', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'BrokenComponentDidUpdate componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1795,12 +1782,12 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentDidMountErrorBoundary componentDidMount [!]', // Fiber proceeds with the hooks 'ErrorBoundary componentDidMount', - 'BrokenComponentDidMountErrorBoundary componentWillUnmount', // The error propagates to the higher boundary 'ErrorBoundary componentDidCatch', // Fiber retries from the root 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', + 'BrokenComponentDidMountErrorBoundary componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1887,14 +1874,14 @@ describe('ReactErrorBoundaries', () => { 'OuterErrorBoundary componentDidUpdate', // After the commit phase, attempt to recover from any errors that // were captured - 'BrokenComponentDidUpdate componentWillUnmount', - 'BrokenComponentDidUpdate componentWillUnmount', 'InnerUnmountBoundary componentDidCatch', - 'InnerUpdateBoundary componentDidCatch', 'InnerUnmountBoundary componentWillUpdate', 'InnerUnmountBoundary render error', + 'InnerUpdateBoundary componentDidCatch', 'InnerUpdateBoundary componentWillUpdate', 'InnerUpdateBoundary render error', + 'BrokenComponentDidUpdate componentWillUnmount', + 'BrokenComponentDidUpdate componentWillUnmount', 'InnerUnmountBoundary componentDidUpdate', 'InnerUpdateBoundary componentDidUpdate', ]); @@ -1933,34 +1920,41 @@ describe('ReactErrorBoundaries', () => { expect(err2.message).toMatch(/got: undefined/); }); - it('renders empty output if error boundary does not handle the error', () => { + it('propagates to next boundary if boundary does not handle the error', () => { const container = document.createElement('div'); - ReactDOM.render( -
- Sibling - - - -
, - container, - ); - expect(container.firstChild.textContent).toBe('Sibling'); + expect(() => + ReactDOM.render( +
+ Sibling + + + +
, + container, + ), + ).toThrow('Hello'); + expect(container.firstChild).toBe(null); expect(log).toEqual([ 'NoopErrorBoundary constructor', 'NoopErrorBoundary componentWillMount', 'NoopErrorBoundary render', 'BrokenRender constructor', 'BrokenRender componentWillMount', + // Render throws an error 'BrokenRender render [!]', - // In Fiber, noop error boundaries render null - 'NoopErrorBoundary componentDidMount', + // Capture the error and retry 'NoopErrorBoundary componentDidCatch', - // Nothing happens. + 'NoopErrorBoundary render', + // The same failure happens again + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // This time, the error propagates up to the root and is thrown ]); log.length = 0; ReactDOM.unmountComponentAtNode(container); - expect(log).toEqual(['NoopErrorBoundary componentWillUnmount']); + expect(log).toEqual([]); }); it('passes first error when two errors happen in commit', () => { diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index b0335506418..e5fe63b418b 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1362,32 +1362,27 @@ describe('ReactUpdates', () => { }); it('does not fall into an infinite error loop', () => { - function BadRender() { - throw new Error('error'); - } - - class ErrorBoundary extends React.Component { - componentDidCatch() { - this.props.parent.remount(); + class BadMount extends React.Component { + componentDidMount() { + throw new Error('error'); } render() { - return ; + return null; } } - class NonTerminating extends React.Component { - state = {step: 0}; - remount() { - this.setState(state => ({step: state.step + 1})); + class ErrorBoundary extends React.Component { + componentDidCatch() { + // Noop } render() { - return ; + return ; } } const container = document.createElement('div'); expect(() => { - ReactDOM.render(, container); + ReactDOM.render(, container); }).toThrow('Maximum'); }); }); diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 8342a028c23..c8bcf744692 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -1163,6 +1163,7 @@ function ChildReconciler(shouldTrackSideEffects) { returnFiber: Fiber, currentFirstChild: Fiber | null, newChild: any, + deleteExistingChildren: boolean, expirationTime: ExpirationTime, ): Fiber | null { // This function is not recursive. @@ -1182,6 +1183,11 @@ function ChildReconciler(shouldTrackSideEffects) { newChild = newChild.props.children; } + if (deleteExistingChildren) { + deleteRemainingChildren(returnFiber, currentFirstChild); + currentFirstChild = null; + } + // Handle object types const isObject = typeof newChild === 'object' && newChild !== null; diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 9d7c305d6fc..3d2a6c47311 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -270,11 +270,6 @@ export function createWorkInProgress( // We already have an alternate. // Reset the effect tag. workInProgress.effectTag = NoEffect; - - // The effect list is no longer valid. - workInProgress.nextEffect = null; - workInProgress.firstEffect = null; - workInProgress.lastEffect = null; } workInProgress.expirationTime = expirationTime; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 42f53a28a0a..f85956ea7ef 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -13,6 +13,7 @@ import type {HostContext} from './ReactFiberHostContext'; import type {HydrationContext} from './ReactFiberHydrationContext'; import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {CapturedError} from './ReactFiberScheduler'; import { IndeterminateComponent, @@ -31,7 +32,6 @@ import { PerformedWork, Placement, ContentReset, - Err, Ref, } from 'shared/ReactTypeOfSideEffect'; import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState'; @@ -58,6 +58,7 @@ import { invalidateContextProvider, } from './ReactFiberContext'; import {NoWork, Never} from './ReactFiberExpirationTime'; +import {logError} from './ReactFiberScheduler'; let warnedAboutStatelessRefs; @@ -71,6 +72,7 @@ export default function( hydrationContext: HydrationContext, scheduleWork: (fiber: Fiber, expirationTime: ExpirationTime) => void, computeExpirationForFiber: (fiber: Fiber) => ExpirationTime, + markUncaughtError: (error: mixed) => void, ) { const { shouldSetTextContent, @@ -105,6 +107,7 @@ export default function( current, workInProgress, nextChildren, + false, workInProgress.expirationTime, ); } @@ -113,6 +116,7 @@ export default function( current, workInProgress, nextChildren, + deleteExistingChildren, renderExpirationTime, ) { if (current === null) { @@ -124,6 +128,7 @@ export default function( workInProgress, null, nextChildren, + deleteExistingChildren, renderExpirationTime, ); } else { @@ -137,6 +142,7 @@ export default function( workInProgress, current.child, nextChildren, + deleteExistingChildren, renderExpirationTime, ); } @@ -204,16 +210,29 @@ export default function( function updateClassComponent( current: Fiber | null, workInProgress: Fiber, + capturedValues: Array | null, renderExpirationTime: ExpirationTime, ) { // Push context providers early to prevent context stack mismatches. // During mounting we don't know the child context yet as the instance doesn't exist. // We will invalidate the child context in finishClassComponent() right after rendering. const hasContext = pushContextProvider(workInProgress); + const instance = workInProgress.stateNode; + + let didCaptureError = false; + if (capturedValues !== null) { + didCaptureError = true; + invariant(instance !== null, 'Expected class to have an instance.'); + // TODO: Pattern matching. Check that this is an error. + const capturedError: CapturedError = (capturedValues[0]: any); + logError(workInProgress, capturedError); + const error = capturedError.error; + instance.componentDidCatch(error); + } let shouldUpdate; if (current === null) { - if (!workInProgress.stateNode) { + if (instance === null) { // In the initial pass we might need to construct the instance. constructClassInstance(workInProgress, workInProgress.pendingProps); mountClassInstance(workInProgress, renderExpirationTime); @@ -229,9 +248,20 @@ export default function( shouldUpdate = true; } else { - invariant(false, 'Resuming work not yet implemented.'); - // In a resume, we'll already have an instance we can reuse. - // shouldUpdate = resumeMountClassInstance(workInProgress, renderExpirationTime); + if (capturedValues !== null) { + // A capture is more like an update than another mount, + // because we haven't received new props from the parent. + shouldUpdate = true; + updateClassInstance(current, workInProgress, renderExpirationTime); + } else { + // In a resume, we'll already have an instance we can reuse. + // shouldUpdate = resumeMountClassInstance(workInProgress, renderExpirationTime); + invariant( + capturedValues !== null, + 'Resuming work not yet implemented.', + ); + shouldUpdate = true; + } } } else { shouldUpdate = updateClassInstance( @@ -240,11 +270,25 @@ export default function( renderExpirationTime, ); } + + const updateQueue = workInProgress.updateQueue; + if (updateQueue !== null && updateQueue.capturedValues !== null) { + // We already called componentDidCatch inside updateClassInstance. + // We're checking here again so we can grab the values off the + // update queue. + // TODO: Refactor class components. + capturedValues = updateQueue.capturedValues; + updateQueue.capturedValues = null; + shouldUpdate = true; + didCaptureError = true; + } return finishClassComponent( current, workInProgress, shouldUpdate, hasContext, + didCaptureError, + renderExpirationTime, ); } @@ -253,11 +297,13 @@ export default function( workInProgress: Fiber, shouldUpdate: boolean, hasContext: boolean, + didCaptureError: boolean, + renderExpirationTime: ExpirationTime, ) { // Refs should update even if shouldComponentUpdate returns false markRef(current, workInProgress); - if (!shouldUpdate) { + if (!shouldUpdate && !didCaptureError) { // Context providers should defer to sCU for rendering if (hasContext) { invalidateContextProvider(workInProgress, false); @@ -286,7 +332,13 @@ export default function( } // React DevTools reads this flag. workInProgress.effectTag |= PerformedWork; - reconcileChildren(current, workInProgress, nextChildren); + reconcileChildrenAtExpirationTime( + current, + workInProgress, + nextChildren, + didCaptureError, + renderExpirationTime, + ); // Memoize props and state using the values we just used to render. // TODO: Restructure so we never read values from the instance. memoizeState(workInProgress, instance.state); @@ -315,9 +367,47 @@ export default function( pushHostContainer(workInProgress, root.containerInfo); } - function updateHostRoot(current, workInProgress, renderExpirationTime) { + function unmountFailedRoot( + current, + workInProgress, + capturedValues, + renderExpirationTime, + ) { + // TODO: Pattern matching. Check that this is an error. + const didError = true; + const capturedError: CapturedError = (capturedValues[0]: any); + const error = capturedError.error; + markUncaughtError(error); + reconcileChildrenAtExpirationTime( + current, + workInProgress, + null, + didError, + renderExpirationTime, + ); + return null; + } + + function updateHostRoot( + current, + workInProgress, + capturedValues, + renderExpirationTime, + ) { pushHostRootContext(workInProgress); - const updateQueue = workInProgress.updateQueue; + if (capturedValues !== null) { + // TODO: Pattern matching. Check that this is an error. + const capturedError: CapturedError = (capturedValues[0]: any); + logError(workInProgress, capturedError); + return unmountFailedRoot( + current, + workInProgress, + capturedValues, + renderExpirationTime, + ); + } + + let updateQueue = workInProgress.updateQueue; if (updateQueue !== null) { const prevState = workInProgress.memoizedState; const state = processUpdateQueue( @@ -328,6 +418,21 @@ export default function( null, renderExpirationTime, ); + updateQueue = workInProgress.updateQueue; + if (updateQueue !== null && updateQueue.capturedValues !== null) { + capturedValues = updateQueue.capturedValues; + updateQueue.capturedValues = null; + // TODO: Pattern matching. Check that this is an error. + const capturedError: CapturedError = (capturedValues[0]: any); + logError(workInProgress, capturedError); + memoizeState(workInProgress, state); + return unmountFailedRoot( + current, + workInProgress, + capturedValues, + renderExpirationTime, + ); + } if (prevState === state) { // If the state is the same as before, that's a bailout because we had // no work that expires at this time. @@ -359,6 +464,7 @@ export default function( workInProgress, null, element, + false, renderExpirationTime, ); } else { @@ -489,7 +595,14 @@ export default function( const hasContext = pushContextProvider(workInProgress); adoptClassInstance(workInProgress, value); mountClassInstance(workInProgress, renderExpirationTime); - return finishClassComponent(current, workInProgress, true, hasContext); + return finishClassComponent( + current, + workInProgress, + true, + hasContext, + false, + renderExpirationTime, + ); } else { // Proceed under the assumption that this is a functional component workInProgress.tag = FunctionalComponent; @@ -554,6 +667,7 @@ export default function( workInProgress, workInProgress.stateNode, nextChildren, + false, renderExpirationTime, ); } else { @@ -561,6 +675,7 @@ export default function( workInProgress, workInProgress.stateNode, nextChildren, + false, renderExpirationTime, ); } @@ -595,6 +710,7 @@ export default function( workInProgress, null, nextChildren, + false, renderExpirationTime, ); memoizeProps(workInProgress, nextChildren); @@ -686,8 +802,14 @@ export default function( function beginWork( current: Fiber | null, workInProgress: Fiber, + capturedValues: Array | null, renderExpirationTime: ExpirationTime, ): Fiber | null { + // The effect list is no longer valid. + workInProgress.nextEffect = null; + workInProgress.firstEffect = null; + workInProgress.lastEffect = null; + if ( workInProgress.expirationTime === NoWork || workInProgress.expirationTime > renderExpirationTime @@ -708,10 +830,16 @@ export default function( return updateClassComponent( current, workInProgress, + capturedValues, renderExpirationTime, ); case HostRoot: - return updateHostRoot(current, workInProgress, renderExpirationTime); + return updateHostRoot( + current, + workInProgress, + capturedValues, + renderExpirationTime, + ); case HostComponent: return updateHostComponent( current, @@ -751,73 +879,7 @@ export default function( } } - function beginFailedWork( - current: Fiber | null, - workInProgress: Fiber, - renderExpirationTime: ExpirationTime, - ) { - // Push context providers here to avoid a push/pop context mismatch. - switch (workInProgress.tag) { - case ClassComponent: - pushContextProvider(workInProgress); - break; - case HostRoot: - pushHostRootContext(workInProgress); - break; - default: - invariant( - false, - 'Invalid type of work. This error is likely caused by a bug in React. ' + - 'Please file an issue.', - ); - } - - // Add an error effect so we can handle the error during the commit phase - workInProgress.effectTag |= Err; - - // This is a weird case where we do "resume" work — work that failed on - // our first attempt. Because we no longer have a notion of "progressed - // deletions," reset the child to the current child to make sure we delete - // it again. TODO: Find a better way to handle this, perhaps during a more - // general overhaul of error handling. - if (current === null) { - workInProgress.child = null; - } else if (workInProgress.child !== current.child) { - workInProgress.child = current.child; - } - - if ( - workInProgress.expirationTime === NoWork || - workInProgress.expirationTime > renderExpirationTime - ) { - return bailoutOnLowPriority(current, workInProgress); - } - - // If we don't bail out, we're going be recomputing our children so we need - // to drop our effect list. - workInProgress.firstEffect = null; - workInProgress.lastEffect = null; - - // Unmount the current children as if the component rendered null - const nextChildren = null; - reconcileChildrenAtExpirationTime( - current, - workInProgress, - nextChildren, - renderExpirationTime, - ); - - if (workInProgress.tag === ClassComponent) { - const instance = workInProgress.stateNode; - workInProgress.memoizedProps = instance.props; - workInProgress.memoizedState = instance.state; - } - - return workInProgress.child; - } - return { beginWork, - beginFailedWork, }; } diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 75ddf381699..6f11adcf077 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -9,6 +9,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {CapturedError} from './ReactFiberScheduler'; import {Update} from 'shared/ReactTypeOfSideEffect'; import { @@ -36,6 +37,7 @@ import { processUpdateQueue, } from './ReactFiberUpdateQueue'; import {hasContextChanged} from './ReactFiberContext'; +import {logError} from './ReactFiberScheduler'; const fakeInternalInstance = {}; const isArray = Array.isArray; @@ -100,7 +102,8 @@ export default function( callback, isReplace: false, isForced: false, - nextCallback: null, + isCapture: false, + capturedValue: null, next: null, }; insertUpdateIntoFiber(fiber, update); @@ -119,7 +122,8 @@ export default function( callback, isReplace: true, isForced: false, - nextCallback: null, + isCapture: false, + capturedValue: null, next: null, }; insertUpdateIntoFiber(fiber, update); @@ -138,7 +142,8 @@ export default function( callback, isReplace: false, isForced: true, - nextCallback: null, + isCapture: false, + capturedValue: null, next: null, }; insertUpdateIntoFiber(fiber, update); @@ -635,6 +640,30 @@ export default function( newProps, renderExpirationTime, ); + + let updateQueue = workInProgress.updateQueue; + if (updateQueue !== null && updateQueue.capturedValues !== null) { + const capturedValues = updateQueue.capturedValues; + + // Don't remove these from the update queue yet. We need them in + // finishClassComponent. Do the reset there. + // TODO: This is awkward. Refactor class components. + // updateQueue.capturedValues = null; + + // TODO: Pattern matching. Check that this is an error. + const capturedError: CapturedError = (capturedValues[0]: any); + logError(workInProgress, capturedError); + const error = capturedError.error; + instance.componentDidCatch(error); + newState = processUpdateQueue( + current, + workInProgress, + updateQueue, + instance, + newProps, + renderExpirationTime, + ); + } } else { newState = oldState; } diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 40bb9ea4f46..09f10af347a 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -32,7 +32,13 @@ import { ReturnComponent, Fragment, } from 'shared/ReactTypeOfWork'; -import {Placement, Ref, Update} from 'shared/ReactTypeOfSideEffect'; +import { + NoEffect, + Placement, + Ref, + Update, + Err, +} from 'shared/ReactTypeOfSideEffect'; import invariant from 'fbjs/lib/invariant'; import {reconcileChildFibers} from './ReactChildFiber'; @@ -45,6 +51,7 @@ export default function( config: HostConfig, hostContext: HostContext, hydrationContext: HydrationContext, + captureThrownValues: () => boolean, ) { const { createInstance, @@ -143,6 +150,7 @@ export default function( workInProgress, currentFirstChild, nextChildren, + false, renderExpirationTime, ); return workInProgress.child; @@ -395,11 +403,29 @@ export default function( case FunctionalComponent: return null; case ClassComponent: { + const instance = workInProgress.stateNode; + if ( + (workInProgress.effectTag & Err) === NoEffect && + instance !== null && + typeof instance.componentDidCatch === 'function' + ) { + const didCapture = captureThrownValues(); + if (didCapture) { + workInProgress.effectTag |= Err; + return workInProgress; + } + } + // We are leaving this subtree, so pop context if any. popContextProvider(workInProgress); return null; } case HostRoot: { + const didCapture = captureThrownValues(); + if (didCapture) { + return workInProgress; + } + popHostContainer(workInProgress); popTopLevelContextObject(workInProgress); const fiberRoot = (workInProgress.stateNode: FiberRoot); @@ -407,7 +433,6 @@ export default function( fiberRoot.context = fiberRoot.pendingContext; fiberRoot.pendingContext = null; } - if (current === null || current.child === null) { // If we hydrated, pop so that we can delete any remaining children // that weren't hydrated. diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index ef1eb8cf008..e80c1cdb968 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -344,6 +344,8 @@ export default function( callback, isReplace: false, isForced: false, + isCapture: false, + capturedValue: null, next: null, }; insertUpdateIntoFiber(current, update); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f8c499654c7..c8a2be0f316 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -76,7 +76,10 @@ import { computeExpirationBucket, } from './ReactFiberExpirationTime'; import {AsyncUpdates} from './ReactTypeOfInternalContext'; -import {getUpdateExpirationTime} from './ReactFiberUpdateQueue'; +import { + getUpdateExpirationTime, + insertUpdateIntoFiber, +} from './ReactFiberUpdateQueue'; import {resetContext} from './ReactFiberContext'; const { @@ -154,6 +157,27 @@ if (__DEV__) { }; } +export function logError(boundaryFiber: Fiber, capturedError: CapturedError) { + const errorBoundaryFound = boundaryFiber.tag === ClassComponent; + capturedError.errorBoundary = boundaryFiber; + capturedError.errorBoundaryFound = errorBoundaryFound; + capturedError.errorBoundaryName = errorBoundaryFound + ? getComponentName(boundaryFiber) + : null; + // TODO: These are always the same. Why is it needed? + capturedError.willRetry = errorBoundaryFound; + try { + logCapturedError(capturedError); + } catch (e) { + // Prevent cycle if logCapturedError() throws. + // A cycle may still occur if logCapturedError renders a component that throws. + const suppressLogging = e && e.suppressReactErrorLogging; + if (!suppressLogging) { + console.error(e); + } + } +} + export default function( config: HostConfig, ) { @@ -162,17 +186,19 @@ export default function( config, ); const {popHostContainer, popHostContext, resetHostContainer} = hostContext; - const {beginWork, beginFailedWork} = ReactFiberBeginWork( + const {beginWork} = ReactFiberBeginWork( config, hostContext, hydrationContext, scheduleWork, computeExpirationForFiber, + markUncaughtError, ); const {completeWork} = ReactFiberCompleteWork( config, hostContext, hydrationContext, + captureThrownValues, ); const { commitResetTextContent, @@ -182,7 +208,7 @@ export default function( commitLifeCycles, commitAttachRef, commitDetachRef, - } = ReactFiberCommitWork(config, captureError); + } = ReactFiberCommitWork(config, onCommitPhaseError); const { now, scheduleDeferredCallback, @@ -215,21 +241,13 @@ export default function( // The next fiber with an effect that we're currently committing. let nextEffect: Fiber | null = null; - // Keep track of which fibers have captured an error that need to be handled. - // Work is removed from this collection after componentDidCatch is called. - let capturedErrors: Map | null = null; - // Keep track of which fibers have failed during the current batch of work. - // This is a different set than capturedErrors, because it is not reset until - // the end of the batch. This is needed to propagate errors correctly if a - // subtree fails more than once. - let failedBoundaries: Set | null = null; - // Error boundaries that captured an error during the current commit. - let commitPhaseBoundaries: Set | null = null; + let hasUncaughtError: boolean = false; let firstUncaughtError: mixed | null = null; - let didFatal: boolean = false; let isCommitting: boolean = false; - let isUnmounting: boolean = false; + + let thrownValues: Array | null = null; + let capturedValues: Array | null = null; // Used for performance tracking. let interruptedBy: Fiber | null = null; @@ -240,6 +258,9 @@ export default function( // Reset the cursors resetContext(); resetHostContainer(); + + thrownValues = null; + capturedValues = null; } function commitAllHostEffects() { @@ -296,9 +317,7 @@ export default function( break; } case Deletion: { - isUnmounting = true; commitDeletion(nextEffect); - isUnmounting = false; break; } } @@ -325,11 +344,6 @@ export default function( commitAttachRef(nextEffect); } - if (effectTag & Err) { - recordEffect(); - commitErrorHandling(nextEffect); - } - const next = nextEffect.nextEffect; // Ensure that we clean these up so that we don't accidentally keep them. // I'm not actually sure this matters because we can't reset firstEffect @@ -343,10 +357,6 @@ export default function( } function commitRoot(finishedWork: Fiber): ExpirationTime { - // We keep track of this so that captureError can collect any boundaries - // that capture an error during the commit phase. The reason these aren't - // local to this function is because errors that occur during cWU are - // captured elsewhere, to prevent the unmount from being interrupted. isWorking = true; isCommitting = true; startCommitTimer(); @@ -410,7 +420,7 @@ export default function( 'Should have next effect. This error is likely caused by a bug ' + 'in React. Please file an issue.', ); - captureError(nextEffect, error); + onCommitPhaseError(nextEffect, error); // Clean-up if (nextEffect !== null) { nextEffect = nextEffect.nextEffect; @@ -456,7 +466,7 @@ export default function( 'Should have next effect. This error is likely caused by a bug ' + 'in React. Please file an issue.', ); - captureError(nextEffect, error); + onCommitPhaseError(nextEffect, error); if (nextEffect !== null) { nextEffect = nextEffect.nextEffect; } @@ -474,26 +484,7 @@ export default function( ReactFiberInstrumentation.debugTool.onCommitWork(finishedWork); } - // If we caught any errors during this commit, schedule their boundaries - // to update. - if (commitPhaseBoundaries) { - commitPhaseBoundaries.forEach(scheduleErrorRecovery); - commitPhaseBoundaries = null; - } - - if (firstUncaughtError !== null) { - const error = firstUncaughtError; - firstUncaughtError = null; - onUncaughtError(error); - } - const remainingTime = root.current.expirationTime; - - if (remainingTime === NoWork) { - capturedErrors = null; - failedBoundaries = null; - } - return remainingTime; } @@ -549,7 +540,18 @@ export default function( const returnFiber = workInProgress.return; const siblingFiber = workInProgress.sibling; - resetExpirationTime(workInProgress, nextRenderExpirationTime); + if (next !== workInProgress) { + if (workInProgress.effectTag & Err) { + // Restarting an error boundary + stopFailedWorkTimer(workInProgress); + } else { + stopWorkTimer(workInProgress); + } + resetExpirationTime(workInProgress, nextRenderExpirationTime); + } else { + // This fiber did not complete. + stopWorkTimer(workInProgress); + } if (next !== null) { stopWorkTimer(workInProgress); @@ -594,7 +596,6 @@ export default function( } } - stopWorkTimer(workInProgress); if (__DEV__ && ReactFiberInstrumentation.debugTool) { ReactFiberInstrumentation.debugTool.onCompleteWork(workInProgress); } @@ -633,41 +634,13 @@ export default function( ReactDebugCurrentFiber.setCurrentFiber(workInProgress); } - let next = beginWork(current, workInProgress, nextRenderExpirationTime); - if (__DEV__) { - ReactDebugCurrentFiber.resetCurrentFiber(); - } - if (__DEV__ && ReactFiberInstrumentation.debugTool) { - ReactFiberInstrumentation.debugTool.onBeginWork(workInProgress); - } - - if (next === null) { - // If this doesn't spawn new work, complete the current work. - next = completeUnitOfWork(workInProgress); - } - - ReactCurrentOwner.current = null; - - return next; - } - - function performFailedUnitOfWork(workInProgress: Fiber): Fiber | null { - // The current, flushed, state of this fiber is the alternate. - // Ideally nothing should rely on this, but relying on it here - // means that we don't need an additional field on the work in - // progress. - const current = workInProgress.alternate; - - // See if beginning this work spawns more work. - startWorkTimer(workInProgress); - if (__DEV__) { - ReactDebugCurrentFiber.setCurrentFiber(workInProgress); - } - let next = beginFailedWork( + let next = beginWork( current, workInProgress, + capturedValues, nextRenderExpirationTime, ); + capturedValues = null; if (__DEV__) { ReactDebugCurrentFiber.resetCurrentFiber(); } @@ -686,14 +659,6 @@ export default function( } function workLoop(expirationTime: ExpirationTime) { - if (capturedErrors !== null) { - // If there are unhandled errors, switch to the slow work loop. - // TODO: How to avoid this check in the fast path? Maybe the renderer - // could keep track of which roots have unhandled errors and call a - // forked version of renderRoot. - slowWorkLoopThatChecksForFailedWork(expirationTime); - return; - } if ( nextRenderExpirationTime === NoWork || nextRenderExpirationTime > expirationTime @@ -714,57 +679,53 @@ export default function( } } - function slowWorkLoopThatChecksForFailedWork(expirationTime: ExpirationTime) { - if ( - nextRenderExpirationTime === NoWork || - nextRenderExpirationTime > expirationTime - ) { - return; + function unwindToNearestBoundary(sourceFiber: Fiber, thrownValue: mixed) { + // Push the thrown value onto the global list. + + // TODO: Pattern matching. Check that this is an error. + const capturedError: CapturedError = { + componentName: getComponentName(sourceFiber), + componentStack: getStackAddendumByWorkInProgressFiber(sourceFiber), + error: thrownValue, + errorBoundary: null, + errorBoundaryFound: false, + errorBoundaryName: null, + willRetry: false, + }; + if (thrownValues === null) { + thrownValues = [capturedError]; + } else { + thrownValues.push(capturedError); } - if (nextRenderExpirationTime <= mostRecentCurrentTime) { - // Flush all expired work. - while (nextUnitOfWork !== null) { - if (hasCapturedError(nextUnitOfWork)) { - // Use a forked version of performUnitOfWork - nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork); - } else { - nextUnitOfWork = performUnitOfWork(nextUnitOfWork); - } - } - } else { - // Flush asynchronous work until the deadline runs out of time. - while (nextUnitOfWork !== null && !shouldYield()) { - if (hasCapturedError(nextUnitOfWork)) { - // Use a forked version of performUnitOfWork - nextUnitOfWork = performFailedUnitOfWork(nextUnitOfWork); - } else { - nextUnitOfWork = performUnitOfWork(nextUnitOfWork); - } + popContexts(sourceFiber); + + // TODO: If the value is something other than an error (like a + // promise), continue working on the siblings, and unwind using + // the normal complete phase algorithm. This should be safe, because + // we assume the value was thrown from inside the render method. For + // errors, we have to skip the siblings and immediately unwind to + // the nearest boundary, because the tree may be in a corrupt state. + let boundaryFiber = null; + let fiber = sourceFiber.return; + traversal: while (fiber !== null) { + switch (fiber.tag) { + case ClassComponent: + const instance = fiber.stateNode; + if (typeof instance.componentDidCatch === 'function') { + boundaryFiber = fiber; + break traversal; + } + break; + case HostRoot: + boundaryFiber = fiber; + break; } + popContexts(fiber); + fiber = fiber.return; } - } - function renderRootCatchBlock( - root: FiberRoot, - failedWork: Fiber, - boundary: Fiber, - expirationTime: ExpirationTime, - ) { - // We're going to restart the error boundary that captured the error. - // Conceptually, we're unwinding the stack. We need to unwind the - // context stack, too. - unwindContexts(failedWork, boundary); - - // Restart the error boundary using a forked version of - // performUnitOfWork that deletes the boundary's children. The entire - // failed subree will be unmounted. During the commit phase, a special - // lifecycle method is called on the error boundary, which triggers - // a re-render. - nextUnitOfWork = performFailedUnitOfWork(boundary); - - // Continue working. - workLoop(expirationTime); + return boundaryFiber; } function renderRoot( @@ -802,339 +763,168 @@ export default function( startWorkLoopTimer(nextUnitOfWork); - let didError = false; - let error = null; - if (__DEV__) { - invokeGuardedCallback(null, workLoop, null, expirationTime); - if (hasCaughtError()) { - didError = true; - error = clearCaughtError(); - } - } else { - try { - workLoop(expirationTime); - } catch (e) { - didError = true; - error = e; - } - } - - // An error was thrown during the render phase. - while (didError) { - if (didFatal) { - // This was a fatal error. Don't attempt to recover from it. - firstUncaughtError = error; - break; - } + let didThrow; + let thrownValue; + do { + didThrow = false; + thrownValue = null; - const failedWork = nextUnitOfWork; - if (failedWork === null) { - // An error was thrown but there's no current unit of work. This can - // happen during the commit phase if there's a bug in the renderer. - didFatal = true; - continue; - } - - // "Capture" the error by finding the nearest boundary. If there is no - // error boundary, we use the root. - const boundary = captureError(failedWork, error); - invariant( - boundary !== null, - 'Should have found an error boundary. This error is likely ' + - 'caused by a bug in React. Please file an issue.', - ); - - if (didFatal) { - // The error we just captured was a fatal error. This happens - // when the error propagates to the root more than once. - continue; - } - - didError = false; - error = null; if (__DEV__) { - invokeGuardedCallback( - null, - renderRootCatchBlock, - null, - root, - failedWork, - boundary, - expirationTime, - ); + invokeGuardedCallback(null, workLoop, null, expirationTime); if (hasCaughtError()) { - didError = true; - error = clearCaughtError(); - continue; + didThrow = true; + thrownValue = clearCaughtError(); } } else { try { - renderRootCatchBlock(root, failedWork, boundary, expirationTime); - error = null; + workLoop(expirationTime); } catch (e) { - didError = true; - error = e; - continue; + didThrow = true; + thrownValue = e; } } - // We're finished working. Exit the error loop. - break; - } - const uncaughtError = firstUncaughtError; + if (!didThrow) { + break; + } + + if (nextUnitOfWork === null) { + // Should have a nextUnitOfWork here. + hasUncaughtError = true; + firstUncaughtError = thrownValue; + break; + } + + const boundary = unwindToNearestBoundary(nextUnitOfWork, thrownValue); + if (boundary !== null) { + nextUnitOfWork = completeUnitOfWork(boundary); + } else { + // The root failed to render. This is a fatal error. + hasUncaughtError = true; + firstUncaughtError = thrownValue; + break; + } + } while (true); + + if (hasUncaughtError) { + onUncaughtError(firstUncaughtError); + hasUncaughtError = false; + firstUncaughtError = null; + // Set this to null to indicate there's no more work left. + // That way the stack is reset next time we work on this root. + nextUnitOfWork = null; + } // We're done performing work. Time to clean up. stopWorkLoopTimer(interruptedBy); interruptedBy = null; isWorking = false; - didFatal = false; - firstUncaughtError = null; - - if (uncaughtError !== null) { - onUncaughtError(uncaughtError); - } return root.isReadyForCommit ? root.current.alternate : null; } - // Returns the boundary that captured the error, or null if the error is ignored - function captureError(failedWork: Fiber, error: mixed): Fiber | null { - // It is no longer valid because we exited the user code. - ReactCurrentOwner.current = null; - if (__DEV__) { - ReactDebugCurrentFiber.resetCurrentFiber(); + function popContexts(workInProgress: Fiber) { + switch (workInProgress.tag) { + case ClassComponent: + popContextProvider(workInProgress); + break; + case HostComponent: + popHostContext(workInProgress); + break; + case HostRoot: + popHostContainer(workInProgress); + break; + case HostPortal: + popHostContainer(workInProgress); + break; } + stopWorkTimer(workInProgress); + } - // Search for the nearest error boundary. - let boundary: Fiber | null = null; - - // Passed to logCapturedError() - let errorBoundaryFound: boolean = false; - let willRetry: boolean = false; - let errorBoundaryName: string | null = null; - - // Host containers are a special case. If the failed work itself is a host - // container, then it acts as its own boundary. In all other cases, we - // ignore the work itself and only search through the parents. - if (failedWork.tag === HostRoot) { - boundary = failedWork; - - if (isFailedBoundary(failedWork)) { - // If this root already failed, there must have been an error when - // attempting to unmount it. This is a worst-case scenario and - // should only be possible if there's a bug in the renderer. - didFatal = true; - } - } else { - let node = failedWork.return; - while (node !== null && boundary === null) { - if (node.tag === ClassComponent) { - const instance = node.stateNode; - if (typeof instance.componentDidCatch === 'function') { - errorBoundaryFound = true; - errorBoundaryName = getComponentName(node); - - // Found an error boundary! - boundary = node; - willRetry = true; - } - } else if (node.tag === HostRoot) { - // Treat the root like a no-op error boundary - boundary = node; - } - - if (isFailedBoundary(node)) { - // This boundary is already in a failed state. - - // If we're currently unmounting, that means this error was - // thrown while unmounting a failed subtree. We should ignore - // the error. - if (isUnmounting) { - return null; - } - - // If we're in the commit phase, we should check to see if - // this boundary already captured an error during this commit. - // This case exists because multiple errors can be thrown during - // a single commit without interruption. - if ( - commitPhaseBoundaries !== null && - (commitPhaseBoundaries.has(node) || - (node.alternate !== null && - commitPhaseBoundaries.has(node.alternate))) - ) { - // If so, we should ignore this error. - return null; - } - - // The error should propagate to the next boundary -— we keep looking. - boundary = null; - willRetry = false; - } - - node = node.return; - } + // Called during complete phase + function captureThrownValues(): boolean { + if (thrownValues === null) { + return false; } - if (boundary !== null) { - // Add to the collection of failed boundaries. This lets us know that - // subsequent errors in this subtree should propagate to the next boundary. - if (failedBoundaries === null) { - failedBoundaries = new Set(); - } - failedBoundaries.add(boundary); - - // This method is unsafe outside of the begin and complete phases. - // We might be in the commit phase when an error is captured. - // The risk is that the return path from this Fiber may not be accurate. - // That risk is acceptable given the benefit of providing users more context. - const componentStack = getStackAddendumByWorkInProgressFiber(failedWork); - const componentName = getComponentName(failedWork); - - // Add to the collection of captured errors. This is stored as a global - // map of errors and their component stack location keyed by the boundaries - // that capture them. We mostly use this Map as a Set; it's a Map only to - // avoid adding a field to Fiber to store the error. - if (capturedErrors === null) { - capturedErrors = new Map(); - } - - const capturedError = { - componentName, - componentStack, - error, - errorBoundary: errorBoundaryFound ? boundary.stateNode : null, - errorBoundaryFound, - errorBoundaryName, - willRetry, - }; - - capturedErrors.set(boundary, capturedError); - - try { - logCapturedError(capturedError); - } catch (e) { - // Prevent cycle if logCapturedError() throws. - // A cycle may still occur if logCapturedError renders a component that throws. - const suppressLogging = e && e.suppressReactErrorLogging; - if (!suppressLogging) { - console.error(e); - } - } - - // If we're in the commit phase, defer scheduling an update on the - // boundary until after the commit is complete - if (isCommitting) { - if (commitPhaseBoundaries === null) { - commitPhaseBoundaries = new Set(); - } - commitPhaseBoundaries.add(boundary); - } else { - // Otherwise, schedule an update now. - // TODO: Is this actually necessary during the render phase? Is it - // possible to unwind and continue rendering at the same priority, - // without corrupting internal state? - scheduleErrorRecovery(boundary); - } - return boundary; - } else if (firstUncaughtError === null) { - // If no boundary is found, we'll need to throw the error - firstUncaughtError = error; - } - return null; + capturedValues = thrownValues; + // Clear the values from the scheduler. + thrownValues = null; + return true; } - function hasCapturedError(fiber: Fiber): boolean { - // TODO: capturedErrors should store the boundary instance, to avoid needing - // to check the alternate. - return ( - capturedErrors !== null && - (capturedErrors.has(fiber) || - (fiber.alternate !== null && capturedErrors.has(fiber.alternate))) - ); + function markUncaughtError(error: mixed): void { + hasUncaughtError = true; + firstUncaughtError = error; } - function isFailedBoundary(fiber: Fiber): boolean { - // TODO: failedBoundaries should store the boundary instance, to avoid - // needing to check the alternate. - return ( - failedBoundaries !== null && - (failedBoundaries.has(fiber) || - (fiber.alternate !== null && failedBoundaries.has(fiber.alternate))) - ); + function scheduleCapture( + sourceFiber, + boundaryFiber, + capturedValue, + expirationTime, + ) { + // TODO: Pattern matching. Check that this is an error. + const capturedError: CapturedError = { + componentName: getComponentName(sourceFiber), + componentStack: getStackAddendumByWorkInProgressFiber(sourceFiber), + error: capturedValue, + // Set these once the boundary captures, during the render phase. + errorBoundary: null, + errorBoundaryFound: false, + errorBoundaryName: null, + willRetry: false, + }; + + const update = { + expirationTime, + partialState: null, + callback: null, + isReplace: false, + isForced: false, + isCapture: true, + capturedValue: capturedError, + next: null, + }; + insertUpdateIntoFiber(boundaryFiber, update); + scheduleWork(boundaryFiber, expirationTime); } - function commitErrorHandling(effectfulFiber: Fiber) { - let capturedError; - if (capturedErrors !== null) { - capturedError = capturedErrors.get(effectfulFiber); - capturedErrors.delete(effectfulFiber); - if (capturedError == null) { - if (effectfulFiber.alternate !== null) { - effectfulFiber = effectfulFiber.alternate; - capturedError = capturedErrors.get(effectfulFiber); - capturedErrors.delete(effectfulFiber); - } - } - } - + function dispatch( + sourceFiber: Fiber, + value: mixed, + expirationTime: ExpirationTime, + ) { invariant( - capturedError != null, - 'No error for given unit of work. This error is likely caused by a ' + - 'bug in React. Please file an issue.', + !isWorking || isCommitting, + 'dispatch: Cannot dispatch during the render phase.', ); - - switch (effectfulFiber.tag) { - case ClassComponent: - const instance = effectfulFiber.stateNode; - - const info: HandleErrorInfo = { - componentStack: capturedError.componentStack, - }; - - // Allow the boundary to handle the error, usually by scheduling - // an update to itself - instance.componentDidCatch(capturedError.error, info); - return; - case HostRoot: - if (firstUncaughtError === null) { - firstUncaughtError = capturedError.error; - } - return; - default: - invariant( - false, - 'Invalid type of work. This error is likely caused by a bug in ' + - 'React. Please file an issue.', - ); - } - } - - function unwindContexts(from: Fiber, to: Fiber) { - let node = from; - while (node !== null) { - switch (node.tag) { + let fiber = sourceFiber.return; + while (fiber !== null) { + switch (fiber.tag) { case ClassComponent: - popContextProvider(node); - break; - case HostComponent: - popHostContext(node); + const instance = fiber.stateNode; + if (typeof instance.componentDidCatch === 'function') { + scheduleCapture(sourceFiber, fiber, value, expirationTime); + return; + } break; case HostRoot: - popHostContainer(node); - break; - case HostPortal: - popHostContainer(node); - break; - } - if (node === to || node.alternate === to) { - stopFailedWorkTimer(node); - break; - } else { - stopWorkTimer(node); + scheduleCapture(sourceFiber, fiber, value, expirationTime); + return; } - node = node.return; + fiber = fiber.return; } + + if (sourceFiber.tag === HostRoot) { + // Error was thrown at the root. There is no parent, so the root + // itself should capture it. + scheduleCapture(sourceFiber, sourceFiber, value, expirationTime); + } + } + + function onCommitPhaseError(fiber: Fiber, error: mixed) { + return dispatch(fiber, error, Sync); } function computeAsyncExpiration() { @@ -1266,10 +1056,6 @@ export default function( } } - function scheduleErrorRecovery(fiber: Fiber) { - scheduleWorkImpl(fiber, Sync, true); - } - function recalculateCurrentTime(): ExpirationTime { // Subtract initial time so it fits inside 32bits const ms = now() - startTime; diff --git a/packages/react-reconciler/src/ReactFiberUpdateQueue.js b/packages/react-reconciler/src/ReactFiberUpdateQueue.js index a036c5d39e4..3297d66b2a1 100644 --- a/packages/react-reconciler/src/ReactFiberUpdateQueue.js +++ b/packages/react-reconciler/src/ReactFiberUpdateQueue.js @@ -37,6 +37,8 @@ export type Update = { callback: Callback | null, isReplace: boolean, isForced: boolean, + isCapture: boolean, + capturedValue: mixed | null, next: Update | null, }; @@ -64,6 +66,7 @@ export type UpdateQueue = { callbackList: Array> | null, hasForceUpdate: boolean, isInitialized: boolean, + capturedValues: Array | null, // Dev only isProcessing?: boolean, @@ -78,6 +81,7 @@ function createUpdateQueue(baseState: State): UpdateQueue { callbackList: null, hasForceUpdate: false, isInitialized: false, + capturedValues: null, }; if (__DEV__) { queue.isProcessing = false; @@ -213,6 +217,7 @@ export function processUpdateQueue( first: currentQueue.first, last: currentQueue.last, isInitialized: currentQueue.isInitialized, + capturedValues: currentQueue.capturedValues, // These fields are no longer valid because they were already committed. // Reset them. callbackList: null, @@ -304,12 +309,24 @@ export function processUpdateQueue( } callbackList.push(update); } + if (update.isCapture) { + let capturedValues = queue.capturedValues; + if (capturedValues === null) { + queue.capturedValues = [update.capturedValue]; + } else { + capturedValues.push(update.capturedValue); + } + } update = update.next; } if (queue.callbackList !== null) { workInProgress.effectTag |= CallbackEffect; - } else if (queue.first === null && !queue.hasForceUpdate) { + } else if ( + queue.first === null && + !queue.hasForceUpdate && + queue.capturedValues === null + ) { // The queue is empty. We can reset it. workInProgress.updateQueue = null; } diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.js index f12056aa5dd..172d9f9ebc0 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.js @@ -2717,7 +2717,9 @@ describe('ReactIncremental', () => { expect(ReactNoop.flush()).toEqual([]); }); - it('does not break with a bad Map polyfill', () => { + // We don't currently use fibers as keys. Re-enable this test if we + // ever do again. + it.skip('does not break with a bad Map polyfill', () => { const realMapSet = Map.prototype.set; function triggerCodePathThatUsesFibersAsMapKeys() { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js index 80210157e00..188c7390f92 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js @@ -61,27 +61,26 @@ describe('ReactIncrementalErrorHandling', () => { }); it('catches render error in a boundary during partial deferred mounting', () => { - const ops = []; class ErrorBoundary extends React.Component { state = {error: null}; componentDidCatch(error) { - ops.push('ErrorBoundary componentDidCatch'); + ReactNoop.yield('ErrorBoundary componentDidCatch'); this.setState({error}); } render() { if (this.state.error) { - ops.push('ErrorBoundary render error'); + ReactNoop.yield('ErrorBoundary render error'); return ( ); } - ops.push('ErrorBoundary render success'); + ReactNoop.yield('ErrorBoundary render success'); return this.props.children; } } function BrokenRender(props) { - ops.push('BrokenRender'); + ReactNoop.yield('BrokenRender'); throw new Error('Hello'); } @@ -91,13 +90,10 @@ describe('ReactIncrementalErrorHandling', () => { , ); - ReactNoop.flushDeferredPri(15); - expect(ops).toEqual(['ErrorBoundary render success']); + ReactNoop.flushThrough(['ErrorBoundary render success']); expect(ReactNoop.getChildren()).toEqual([]); - ops.length = 0; - ReactNoop.flushDeferredPri(30); - expect(ops).toEqual([ + expect(ReactNoop.flush()).toEqual([ 'BrokenRender', 'ErrorBoundary componentDidCatch', 'ErrorBoundary render error', @@ -398,7 +394,6 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.flush(); }).toThrow('Hello'); expect(ops).toEqual(['BrokenRender']); - ops = []; ReactNoop.render(); ReactNoop.flush(); diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js index 15630c1f496..25ff074ca78 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.internal.js @@ -179,11 +179,20 @@ describe('ReactIncrementalErrorLogging', () => { let renderAttempts = 0; class ErrorBoundaryComponent extends React.Component { + state = {error: null}; componentDidCatch(error) { handleErrorCalls.push(error); - this.setState({}); // Render again + if (renderAttempts < 10) { + // Render again + } else { + // Stop after 10 tries + this.setState({error}); + } } render() { + if (this.state.error !== null) { + return null; + } return ; } } @@ -208,9 +217,9 @@ describe('ReactIncrementalErrorLogging', () => { ReactNoop.flush(); } catch (error) {} - expect(renderAttempts).toBe(2); - expect(handleErrorCalls.length).toBe(1); - expect(console.error.calls.count()).toBe(2); + expect(renderAttempts).toBe(10); + expect(handleErrorCalls.length).toBe(10); + expect(console.error.calls.count()).toBe(10); if (__DEV__) { expect(console.error.calls.argsFor(0)[0]).toContain( 'The above error occurred in the component:', @@ -223,8 +232,8 @@ describe('ReactIncrementalErrorLogging', () => { 'The above error occurred in the component:', ); expect(console.error.calls.argsFor(1)[0]).toContain( - 'This error was initially handled by the error boundary ErrorBoundaryComponent.\n' + - 'Recreating the tree from scratch failed so React will unmount the tree.', + 'React will try to recreate this component tree from scratch using ' + + 'the error boundary you provided, ErrorBoundaryComponent', ); } }); diff --git a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap index 753ac38ef07..c8d50898954 100644 --- a/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap +++ b/packages/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap @@ -221,22 +221,15 @@ exports[`ReactDebugFiberPerf recovers from caught errors 1`] = ` // Stop on Baddie and restart from Boundary ⚛ (React Tree Reconciliation) ⚛ Parent [mount] - ⛔ Boundary [mount] Warning: An error was thrown inside this error boundary + ⚛ Boundary [mount] ⚛ Parent [mount] ⚛ Baddie [mount] - ⚛ Boundary [mount] - -⛔ (Committing Changes) Warning: Lifecycle hook scheduled a cascading update - ⚛ (Committing Host Effects: 2 Total) - ⚛ (Calling Lifecycle Methods: 1 Total) - -⚛ (React Tree Reconciliation) - ⚛ Boundary [update] - ⚛ ErrorReport [mount] + ⛔ Boundary [mount] Warning: An error was thrown inside this error boundary + ⚛ ErrorReport [mount] ⚛ (Committing Changes) ⚛ (Committing Host Effects: 2 Total) - ⚛ (Calling Lifecycle Methods: 1 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) " `; @@ -249,8 +242,8 @@ exports[`ReactDebugFiberPerf recovers from fatal errors 1`] = ` ⚛ Baddie [mount] ⚛ (Committing Changes) - ⚛ (Committing Host Effects: 1 Total) - ⚛ (Calling Lifecycle Methods: 1 Total) + ⚛ (Committing Host Effects: 0 Total) + ⚛ (Calling Lifecycle Methods: 0 Total) ⚛ (Waiting for async callback...) diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js index 3d461d293b5..5efc96fc6f0 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js @@ -460,8 +460,8 @@ describe('ReactTestRenderer', () => { expect(log).toEqual([ 'Boundary render', 'Angry render', - 'Boundary componentDidMount', 'Boundary render', + 'Boundary componentDidMount', ]); }); From fa3884d4f2a8413bb33604ef2e76a093efb90de3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 21 Dec 2017 12:03:24 -0800 Subject: [PATCH 2/6] Test recovering from render phase errors asynchronously --- .../ReactIncrementalErrorHandling-test.js | 62 +++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js index 188c7390f92..913ea2a69d3 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js @@ -31,6 +31,68 @@ describe('ReactIncrementalErrorHandling', () => { return {type: 'span', children: [], prop}; } + it('recovers from errors asynchronously', () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + componentDidCatch(error) { + ReactNoop.yield('componentDidCatch'); + this.setState({error}); + } + render() { + if (this.state.error) { + ReactNoop.yield('ErrorBoundary (catch)'); + return ; + } + ReactNoop.yield('ErrorBoundary (try)'); + return this.props.children; + } + } + + function ErrorMessage(props) { + ReactNoop.yield('ErrorMessage'); + return ; + } + + function Indirection(props) { + ReactNoop.yield('Indirection'); + return props.children; + } + + function BadRender() { + ReactNoop.yield('throw'); + throw new Error('oops!'); + } + + ReactNoop.render( + + + + + + + + + , + ); + + ReactNoop.flushThrough([ + 'ErrorBoundary (try)', + 'Indirection', + 'Indirection', + 'Indirection', + // The error was thrown, but React ran out of time and yielded + // before recovering. + 'throw', + ]); + + // Upon resuming, componentDidCatch is called + ReactNoop.flushThrough(['componentDidCatch', 'ErrorBoundary (catch)']); + + // Flush the rest of the work + expect(ReactNoop.flush()).toEqual(['ErrorMessage']); + expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]); + }); + it('catches render error in a boundary during full deferred mounting', () => { class ErrorBoundary extends React.Component { state = {error: null}; From 7e170ab236a89baac03c300015f59e9935a2303b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Jan 2018 10:39:19 -0800 Subject: [PATCH 3/6] Error boundaries only capture errors Non-errors are skipped by error boundaries. We'll use the same pattern matching strategy for other types of boundaries in the future. If a thrown value reaches the root, it's rethrown regardless of type. --- .../src/ReactCapturedValue.js | 129 ++++++++++++++ .../src/ReactFiberBeginWork.js | 42 +++-- .../src/ReactFiberClassComponent.js | 14 +- .../src/ReactFiberCompleteWork.js | 3 +- .../src/ReactFiberErrorDialog.js | 2 +- .../src/ReactFiberErrorLogger.js | 2 +- .../src/ReactFiberScheduler.js | 162 ++++++++---------- .../src/ReactFiberUpdateQueue.js | 8 +- .../ReactIncrementalErrorHandling-test.js | 50 ++++++ .../src/forks/ReactFiberErrorDialog.native.js | 2 +- .../src/forks/ReactFiberErrorDialog.www.js | 2 +- 11 files changed, 288 insertions(+), 128 deletions(-) create mode 100644 packages/react-reconciler/src/ReactCapturedValue.js diff --git a/packages/react-reconciler/src/ReactCapturedValue.js b/packages/react-reconciler/src/ReactCapturedValue.js new file mode 100644 index 00000000000..ccfa909a057 --- /dev/null +++ b/packages/react-reconciler/src/ReactCapturedValue.js @@ -0,0 +1,129 @@ +/** + * Copyright (c) 2013-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {Fiber} from './ReactFiber'; + +import {ClassComponent} from 'shared/ReactTypeOfWork'; +import getComponentName from 'shared/getComponentName'; +import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook'; + +import {logCapturedError} from './ReactFiberErrorLogger'; + +const getPrototypeOf = + Object.getPrototypeOf === 'function' ? Object.getPrototypeOf : null; +const objectToString = Object.prototype.toString; + +export type CapturedValue = { + value: T, + isError: boolean, + source: Fiber | null, + boundary: Fiber | null, + stack: string | null, +}; + +// Object that is passed to the error logger module. +// TODO: This is different for legacy reasons, but I don't think it's +// exposed to anyone outside FB, so we can probably change it +export type CapturedError = { + componentName: string | null, + componentStack: string, + error: mixed, + errorBoundary: Fiber | null, + errorBoundaryFound: boolean, + errorBoundaryName: string | null, + willRetry: boolean, +}; + +export function createCapturedValue( + value: T, + source: Fiber | null, +): CapturedValue { + const valueIsError = isError(value); + return { + value, + isError: valueIsError, + source, + boundary: null, + // Don't compute the stack unless this is an error. + stack: + source !== null && valueIsError + ? getStackAddendumByWorkInProgressFiber(source) + : null, + }; +} + +export function logError(capturedValue: CapturedValue): void { + const capturedError = createCapturedError(capturedValue); + try { + logCapturedError(capturedError); + } catch (e) { + // Prevent cycle if logCapturedError() throws. + // A cycle may still occur if logCapturedError renders a component that throws. + const suppressLogging = e && e.suppressReactErrorLogging; + if (!suppressLogging) { + console.error(e); + } + } +} + +function createCapturedError( + capturedValue: CapturedValue, +): CapturedError { + const source = capturedValue.source; + const boundary = capturedValue.boundary; + const stack = capturedValue.stack; + + const capturedError: CapturedError = { + componentName: source !== null ? getComponentName(source) : null, + error: capturedValue.value, + errorBoundary: boundary, + componentStack: stack !== null ? stack : '', + errorBoundaryName: null, + errorBoundaryFound: false, + willRetry: false, + }; + + if (boundary !== null) { + capturedError.errorBoundaryName = getComponentName(boundary); + // TODO: These are always the same. Why is it needed? + capturedError.errorBoundaryFound = capturedError.willRetry = + boundary.tag === ClassComponent; + } else { + capturedError.errorBoundaryName = null; + capturedError.errorBoundaryFound = capturedError.willRetry = false; + } + + return capturedError; +} + +function isError(value: mixed): boolean { + if (value instanceof Error) { + return true; + } + + // instanceof fails across realms. Check the prototype chain. + if (getPrototypeOf !== null) { + let proto = getPrototypeOf(value); + while (proto !== null) { + if (objectToString.call(proto) === '[object Error]') { + return true; + } + proto = getPrototypeOf(value); + } + return false; + } + + // If getPrototypeOf is not available, fall back to duck typing. + return ( + value !== null && + typeof value === 'object' && + typeof value.stack === 'string' && + typeof value.message === 'string' + ); +} diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index f85956ea7ef..ebe9a2f790a 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -13,7 +13,7 @@ import type {HostContext} from './ReactFiberHostContext'; import type {HydrationContext} from './ReactFiberHydrationContext'; import type {FiberRoot} from './ReactFiberRoot'; import type {ExpirationTime} from './ReactFiberExpirationTime'; -import type {CapturedError} from './ReactFiberScheduler'; +import type {CapturedValue} from './ReactCapturedValue'; import { IndeterminateComponent, @@ -36,6 +36,7 @@ import { } from 'shared/ReactTypeOfSideEffect'; import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState'; import {debugRenderPhaseSideEffects} from 'shared/ReactFeatureFlags'; +import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook'; import invariant from 'fbjs/lib/invariant'; import getComponentName from 'shared/getComponentName'; import warning from 'fbjs/lib/warning'; @@ -57,8 +58,9 @@ import { pushTopLevelContextObject, invalidateContextProvider, } from './ReactFiberContext'; + import {NoWork, Never} from './ReactFiberExpirationTime'; -import {logError} from './ReactFiberScheduler'; +import {logError} from './ReactCapturedValue'; let warnedAboutStatelessRefs; @@ -210,7 +212,7 @@ export default function( function updateClassComponent( current: Fiber | null, workInProgress: Fiber, - capturedValues: Array | null, + capturedValues: Array> | null, renderExpirationTime: ExpirationTime, ) { // Push context providers early to prevent context stack mismatches. @@ -224,10 +226,11 @@ export default function( didCaptureError = true; invariant(instance !== null, 'Expected class to have an instance.'); // TODO: Pattern matching. Check that this is an error. - const capturedError: CapturedError = (capturedValues[0]: any); - logError(workInProgress, capturedError); - const error = capturedError.error; - instance.componentDidCatch(error); + const capturedValue: CapturedValue = (capturedValues[0]: any); + if (capturedValue.isError) { + logError(capturedValue); + instance.componentDidCatch(capturedValue.value); + } } let shouldUpdate; @@ -373,11 +376,20 @@ export default function( capturedValues, renderExpirationTime, ) { - // TODO: Pattern matching. Check that this is an error. - const didError = true; - const capturedError: CapturedError = (capturedValues[0]: any); - const error = capturedError.error; + const capturedValue: CapturedValue = (capturedValues[0]: any); + if (capturedValue.isError) { + logError(capturedValue); + } else { + capturedValue.isError = true; + const source = capturedValue.source; + if (source !== null) { + capturedValue.stack = getStackAddendumByWorkInProgressFiber(source); + } + } + const error = capturedValue.value; markUncaughtError(error); + + const didError = true; reconcileChildrenAtExpirationTime( current, workInProgress, @@ -396,9 +408,6 @@ export default function( ) { pushHostRootContext(workInProgress); if (capturedValues !== null) { - // TODO: Pattern matching. Check that this is an error. - const capturedError: CapturedError = (capturedValues[0]: any); - logError(workInProgress, capturedError); return unmountFailedRoot( current, workInProgress, @@ -422,9 +431,6 @@ export default function( if (updateQueue !== null && updateQueue.capturedValues !== null) { capturedValues = updateQueue.capturedValues; updateQueue.capturedValues = null; - // TODO: Pattern matching. Check that this is an error. - const capturedError: CapturedError = (capturedValues[0]: any); - logError(workInProgress, capturedError); memoizeState(workInProgress, state); return unmountFailedRoot( current, @@ -802,7 +808,7 @@ export default function( function beginWork( current: Fiber | null, workInProgress: Fiber, - capturedValues: Array | null, + capturedValues: Array> | null, renderExpirationTime: ExpirationTime, ): Fiber | null { // The effect list is no longer valid. diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 6f11adcf077..4c1b1c1f184 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -9,7 +9,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; -import type {CapturedError} from './ReactFiberScheduler'; +import type {CapturedValue} from './ReactCapturedValue'; import {Update} from 'shared/ReactTypeOfSideEffect'; import { @@ -37,7 +37,7 @@ import { processUpdateQueue, } from './ReactFiberUpdateQueue'; import {hasContextChanged} from './ReactFiberContext'; -import {logError} from './ReactFiberScheduler'; +import {logError} from './ReactCapturedValue'; const fakeInternalInstance = {}; const isArray = Array.isArray; @@ -650,11 +650,13 @@ export default function( // TODO: This is awkward. Refactor class components. // updateQueue.capturedValues = null; - // TODO: Pattern matching. Check that this is an error. - const capturedError: CapturedError = (capturedValues[0]: any); - logError(workInProgress, capturedError); - const error = capturedError.error; + const capturedValue: CapturedValue = (capturedValues[0]: any); + if (capturedValue.isError) { + logError(capturedValue); + } + const error = capturedValue.value; instance.componentDidCatch(error); + newState = processUpdateQueue( current, workInProgress, diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 09f10af347a..cb0241d7b8b 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -52,6 +52,7 @@ export default function( hostContext: HostContext, hydrationContext: HydrationContext, captureThrownValues: () => boolean, + captureErrors: () => boolean, ) { const { createInstance, @@ -409,7 +410,7 @@ export default function( instance !== null && typeof instance.componentDidCatch === 'function' ) { - const didCapture = captureThrownValues(); + const didCapture = captureErrors(); if (didCapture) { workInProgress.effectTag |= Err; return workInProgress; diff --git a/packages/react-reconciler/src/ReactFiberErrorDialog.js b/packages/react-reconciler/src/ReactFiberErrorDialog.js index 8459be5bad8..60863514d8a 100644 --- a/packages/react-reconciler/src/ReactFiberErrorDialog.js +++ b/packages/react-reconciler/src/ReactFiberErrorDialog.js @@ -7,7 +7,7 @@ * @flow */ -import type {CapturedError} from './ReactFiberScheduler'; +import type {CapturedError} from './ReactCapturedValue'; // This module is forked in different environments. // By default, return `true` to log errors to the console. diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index d9bf2bb950c..1a15878262d 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -7,7 +7,7 @@ * @flow */ -import type {CapturedError} from './ReactFiberScheduler'; +import type {CapturedError} from './ReactCapturedValue'; import {showErrorDialog} from './ReactFiberErrorDialog'; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index c8a2be0f316..e49db71ab06 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -12,8 +12,8 @@ import type {Fiber} from './ReactFiber'; import type {FiberRoot, Batch} from './ReactFiberRoot'; import type {HydrationContext} from './ReactFiberHydrationContext'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {CapturedValue} from './ReactCapturedValue'; -import {getStackAddendumByWorkInProgressFiber} from 'shared/ReactFiberComponentTreeHook'; import ReactErrorUtils from 'shared/ReactErrorUtils'; import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState'; import { @@ -64,7 +64,6 @@ import { } from './ReactDebugFiberPerf'; import {popContextProvider} from './ReactFiberContext'; import {reset} from './ReactFiberStack'; -import {logCapturedError} from './ReactFiberErrorLogger'; import {createWorkInProgress} from './ReactFiber'; import {onCommitRoot} from './ReactFiberDevToolsHook'; import { @@ -81,6 +80,7 @@ import { insertUpdateIntoFiber, } from './ReactFiberUpdateQueue'; import {resetContext} from './ReactFiberContext'; +import {createCapturedValue} from './ReactCapturedValue'; const { invokeGuardedCallback, @@ -88,20 +88,6 @@ const { clearCaughtError, } = ReactErrorUtils; -export type CapturedError = { - componentName: ?string, - componentStack: string, - error: mixed, - errorBoundary: ?Object, - errorBoundaryFound: boolean, - errorBoundaryName: string | null, - willRetry: boolean, -}; - -export type HandleErrorInfo = { - componentStack: string, -}; - let didWarnAboutStateTransition; let didWarnSetStateChildContext; let warnAboutUpdateOnUnmounted; @@ -157,27 +143,6 @@ if (__DEV__) { }; } -export function logError(boundaryFiber: Fiber, capturedError: CapturedError) { - const errorBoundaryFound = boundaryFiber.tag === ClassComponent; - capturedError.errorBoundary = boundaryFiber; - capturedError.errorBoundaryFound = errorBoundaryFound; - capturedError.errorBoundaryName = errorBoundaryFound - ? getComponentName(boundaryFiber) - : null; - // TODO: These are always the same. Why is it needed? - capturedError.willRetry = errorBoundaryFound; - try { - logCapturedError(capturedError); - } catch (e) { - // Prevent cycle if logCapturedError() throws. - // A cycle may still occur if logCapturedError renders a component that throws. - const suppressLogging = e && e.suppressReactErrorLogging; - if (!suppressLogging) { - console.error(e); - } - } -} - export default function( config: HostConfig, ) { @@ -199,6 +164,7 @@ export default function( hostContext, hydrationContext, captureThrownValues, + captureErrors, ); const { commitResetTextContent, @@ -246,8 +212,8 @@ export default function( let isCommitting: boolean = false; - let thrownValues: Array | null = null; - let capturedValues: Array | null = null; + let thrownValues: Array> | null = null; + let capturedValues: Array> | null = null; // Used for performance tracking. let interruptedBy: Fiber | null = null; @@ -679,40 +645,18 @@ export default function( } } - function unwindToNearestBoundary(sourceFiber: Fiber, thrownValue: mixed) { - // Push the thrown value onto the global list. - - // TODO: Pattern matching. Check that this is an error. - const capturedError: CapturedError = { - componentName: getComponentName(sourceFiber), - componentStack: getStackAddendumByWorkInProgressFiber(sourceFiber), - error: thrownValue, - errorBoundary: null, - errorBoundaryFound: false, - errorBoundaryName: null, - willRetry: false, - }; - if (thrownValues === null) { - thrownValues = [capturedError]; - } else { - thrownValues.push(capturedError); - } - + function unwindToNearestErrorBoundary(sourceFiber) { popContexts(sourceFiber); - - // TODO: If the value is something other than an error (like a - // promise), continue working on the siblings, and unwind using - // the normal complete phase algorithm. This should be safe, because - // we assume the value was thrown from inside the render method. For - // errors, we have to skip the siblings and immediately unwind to - // the nearest boundary, because the tree may be in a corrupt state. let boundaryFiber = null; let fiber = sourceFiber.return; traversal: while (fiber !== null) { switch (fiber.tag) { case ClassComponent: const instance = fiber.stateNode; - if (typeof instance.componentDidCatch === 'function') { + if ( + typeof instance.componentDidCatch === 'function' && + !(fiber.effectTag & Err) + ) { boundaryFiber = fiber; break traversal; } @@ -724,7 +668,6 @@ export default function( popContexts(fiber); fiber = fiber.return; } - return boundaryFiber; } @@ -795,14 +738,36 @@ export default function( break; } - const boundary = unwindToNearestBoundary(nextUnitOfWork, thrownValue); - if (boundary !== null) { - nextUnitOfWork = completeUnitOfWork(boundary); + const sourceFiber: Fiber = nextUnitOfWork; + const capturedValue = createCapturedValue(thrownValue, sourceFiber); + if (thrownValues === null) { + thrownValues = [capturedValue]; } else { - // The root failed to render. This is a fatal error. - hasUncaughtError = true; - firstUncaughtError = thrownValue; - break; + thrownValues.push(capturedValue); + } + if (capturedValue.isError) { + const boundaryFiber = unwindToNearestErrorBoundary(sourceFiber); + capturedValue.boundary = boundaryFiber; + if (boundaryFiber !== null) { + nextUnitOfWork = completeUnitOfWork(boundaryFiber); + } else { + // The root failed to render. This is a fatal error. + hasUncaughtError = true; + firstUncaughtError = thrownValue; + break; + } + } else { + // Move to next sibling, or return to parent + if (sourceFiber.sibling !== null) { + nextUnitOfWork = sourceFiber.sibling; + } else if (sourceFiber.return !== null) { + nextUnitOfWork = completeUnitOfWork(sourceFiber.return); + } else { + // The root failed to render. This is a fatal error. + hasUncaughtError = true; + firstUncaughtError = thrownValue; + break; + } } } while (true); @@ -846,36 +811,43 @@ export default function( if (thrownValues === null) { return false; } - capturedValues = thrownValues; - // Clear the values from the scheduler. + // Reset the list of thrown values, now that they've been captured. thrownValues = null; return true; } + function captureErrors(): boolean { + if (thrownValues === null) { + return false; + } + let errors = null; + for (let i = 0; i < thrownValues.length; i++) { + const value = thrownValues[i]; + if (value.isError) { + thrownValues.splice(i, 1); + if (errors === null) { + errors = [value]; + } else { + errors.push(value); + } + } + } + if (thrownValues.length === 0) { + thrownValues = null; + } + capturedValues = errors; + return true; + } + function markUncaughtError(error: mixed): void { hasUncaughtError = true; firstUncaughtError = error; } - function scheduleCapture( - sourceFiber, - boundaryFiber, - capturedValue, - expirationTime, - ) { - // TODO: Pattern matching. Check that this is an error. - const capturedError: CapturedError = { - componentName: getComponentName(sourceFiber), - componentStack: getStackAddendumByWorkInProgressFiber(sourceFiber), - error: capturedValue, - // Set these once the boundary captures, during the render phase. - errorBoundary: null, - errorBoundaryFound: false, - errorBoundaryName: null, - willRetry: false, - }; - + function scheduleCapture(sourceFiber, boundaryFiber, value, expirationTime) { + const capturedValue = createCapturedValue(value, sourceFiber); + capturedValue.boundary = boundaryFiber; const update = { expirationTime, partialState: null, @@ -883,7 +855,7 @@ export default function( isReplace: false, isForced: false, isCapture: true, - capturedValue: capturedError, + capturedValue, next: null, }; insertUpdateIntoFiber(boundaryFiber, update); diff --git a/packages/react-reconciler/src/ReactFiberUpdateQueue.js b/packages/react-reconciler/src/ReactFiberUpdateQueue.js index 3297d66b2a1..c160cbc7068 100644 --- a/packages/react-reconciler/src/ReactFiberUpdateQueue.js +++ b/packages/react-reconciler/src/ReactFiberUpdateQueue.js @@ -9,6 +9,7 @@ import type {Fiber} from './ReactFiber'; import type {ExpirationTime} from './ReactFiberExpirationTime'; +import type {CapturedValue} from './ReactCapturedValue'; import {debugRenderPhaseSideEffects} from 'shared/ReactFeatureFlags'; import {Callback as CallbackEffect} from 'shared/ReactTypeOfSideEffect'; @@ -37,8 +38,7 @@ export type Update = { callback: Callback | null, isReplace: boolean, isForced: boolean, - isCapture: boolean, - capturedValue: mixed | null, + capturedValue: CapturedValue | null, next: Update | null, }; @@ -66,7 +66,7 @@ export type UpdateQueue = { callbackList: Array> | null, hasForceUpdate: boolean, isInitialized: boolean, - capturedValues: Array | null, + capturedValues: Array> | null, // Dev only isProcessing?: boolean, @@ -309,7 +309,7 @@ export function processUpdateQueue( } callbackList.push(update); } - if (update.isCapture) { + if (update.capturedValue !== null) { let capturedValues = queue.capturedValues; if (capturedValues === null) { queue.capturedValues = [update.capturedValue]; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js index 913ea2a69d3..bc75d2cf2c4 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js @@ -1008,4 +1008,54 @@ describe('ReactIncrementalErrorHandling', () => { }); expect(() => ReactNoop.flush()).toThrow('Error!'); }); + + it('error boundaries do not capture non-errors', () => { + let ops = []; + + class ErrorBoundary extends React.Component { + state = {error: null}; + componentDidCatch(error) { + // Should not be called + ops.push('componentDidCatch'); + this.setState({error}); + } + render() { + if (this.state.error) { + ops.push('ErrorBoundary (catch)'); + return ; + } + ops.push('ErrorBoundary (try)'); + return this.props.children; + } + } + + function Indirection(props) { + ops.push('Indirection'); + return props.children; + } + + const notAnError = {}; + function BadRender() { + ops.push('BadRender'); + throw notAnError; + } + + ReactNoop.render( + + + + + , + ); + + let caught; + try { + ReactNoop.flush(); + } catch (e) { + caught = e; + } + // Rethrow object at the root + expect(caught).toBe(notAnError); + expect(ops).toEqual(['ErrorBoundary (try)', 'Indirection', 'BadRender']); + }); }); diff --git a/packages/react-reconciler/src/forks/ReactFiberErrorDialog.native.js b/packages/react-reconciler/src/forks/ReactFiberErrorDialog.native.js index 40d3d6460f8..357c21dddb1 100644 --- a/packages/react-reconciler/src/forks/ReactFiberErrorDialog.native.js +++ b/packages/react-reconciler/src/forks/ReactFiberErrorDialog.native.js @@ -7,7 +7,7 @@ * @flow */ -import type {CapturedError} from '../ReactFiberScheduler'; +import type {CapturedError} from '../ReactCapturedValue'; // Module provided by RN: import ExceptionsManager from 'ExceptionsManager'; diff --git a/packages/react-reconciler/src/forks/ReactFiberErrorDialog.www.js b/packages/react-reconciler/src/forks/ReactFiberErrorDialog.www.js index b7cd2d59d01..16d9f71c4c9 100644 --- a/packages/react-reconciler/src/forks/ReactFiberErrorDialog.www.js +++ b/packages/react-reconciler/src/forks/ReactFiberErrorDialog.www.js @@ -7,7 +7,7 @@ * @flow */ -import type {CapturedError} from '../ReactFiberScheduler'; +import type {CapturedError} from '../ReactCapturedValue'; import invariant from 'fbjs/lib/invariant'; From 1897e5dcab2b2771832a2dd10e53535329d9ba17 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Jan 2018 11:43:19 -0800 Subject: [PATCH 4/6] Use "resume mount" path when recovering from mount error Error boundaries are a type of resuming. --- .../__tests__/ReactErrorBoundaries-test.js | 20 +- .../src/ReactFiberBeginWork.js | 21 +- .../src/ReactFiberClassComponent.js | 239 ++++++++++-------- 3 files changed, 151 insertions(+), 129 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js index 7cda0fee506..3da0bf1312c 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js @@ -627,7 +627,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // Catch and render an error message 'ErrorBoundary componentDidCatch', - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -653,7 +653,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenConstructor constructor [!]', // Catch and render an error message 'ErrorBoundary componentDidCatch', - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -679,7 +679,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillMount constructor', 'BrokenComponentWillMount componentWillMount [!]', 'ErrorBoundary componentDidCatch', - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -761,7 +761,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender componentWillMount', 'BrokenRender render [!]', 'ErrorBoundary componentDidCatch', - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorMessage constructor', 'ErrorMessage componentWillMount', @@ -800,6 +800,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender componentWillMount', 'BrokenRender render [!]', 'RetryErrorBoundary componentDidCatch [!]', + 'RetryErrorBoundary componentWillMount', // Retry 'RetryErrorBoundary render', 'BrokenRender constructor', @@ -808,7 +809,7 @@ describe('ReactErrorBoundaries', () => { // This time, the error propagates to the higher boundary 'ErrorBoundary componentDidCatch', // Render the error - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -835,7 +836,7 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillMountErrorBoundary componentWillMount [!]', // The error propagates to the higher boundary 'ErrorBoundary componentDidCatch', - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -870,11 +871,12 @@ describe('ReactErrorBoundaries', () => { // It adjusts state but throws displaying the message // Attempt to handle the error 'BrokenRenderErrorBoundary componentDidCatch', + 'BrokenRenderErrorBoundary componentWillMount', 'BrokenRenderErrorBoundary render error [!]', // Boundary fails with new error, propagate to next boundary // Attempt to handle the error again 'ErrorBoundary componentDidCatch', - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -910,7 +912,7 @@ describe('ReactErrorBoundaries', () => { // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'ErrorBoundary componentDidMount', ]); @@ -947,7 +949,7 @@ describe('ReactErrorBoundaries', () => { // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message - 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary componentWillMount', 'ErrorBoundary render error', 'Error message ref is set to [object HTMLDivElement]', 'ErrorBoundary componentDidMount', diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index ebe9a2f790a..0eba32ca24b 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -94,7 +94,7 @@ export default function( adoptClassInstance, constructClassInstance, mountClassInstance, - // resumeMountClassInstance, + resumeMountClassInstance, updateClassInstance, } = ReactFiberClassComponent( scheduleWork, @@ -251,20 +251,11 @@ export default function( shouldUpdate = true; } else { - if (capturedValues !== null) { - // A capture is more like an update than another mount, - // because we haven't received new props from the parent. - shouldUpdate = true; - updateClassInstance(current, workInProgress, renderExpirationTime); - } else { - // In a resume, we'll already have an instance we can reuse. - // shouldUpdate = resumeMountClassInstance(workInProgress, renderExpirationTime); - invariant( - capturedValues !== null, - 'Resuming work not yet implemented.', - ); - shouldUpdate = true; - } + // In a resume, we'll already have an instance we can reuse. + shouldUpdate = resumeMountClassInstance( + workInProgress, + renderExpirationTime, + ); } } else { shouldUpdate = updateClassInstance( diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 4c1b1c1f184..2aa7fa259d5 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -491,110 +491,139 @@ export default function( } } - // Called on a preexisting class instance. Returns false if a resumed render - // could be reused. - // function resumeMountClassInstance( - // workInProgress: Fiber, - // priorityLevel: PriorityLevel, - // ): boolean { - // const instance = workInProgress.stateNode; - // resetInputPointers(workInProgress, instance); - - // let newState = workInProgress.memoizedState; - // let newProps = workInProgress.pendingProps; - // if (!newProps) { - // // If there isn't any new props, then we'll reuse the memoized props. - // // This could be from already completed work. - // newProps = workInProgress.memoizedProps; - // invariant( - // newProps != null, - // 'There should always be pending or memoized props. This error is ' + - // 'likely caused by a bug in React. Please file an issue.', - // ); - // } - // const newUnmaskedContext = getUnmaskedContext(workInProgress); - // const newContext = getMaskedContext(workInProgress, newUnmaskedContext); - - // const oldContext = instance.context; - // const oldProps = workInProgress.memoizedProps; - - // if ( - // typeof instance.componentWillReceiveProps === 'function' && - // (oldProps !== newProps || oldContext !== newContext) - // ) { - // callComponentWillReceiveProps( - // workInProgress, - // instance, - // newProps, - // newContext, - // ); - // } - - // // Process the update queue before calling shouldComponentUpdate - // const updateQueue = workInProgress.updateQueue; - // if (updateQueue !== null) { - // newState = processUpdateQueue( - // workInProgress, - // updateQueue, - // instance, - // newState, - // newProps, - // priorityLevel, - // ); - // } - - // // TODO: Should we deal with a setState that happened after the last - // // componentWillMount and before this componentWillMount? Probably - // // unsupported anyway. - - // if ( - // !checkShouldComponentUpdate( - // workInProgress, - // workInProgress.memoizedProps, - // newProps, - // workInProgress.memoizedState, - // newState, - // newContext, - // ) - // ) { - // // Update the existing instance's state, props, and context pointers even - // // though we're bailing out. - // instance.props = newProps; - // instance.state = newState; - // instance.context = newContext; - // return false; - // } - - // // Update the input pointers now so that they are correct when we call - // // componentWillMount - // instance.props = newProps; - // instance.state = newState; - // instance.context = newContext; - - // if (typeof instance.componentWillMount === 'function') { - // callComponentWillMount(workInProgress, instance); - // // componentWillMount may have called setState. Process the update queue. - // const newUpdateQueue = workInProgress.updateQueue; - // if (newUpdateQueue !== null) { - // newState = processUpdateQueue( - // workInProgress, - // newUpdateQueue, - // instance, - // newState, - // newProps, - // priorityLevel, - // ); - // } - // } - - // if (typeof instance.componentDidMount === 'function') { - // workInProgress.effectTag |= Update; - // } - - // instance.state = newState; - - // return true; - // } + function resumeMountClassInstance( + workInProgress: Fiber, + renderExpirationTime: ExpirationTime, + ): boolean { + const instance = workInProgress.stateNode; + resetInputPointers(workInProgress, instance); + + const oldProps = workInProgress.memoizedProps; + const newProps = workInProgress.pendingProps; + const oldContext = instance.context; + const newUnmaskedContext = getUnmaskedContext(workInProgress); + const newContext = getMaskedContext(workInProgress, newUnmaskedContext); + + // Note: During these life-cycles, instance.props/instance.state are what + // ever the previously attempted to render - not the "current". However, + // during componentDidUpdate we pass the "current" props. + + if ( + typeof instance.componentWillReceiveProps === 'function' && + (oldProps !== newProps || oldContext !== newContext) + ) { + callComponentWillReceiveProps( + workInProgress, + instance, + newProps, + newContext, + ); + } + + // Compute the next state using the memoized state and the update queue. + const oldState = workInProgress.memoizedState; + // TODO: Previous state can be null. + let newState; + if (workInProgress.updateQueue !== null) { + newState = processUpdateQueue( + null, + workInProgress, + workInProgress.updateQueue, + instance, + newProps, + renderExpirationTime, + ); + + let updateQueue = workInProgress.updateQueue; + if (updateQueue !== null && updateQueue.capturedValues !== null) { + const capturedValues = updateQueue.capturedValues; + + // Don't remove these from the update queue yet. We need them in + // finishClassComponent. Do the reset there. + // TODO: This is awkward. Refactor class components. + // updateQueue.capturedValues = null; + + const capturedValue: CapturedValue = (capturedValues[0]: any); + if (capturedValue.isError) { + logError(capturedValue); + } + const error = capturedValue.value; + instance.componentDidCatch(error); + + newState = processUpdateQueue( + null, + workInProgress, + updateQueue, + instance, + newProps, + renderExpirationTime, + ); + } + } else { + newState = oldState; + } + + if ( + oldProps === newProps && + oldState === newState && + !hasContextChanged() && + !( + workInProgress.updateQueue !== null && + workInProgress.updateQueue.hasForceUpdate + ) + ) { + // If an update was already in progress, we should schedule an Update + // effect even though we're bailing out, so that cWU/cDU are called. + if (typeof instance.componentDidUpdate === 'function') { + workInProgress.effectTag |= Update; + } + return false; + } + + const shouldUpdate = checkShouldComponentUpdate( + workInProgress, + oldProps, + newProps, + oldState, + newState, + newContext, + ); + + if (shouldUpdate) { + if (typeof instance.componentWillMount === 'function') { + startPhaseTimer(workInProgress, 'componentWillMount'); + instance.componentWillMount(newProps, newState, newContext); + stopPhaseTimer(); + + // Simulate an async bailout/interruption by invoking lifecycle twice. + if (debugRenderPhaseSideEffects) { + instance.componentWillMount(newProps, newState, newContext); + } + } + if (typeof instance.componentDidMount === 'function') { + workInProgress.effectTag |= Update; + } + } else { + // If an update was already in progress, we should schedule an Update + // effect even though we're bailing out, so that cWU/cDU are called. + if (typeof instance.componentDidMount === 'function') { + workInProgress.effectTag |= Update; + } + + // If shouldComponentUpdate returned false, we should still update the + // memoized props/state to indicate that this work can be reused. + memoizeProps(workInProgress, newProps); + memoizeState(workInProgress, newState); + } + + // Update the existing instance's state, props, and context pointers even + // if shouldComponentUpdate returns false. + instance.props = newProps; + instance.state = newState; + instance.context = newContext; + + return shouldUpdate; + } // Invokes the update life-cycles and returns false if it shouldn't rerender. function updateClassInstance( @@ -746,7 +775,7 @@ export default function( adoptClassInstance, constructClassInstance, mountClassInstance, - // resumeMountClassInstance, + resumeMountClassInstance, updateClassInstance, }; } From df64ed08df9f60366a8011aa6f041ecf00846078 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Jan 2018 13:44:21 -0800 Subject: [PATCH 5/6] Call componentDidCatch for each captured error If a boundary captures multiple errors, componentDidCatch should fire for each one. This is only possible for commit phase errors, because render phase errors unwind immediately to the nearest boundary. --- .../__tests__/ReactErrorBoundaries-test.js | 6 ++- .../src/ReactFiberClassComponent.js | 31 +++++------- .../ReactIncrementalErrorHandling-test.js | 47 +++++++++++++++++++ 3 files changed, 63 insertions(+), 21 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js index 3da0bf1312c..bf6dd38fac6 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js @@ -1798,7 +1798,7 @@ describe('ReactErrorBoundaries', () => { expect(log).toEqual(['ErrorBoundary componentWillUnmount']); }); - it('lets different boundaries catch their own first errors', () => { + it('lets different boundaries catch their own errors', () => { function renderUnmountError(error) { return
Caught an unmounting error: {error.message}.
; } @@ -1843,7 +1843,7 @@ describe('ReactErrorBoundaries', () => { ); expect(container.firstChild.textContent).toBe( - 'Caught an unmounting error: E1.' + 'Caught an updating error: E3.', + 'Caught an unmounting error: E2.' + 'Caught an updating error: E4.', ); expect(log).toEqual([ // Begin update phase @@ -1877,9 +1877,11 @@ describe('ReactErrorBoundaries', () => { // After the commit phase, attempt to recover from any errors that // were captured 'InnerUnmountBoundary componentDidCatch', + 'InnerUnmountBoundary componentDidCatch', 'InnerUnmountBoundary componentWillUpdate', 'InnerUnmountBoundary render error', 'InnerUpdateBoundary componentDidCatch', + 'InnerUpdateBoundary componentDidCatch', 'InnerUpdateBoundary componentWillUpdate', 'InnerUpdateBoundary render error', 'BrokenComponentDidUpdate componentWillUnmount', diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 2aa7fa259d5..07e6891deba 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -440,6 +440,15 @@ export default function( } } + function callComponentDidCatch(instance: any, capturedValues: Array) { + for (let i = 0; i < capturedValues.length; i++) { + const capturedValue: CapturedValue = (capturedValues[i]: any); + logError(capturedValue); + const error = capturedValue.value; + instance.componentDidCatch(error); + } + } + // Invokes the mount life-cycles on a previously never rendered instance. function mountClassInstance( workInProgress: Fiber, @@ -537,19 +546,11 @@ export default function( let updateQueue = workInProgress.updateQueue; if (updateQueue !== null && updateQueue.capturedValues !== null) { const capturedValues = updateQueue.capturedValues; - // Don't remove these from the update queue yet. We need them in // finishClassComponent. Do the reset there. // TODO: This is awkward. Refactor class components. // updateQueue.capturedValues = null; - - const capturedValue: CapturedValue = (capturedValues[0]: any); - if (capturedValue.isError) { - logError(capturedValue); - } - const error = capturedValue.value; - instance.componentDidCatch(error); - + callComponentDidCatch(instance, capturedValues); newState = processUpdateQueue( null, workInProgress, @@ -673,21 +674,13 @@ export default function( let updateQueue = workInProgress.updateQueue; if (updateQueue !== null && updateQueue.capturedValues !== null) { const capturedValues = updateQueue.capturedValues; - // Don't remove these from the update queue yet. We need them in // finishClassComponent. Do the reset there. // TODO: This is awkward. Refactor class components. // updateQueue.capturedValues = null; - - const capturedValue: CapturedValue = (capturedValues[0]: any); - if (capturedValue.isError) { - logError(capturedValue); - } - const error = capturedValue.value; - instance.componentDidCatch(error); - + callComponentDidCatch(instance, capturedValues); newState = processUpdateQueue( - current, + null, workInProgress, updateQueue, instance, diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js index bc75d2cf2c4..93e3bde322d 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js @@ -93,6 +93,53 @@ describe('ReactIncrementalErrorHandling', () => { expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]); }); + it('calls componentDidCatch multiple times for multiple errors', () => { + let id = 0; + class BadMount extends React.Component { + componentDidMount() { + throw new Error(`Error ${++id}`); + } + render() { + ReactNoop.yield('BadMount'); + return null; + } + } + + class ErrorBoundary extends React.Component { + state = {errorCount: 0}; + componentDidCatch(error) { + ReactNoop.yield(`componentDidCatch: ${error.message}`); + this.setState(state => ({errorCount: state.errorCount + 1})); + } + render() { + if (this.state.errorCount > 0) { + return ; + } + ReactNoop.yield('ErrorBoundary'); + return this.props.children; + } + } + + ReactNoop.render( + + + + + , + ); + + expect(ReactNoop.flush()).toEqual([ + 'ErrorBoundary', + 'BadMount', + 'BadMount', + 'BadMount', + 'componentDidCatch: Error 1', + 'componentDidCatch: Error 2', + 'componentDidCatch: Error 3', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Number of errors: 3')]); + }); + it('catches render error in a boundary during full deferred mounting', () => { class ErrorBoundary extends React.Component { state = {error: null}; From b71fca18671e199158ad8e59281a6d871e742459 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 2 Jan 2018 17:27:03 -0800 Subject: [PATCH 6/6] Don't complete parents of children that threw --- .../__tests__/ReactErrorBoundaries-test.js | 6 +- .../src/ReactCapturedValue.js | 9 +- .../react-reconciler/src/ReactChildFiber.js | 3 + .../src/ReactFiberClassComponent.js | 1 + .../src/ReactFiberCompleteWork.js | 28 +-- .../src/ReactFiberScheduler.js | 198 +++++++++--------- .../ReactIncrementalErrorHandling-test.js | 54 +++++ 7 files changed, 170 insertions(+), 129 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js index bf6dd38fac6..0d9aad9ec04 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.js @@ -819,7 +819,7 @@ describe('ReactErrorBoundaries', () => { expect(log).toEqual(['ErrorBoundary componentWillUnmount']); }); - it('propagates errors inside boundary during componentWillMount', () => { + fit('propagates errors inside boundary during componentWillMount', () => { const container = document.createElement('div'); ReactDOM.render( @@ -909,6 +909,10 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', + // Continue rendering siblings + 'Normal constructor', + 'Normal componentWillMount', + 'Normal render', // Capture the error 'ErrorBoundary componentDidCatch', // Render the error message diff --git a/packages/react-reconciler/src/ReactCapturedValue.js b/packages/react-reconciler/src/ReactCapturedValue.js index ccfa909a057..c8083838b1d 100644 --- a/packages/react-reconciler/src/ReactCapturedValue.js +++ b/packages/react-reconciler/src/ReactCapturedValue.js @@ -28,8 +28,8 @@ export type CapturedValue = { }; // Object that is passed to the error logger module. -// TODO: This is different for legacy reasons, but I don't think it's -// exposed to anyone outside FB, so we can probably change it +// TODO: CapturedError is different from CapturedValue for legacy reasons, but I +// don't think it's exposed to anyone outside FB, so we can probably change it. export type CapturedError = { componentName: string | null, componentStack: string, @@ -40,6 +40,7 @@ export type CapturedError = { willRetry: boolean, }; +// Call this immediately after the value is thrown. export function createCapturedValue( value: T, source: Fiber | null, @@ -72,6 +73,10 @@ export function logError(capturedValue: CapturedValue): void { } } +// Create a CapturedError object from a CapturedValue before it is passed to +// the error logger. +// TODO: CapturedError is different from CapturedValue for legacy reasons, but I +// don't think it's exposed to anyone outside FB, so we can probably change it. function createCapturedError( capturedValue: CapturedValue, ): CapturedError { diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index c8bcf744692..f813f47364c 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -1184,6 +1184,9 @@ function ChildReconciler(shouldTrackSideEffects) { } if (deleteExistingChildren) { + // Schedule all the existing children for deletion. This has the + // effect of re-mounting children even if their identity matches, + // as if all the keys changed. deleteRemainingChildren(returnFiber, currentFirstChild); currentFirstChild = null; } diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.js b/packages/react-reconciler/src/ReactFiberClassComponent.js index 07e6891deba..3ac8ef478c0 100644 --- a/packages/react-reconciler/src/ReactFiberClassComponent.js +++ b/packages/react-reconciler/src/ReactFiberClassComponent.js @@ -443,6 +443,7 @@ export default function( function callComponentDidCatch(instance: any, capturedValues: Array) { for (let i = 0; i < capturedValues.length; i++) { const capturedValue: CapturedValue = (capturedValues[i]: any); + // For now, we can assume these are all errors. logError(capturedValue); const error = capturedValue.value; instance.componentDidCatch(error); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index cb0241d7b8b..a3fcaab15a0 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -32,13 +32,7 @@ import { ReturnComponent, Fragment, } from 'shared/ReactTypeOfWork'; -import { - NoEffect, - Placement, - Ref, - Update, - Err, -} from 'shared/ReactTypeOfSideEffect'; +import {Placement, Ref, Update} from 'shared/ReactTypeOfSideEffect'; import invariant from 'fbjs/lib/invariant'; import {reconcileChildFibers} from './ReactChildFiber'; @@ -51,8 +45,6 @@ export default function( config: HostConfig, hostContext: HostContext, hydrationContext: HydrationContext, - captureThrownValues: () => boolean, - captureErrors: () => boolean, ) { const { createInstance, @@ -404,29 +396,11 @@ export default function( case FunctionalComponent: return null; case ClassComponent: { - const instance = workInProgress.stateNode; - if ( - (workInProgress.effectTag & Err) === NoEffect && - instance !== null && - typeof instance.componentDidCatch === 'function' - ) { - const didCapture = captureErrors(); - if (didCapture) { - workInProgress.effectTag |= Err; - return workInProgress; - } - } - // We are leaving this subtree, so pop context if any. popContextProvider(workInProgress); return null; } case HostRoot: { - const didCapture = captureThrownValues(); - if (didCapture) { - return workInProgress; - } - popHostContainer(workInProgress); popTopLevelContextObject(workInProgress); const fiberRoot = (workInProgress.stateNode: FiberRoot); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index e49db71ab06..dc9560734e3 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -17,6 +17,7 @@ import type {CapturedValue} from './ReactCapturedValue'; import ReactErrorUtils from 'shared/ReactErrorUtils'; import {ReactCurrentOwner} from 'shared/ReactGlobalSharedState'; import { + NoEffect, PerformedWork, Placement, Update, @@ -163,8 +164,6 @@ export default function( config, hostContext, hydrationContext, - captureThrownValues, - captureErrors, ); const { commitResetTextContent, @@ -484,7 +483,43 @@ export default function( workInProgress.expirationTime = newExpirationTime; } - function completeUnitOfWork(workInProgress: Fiber): Fiber | null { + function captureThrownValues(): boolean { + if (thrownValues === null) { + return false; + } + capturedValues = thrownValues; + // Reset the list of thrown values, now that they've been captured. + thrownValues = null; + return true; + } + + function captureErrors(): boolean { + if (thrownValues === null) { + return false; + } + let errors = null; + for (let i = 0; i < thrownValues.length; i++) { + const value = thrownValues[i]; + if (value.isError) { + thrownValues.splice(i, 1); + if (errors === null) { + errors = [value]; + } else { + errors.push(value); + } + } + } + if (thrownValues.length === 0) { + thrownValues = null; + } + capturedValues = errors; + return true; + } + + function unwindUnitOfWork(workInProgress: Fiber): Fiber | null { + // Attempt to complete the current unit of work, then move to the + // next sibling. If there are no more siblings, return to the + // parent fiber. while (true) { // The current, flushed, state of this fiber is the alternate. // Ideally nothing should rely on this, but relying on it here @@ -494,19 +529,11 @@ export default function( if (__DEV__) { ReactDebugCurrentFiber.setCurrentFiber(workInProgress); } - const next = completeWork( - current, - workInProgress, - nextRenderExpirationTime, - ); - if (__DEV__) { - ReactDebugCurrentFiber.resetCurrentFiber(); - } - const returnFiber = workInProgress.return; - const siblingFiber = workInProgress.sibling; - - if (next !== workInProgress) { + let next; + if (thrownValues === null) { + // This fiber completed. + next = completeWork(current, workInProgress, nextRenderExpirationTime); if (workInProgress.effectTag & Err) { // Restarting an error boundary stopFailedWorkTimer(workInProgress); @@ -515,10 +542,54 @@ export default function( } resetExpirationTime(workInProgress, nextRenderExpirationTime); } else { - // This fiber did not complete. + // This fiber did not complete because something threw. Pop values off + // the stack without entering the complete phase. If this is a boundary, + // capture values if possible. + popContexts(workInProgress); + switch (workInProgress.tag) { + case ClassComponent: { + const instance = workInProgress.stateNode; + if ( + (workInProgress.effectTag & Err) === NoEffect && + instance !== null && + typeof instance.componentDidCatch === 'function' + ) { + const didCapture = captureErrors(); + if (didCapture) { + workInProgress.effectTag |= Err; + // Render the boundary again + next = workInProgress; + } else { + next = null; + } + } else { + next = null; + } + break; + } + case HostRoot: + const didCapture = captureThrownValues(); + if (didCapture) { + // Render the boundary again + next = workInProgress; + } else { + next = null; + } + break; + default: + next = null; + } + // Because this fiber did not complete, don't reset its expiration time. stopWorkTimer(workInProgress); } + if (__DEV__) { + ReactDebugCurrentFiber.resetCurrentFiber(); + } + + const returnFiber = workInProgress.return; + const siblingFiber = workInProgress.sibling; + if (next !== null) { stopWorkTimer(workInProgress); if (__DEV__ && ReactFiberInstrumentation.debugTool) { @@ -616,7 +687,7 @@ export default function( if (next === null) { // If this doesn't spawn new work, complete the current work. - next = completeUnitOfWork(workInProgress); + next = unwindUnitOfWork(workInProgress); } ReactCurrentOwner.current = null; @@ -645,32 +716,6 @@ export default function( } } - function unwindToNearestErrorBoundary(sourceFiber) { - popContexts(sourceFiber); - let boundaryFiber = null; - let fiber = sourceFiber.return; - traversal: while (fiber !== null) { - switch (fiber.tag) { - case ClassComponent: - const instance = fiber.stateNode; - if ( - typeof instance.componentDidCatch === 'function' && - !(fiber.effectTag & Err) - ) { - boundaryFiber = fiber; - break traversal; - } - break; - case HostRoot: - boundaryFiber = fiber; - break; - } - popContexts(fiber); - fiber = fiber.return; - } - return boundaryFiber; - } - function renderRoot( root: FiberRoot, expirationTime: ExpirationTime, @@ -745,29 +790,18 @@ export default function( } else { thrownValues.push(capturedValue); } - if (capturedValue.isError) { - const boundaryFiber = unwindToNearestErrorBoundary(sourceFiber); - capturedValue.boundary = boundaryFiber; - if (boundaryFiber !== null) { - nextUnitOfWork = completeUnitOfWork(boundaryFiber); - } else { - // The root failed to render. This is a fatal error. - hasUncaughtError = true; - firstUncaughtError = thrownValue; - break; - } + + // Move to next sibling, or return to parent + popContexts(sourceFiber); + if (sourceFiber.sibling !== null) { + nextUnitOfWork = sourceFiber.sibling; + } else if (sourceFiber.return !== null) { + nextUnitOfWork = unwindUnitOfWork(sourceFiber.return); } else { - // Move to next sibling, or return to parent - if (sourceFiber.sibling !== null) { - nextUnitOfWork = sourceFiber.sibling; - } else if (sourceFiber.return !== null) { - nextUnitOfWork = completeUnitOfWork(sourceFiber.return); - } else { - // The root failed to render. This is a fatal error. - hasUncaughtError = true; - firstUncaughtError = thrownValue; - break; - } + // The root failed to render. This is a fatal error. + hasUncaughtError = true; + firstUncaughtError = thrownValue; + break; } } while (true); @@ -806,40 +840,6 @@ export default function( stopWorkTimer(workInProgress); } - // Called during complete phase - function captureThrownValues(): boolean { - if (thrownValues === null) { - return false; - } - capturedValues = thrownValues; - // Reset the list of thrown values, now that they've been captured. - thrownValues = null; - return true; - } - - function captureErrors(): boolean { - if (thrownValues === null) { - return false; - } - let errors = null; - for (let i = 0; i < thrownValues.length; i++) { - const value = thrownValues[i]; - if (value.isError) { - thrownValues.splice(i, 1); - if (errors === null) { - errors = [value]; - } else { - errors.push(value); - } - } - } - if (thrownValues.length === 0) { - thrownValues = null; - } - capturedValues = errors; - return true; - } - function markUncaughtError(error: mixed): void { hasUncaughtError = true; firstUncaughtError = error; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js index 93e3bde322d..a4dd36985bc 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.js @@ -1105,4 +1105,58 @@ describe('ReactIncrementalErrorHandling', () => { expect(caught).toBe(notAnError); expect(ops).toEqual(['ErrorBoundary (try)', 'Indirection', 'BadRender']); }); + + it('continues working on siblings of a component that throws', () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + componentDidCatch(error) { + ReactNoop.yield('componentDidCatch'); + this.setState({error}); + } + render() { + if (this.state.error) { + ReactNoop.yield('ErrorBoundary (catch)'); + return ; + } + ReactNoop.yield('ErrorBoundary (try)'); + return this.props.children; + } + } + + function ErrorMessage(props) { + ReactNoop.yield('ErrorMessage'); + return ; + } + + function BadRenderSibling(props) { + ReactNoop.yield('BadRenderSibling'); + return null; + } + + function BadRender() { + ReactNoop.yield('throw'); + throw new Error('oops!'); + } + + ReactNoop.render( + + + + + , + ); + + expect(ReactNoop.flush()).toEqual([ + 'ErrorBoundary (try)', + 'throw', + // Continue rendering siblings after BadRender throws + 'BadRenderSibling', + 'BadRenderSibling', + // Recover from the error + 'componentDidCatch', + 'ErrorBoundary (catch)', + 'ErrorMessage', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Caught an error: oops!')]); + }); });