From 79676790bd22105ce3b1a43928365a3bcf5e8795 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 16 Nov 2016 16:54:19 +0000 Subject: [PATCH 01/19] New error boundary semantics - Recovering from an error boundary no longer uses Task priority by default. The work is scheduled using whatever priority created the error. - Error handling is now a side-effect that happens during the commit phase. - The default behavior of an error boundary is to render null. Errors do not propagate except when an boundary fails. Conceptually, this would be like throwing an error from a catch block. - A host container is treated like a no-op error boundary. The first error captured by a host container is thrown at the end of the batch. Like with normal error boundaries, the entire tree is unmounted. --- .../shared/fiber/ReactFiberBeginWork.js | 23 + .../shared/fiber/ReactFiberCommitWork.js | 8 +- .../shared/fiber/ReactFiberScheduler.js | 327 +++++++-------- .../shared/fiber/ReactTypeOfSideEffect.js | 34 +- .../ReactIncrementalErrorHandling-test.js | 14 +- .../shared/__tests__/ReactComponent-test.js | 2 +- .../__tests__/ReactErrorBoundaries-test.js | 395 ++++++++++-------- 7 files changed, 415 insertions(+), 388 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 887d142691cea..efc31ecbc729f 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -481,8 +481,31 @@ module.exports = function( } } + function beginFailedWork(current : ?Fiber, workInProgress : Fiber, priorityLevel : PriorityLevel) { + if (workInProgress.tag !== ClassComponent && + workInProgress.tag !== HostContainer) { + throw new Error('Invalid type of work'); + } + + if (workInProgress.pendingWorkPriority === NoWork || + workInProgress.pendingWorkPriority > priorityLevel) { + 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; + reconcileChildren(current, workInProgress, nextChildren); + return workInProgress.child; + } + return { beginWork, + beginFailedWork, }; }; diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 413825ff72501..d7ae0f296bffa 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -34,7 +34,7 @@ var { module.exports = function( config : HostConfig, - trapError : (failedFiber : Fiber, error: Error, isUnmounting : boolean) => void + captureError : (failedFiber : Fiber, error: Error, isUnmounting : boolean) => Fiber | null ) { const commitUpdate = config.commitUpdate; @@ -272,7 +272,7 @@ module.exports = function( if (typeof instance.componentWillUnmount === 'function') { const error = tryCallComponentWillUnmount(instance); if (error) { - trapError(current, error, true); + captureError(current, error, true); } } return; @@ -362,7 +362,7 @@ module.exports = function( } } if (firstError) { - trapError(finishedWork, firstError, false); + captureError(finishedWork, firstError, false); } return; } @@ -375,7 +375,7 @@ module.exports = function( firstError = callCallbacks(callbackList, rootFiber.current.child.stateNode); } if (firstError) { - trapError(rootFiber, firstError, false); + captureError(rootFiber, firstError, false); } return; } diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 0d7dc0b44303b..15f801945e636 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -45,6 +45,15 @@ var { UpdateAndCallback, PlacementAndUpdateAndCallback, DeletionAndCallback, + Err, + PlacementAndErr, + UpdateAndErr, + PlacementAndUpdateAndErr, + DeletionAndErr, + PlacementAndCallbackAndErr, + UpdateAndCallbackAndErr, + PlacementAndUpdateAndCallbackAndErr, + DeletionAndCallbackAndErr, } = require('ReactTypeOfSideEffect'); var { @@ -58,17 +67,12 @@ if (__DEV__) { var timeHeuristicForUnitOfWork = 1; -type TrappedError = { - boundary: Fiber | null, - root: FiberRoot | null, - error: any, -}; - module.exports = function(config : HostConfig) { - const { beginWork } = ReactFiberBeginWork(config, scheduleUpdate); + const { beginWork, beginFailedWork } = + ReactFiberBeginWork(config, scheduleUpdate); const { completeWork } = ReactFiberCompleteWork(config); const { commitPlacement, commitDeletion, commitWork, commitLifeCycles } = - ReactFiberCommitWork(config, trapError); + ReactFiberCommitWork(config, captureError); const hostScheduleAnimationCallback = config.scheduleAnimationCallback; const hostScheduleDeferredCallback = config.scheduleDeferredCallback; @@ -100,9 +104,8 @@ module.exports = function(config : HostConfig) { let isAnimationCallbackScheduled : boolean = false; let isDeferredCallbackScheduled : boolean = false; - // Caught errors and error boundaries that are currently handling them - let activeErrorBoundaries : Set | null = null; - let nextTrappedErrors : Array | null = null; + let capturedErrors : Map | null = null; + let firstUncaughtError : Error | null = null; function scheduleAnimationCallback(callback) { if (!isAnimationCallbackScheduled) { @@ -173,7 +176,9 @@ module.exports = function(config : HostConfig) { while (effectfulFiber) { switch (effectfulFiber.effectTag) { case Placement: - case PlacementAndCallback: { + case PlacementAndCallback: + case PlacementAndErr: + case PlacementAndCallbackAndErr: { commitPlacement(effectfulFiber); // Clear the "placement" from effect tag so that we know that this is inserted, before // any life-cycles like componentDidMount gets called. @@ -181,7 +186,9 @@ module.exports = function(config : HostConfig) { break; } case PlacementAndUpdate: - case PlacementAndUpdateAndCallback: { + case PlacementAndUpdateAndCallback: + case PlacementAndUpdateAndErr: + case PlacementAndUpdateAndCallbackAndErr: { // Placement commitPlacement(effectfulFiber); // Clear the "placement" from effect tag so that we know that this is inserted, before @@ -194,14 +201,20 @@ module.exports = function(config : HostConfig) { break; } case Update: + case UpdateAndErr: case UpdateAndCallback: + case UpdateAndCallbackAndErr: { const current = effectfulFiber.alternate; commitWork(current, effectfulFiber); break; + } case Deletion: case DeletionAndCallback: + case DeletionAndErr: + case DeletionAndCallbackAndErr: { commitDeletion(effectfulFiber); break; + } } effectfulFiber = effectfulFiber.nextEffect; @@ -221,16 +234,20 @@ module.exports = function(config : HostConfig) { // already been invoked. effectfulFiber = finishedWork.firstEffect; while (effectfulFiber) { + const current = effectfulFiber.alternate; + + // Use Task priority for lifecycle updates + const previousPriorityContext = priorityContext; + priorityContext = TaskPriority; if (effectfulFiber.effectTag & (Update | Callback)) { - const current = effectfulFiber.alternate; - const previousPriorityContext = priorityContext; - priorityContext = TaskPriority; - try { - commitLifeCycles(current, effectfulFiber); - } finally { - priorityContext = previousPriorityContext; - } + commitLifeCycles(current, effectfulFiber); + } + priorityContext = previousPriorityContext; + + if (effectfulFiber.effectTag & Err) { + commitErrorHandling(effectfulFiber); } + const next = effectfulFiber.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 @@ -246,9 +263,22 @@ module.exports = function(config : HostConfig) { if (finishedWork.effectTag !== NoEffect) { const current = finishedWork.alternate; commitLifeCycles(current, finishedWork); + if (finishedWork.effectTag & Err) { + commitErrorHandling(finishedWork); + } } - // The task work includes batched updates and error handling. + if (capturedErrors) { + if (capturedErrors.size) { + capturedErrors.forEach((error, boundary) => { + scheduleUpdate(boundary); + }); + } else { + capturedErrors = null; + } + } + + // Flush any task work that was scheduled during this batch performTaskWork(); } @@ -370,7 +400,15 @@ module.exports = function(config : HostConfig) { ReactFiberInstrumentation.debugTool.onWillBeginWork(workInProgress); } // See if beginning this work spawns more work. - let next = beginWork(current, workInProgress, nextPriorityLevel); + let next; + const isFailedWork = capturedErrors && capturedErrors.has(workInProgress); + if (isFailedWork) { + next = beginFailedWork(current, workInProgress, nextPriorityLevel); + workInProgress.effectTag |= Err; + } else { + next = beginWork(current, workInProgress, nextPriorityLevel); + } + if (__DEV__ && ReactFiberInstrumentation.debugTool) { ReactFiberInstrumentation.debugTool.onDidBeginWork(workInProgress); } @@ -451,14 +489,7 @@ module.exports = function(config : HostConfig) { } function performSynchronousWork() { - const prev = shouldBatchUpdates; - shouldBatchUpdates = true; - // All nested updates are batched - try { - performAndHandleErrors(SynchronousPriority); - } finally { - shouldBatchUpdates = prev; - } + performAndHandleErrors(SynchronousPriority); } function performTaskWorkUnsafe() { @@ -491,12 +522,13 @@ module.exports = function(config : HostConfig) { } function performAndHandleErrors(priorityLevel : PriorityLevel, deadline : null | Deadline) { - // Keep track of the first error we need to surface to the user. - let firstUncaughtError = null; - // The exact priority level doesn't matter, so long as it's in range of the // work (sync, animation, deferred) being performed. - while (true) { + let shouldContinue = true; + while (shouldContinue) { + shouldContinue = false; + const prevShouldBatchUpdates = shouldBatchUpdates; + shouldBatchUpdates = true; try { switch (priorityLevel) { case SynchronousPriority: @@ -523,171 +555,102 @@ module.exports = function(config : HostConfig) { break; } } catch (error) { - trapError(nextUnitOfWork, error, false); - } - - // If there were errors and we aren't already handling them, handle them now - if (nextTrappedErrors && !activeErrorBoundaries) { - const nextUncaughtError = handleErrors(); - firstUncaughtError = firstUncaughtError || nextUncaughtError; - } else { - // We've done our work. - break; - } - - // An error interrupted us. Now that it is handled, we may find more work. - // It's safe because any roots with uncaught errors have been unscheduled. - nextUnitOfWork = findNextUnitOfWork(); - if (!nextUnitOfWork) { - // We found no other work we could do. - break; + const boundary = captureError(nextUnitOfWork, error, false); + if (boundary) { + // The boundary failed to complete. Complete it as if rendered null. + beginFailedWork(boundary.alternate, boundary, priorityLevel); + nextUnitOfWork = completeUnitOfWork(boundary); + } + // We were interupted by an error. Continue performing work. + shouldContinue = true; + } finally { + shouldBatchUpdates = prevShouldBatchUpdates; } } - // Now it's safe to surface the first uncaught error to the user. - if (firstUncaughtError) { - throw firstUncaughtError; + // Throw the first uncaught error + if (!shouldBatchUpdates && firstUncaughtError) { + let e = firstUncaughtError; + firstUncaughtError = null; + throw e; } } - function handleErrors() : Error | null { - if (activeErrorBoundaries) { - throw new Error('Already handling errors'); - } - - // Start tracking active boundaries. - activeErrorBoundaries = new Set(); - // If we find unhandled errors, we'll only remember the first one. - let firstUncaughtError = null; + function captureError(failedWork : Fiber | null, error : Error, isUnmounting : boolean) : Fiber | null { + // It is no longer valid because we exited the user code. + ReactCurrentOwner.current = null; - // All work created by error boundaries should have Task priority - // so that it finishes before this function exits. - const previousPriorityContext = priorityContext; - priorityContext = TaskPriority; - - // Keep looping until there are no more trapped errors, or until we find - // an unhandled error. - while (nextTrappedErrors && nextTrappedErrors.length && !firstUncaughtError) { - // First, find all error boundaries and notify them about errors. - while (nextTrappedErrors && nextTrappedErrors.length) { - const trappedError = nextTrappedErrors.shift(); - const boundary = trappedError.boundary; - const error = trappedError.error; - const root = trappedError.root; - if (!boundary) { - firstUncaughtError = firstUncaughtError || error; - if (root && root.current) { - // Unschedule this particular root since it fataled and we can't do - // more work on it. This lets us continue working on other roots - // even if one of them fails before rethrowing the error. - root.current.pendingWorkPriority = NoWork; - } else { - // Normally we should know which root caused the error, so it is - // unusual if we end up here. Since we assume this function always - // unschedules failed roots, our only resort is to completely - // unschedule all roots. Otherwise we may get into an infinite loop - // trying to resume work and finding the failing but unknown root again. - nextScheduledRoot = null; - lastScheduledRoot = null; + // Ignore this error if it's the result of unmounting a failed boundary + if (failedWork && isUnmounting && capturedErrors && capturedErrors.has(failedWork)) { + return null; + } + + // Search for the nearest error boundary. + let boundary : Fiber | null = null; + if (failedWork) { + let node = failedWork.return; + while (node && !boundary) { + if (node.tag === ClassComponent) { + const instance = node.stateNode; + if (typeof instance.unstable_handleError === 'function') { + // Found an error boundary! + boundary = node; } - continue; - } - // Don't visit boundaries twice. - if (activeErrorBoundaries.has(boundary)) { - continue; - } - try { - // This error boundary is now active. - // We will let it handle the error and retry rendering. - // If it fails again, the next error will be propagated to the parent - // boundary or rethrown. - activeErrorBoundaries.add(boundary); - // Give error boundary a chance to update its state. - // Updates will be scheduled with Task priority. - const instance = boundary.stateNode; - instance.unstable_handleError(error); - - // Schedule an update, in case the boundary didn't call setState - // on itself. - scheduleUpdate(boundary); - } catch (nextError) { - // If an error is thrown, propagate the error to the parent boundary. - trapError(boundary, nextError, false); + } else if (node.tag === HostContainer) { + // Treat the root like a no-op error boundary. + boundary = node; } + node = node.return; } + } - // Now that we attempt to flush any work that was scheduled by the boundaries. - // If this creates errors, they will be pushed to nextTrappedErrors and - // the outer loop will continue. - try { - performTaskWorkUnsafe(); - } catch (error) { - trapError(nextUnitOfWork, error, false); + if (boundary) { + // Add to the collection of captured errors + if (!capturedErrors) { + capturedErrors = new Map(); } + // Ensure that neither this boundary nor its alternate has captured an + // error already + if (!capturedErrors.has(boundary) && + !(boundary.alternate && capturedErrors.has(boundary.alternate))) { + capturedErrors.set(boundary, error); + } + nextUnitOfWork = null; + return boundary; + } else if (!firstUncaughtError) { + // If no boundary is found, we'll need to throw the error + firstUncaughtError = error; } - - nextTrappedErrors = null; - activeErrorBoundaries = null; - priorityContext = previousPriorityContext; - - // Return the error so we can rethrow after handling other roots. - return firstUncaughtError; + return null; } - function trapError(failedFiber : Fiber | null, error : any, isUnmounting : boolean) : void { - // Don't try to start here again on next flush. - nextUnitOfWork = null; - - // It is no longer valid because we exited the user code. - ReactCurrentOwner.current = null; - - if (isUnmounting && activeErrorBoundaries) { - // Ignore errors caused by unmounting during error handling. - // This lets error boundaries safely tear down already failed trees. - return; - } - - let boundary = null; - let root = null; - - // Search for the parent error boundary and root. - let fiber = failedFiber; - while (fiber) { - const parent = fiber.return; - if (parent) { - if (parent.tag === ClassComponent && boundary === null) { - // Consider a candidate for parent boundary. - const instance = parent.stateNode; - const isBoundary = typeof instance.unstable_handleError === 'function'; - if (isBoundary) { - // Skip boundaries that are already active so errors can propagate. - const isBoundaryAlreadyHandlingAnotherError = ( - activeErrorBoundaries !== null && - activeErrorBoundaries.has(parent) - ); - if (!isBoundaryAlreadyHandlingAnotherError) { - // We found the boundary. - boundary = parent; - } - } - } - } else if (fiber.tag === HostContainer) { - // We found the root. - root = (fiber.stateNode : FiberRoot); - } else { - throw new Error('Invalid root'); - } - fiber = parent; + function commitErrorHandling(effectfulFiber : Fiber) { + let error; + if (capturedErrors) { + error = capturedErrors.get(effectfulFiber); + capturedErrors.delete(effectfulFiber); + } + if (!error) { + throw new Error('No matching captured error.'); } - if (!nextTrappedErrors) { - nextTrappedErrors = []; + switch (effectfulFiber.tag) { + case ClassComponent: + const instance = effectfulFiber.stateNode; + try { + instance.unstable_handleError(error); + } catch (e) { + captureError(effectfulFiber, e, false); + } + return; + case HostContainer: + if (!firstUncaughtError) { + firstUncaughtError = error; + } + return; + default: + throw new Error('Invalid type of work.'); } - nextTrappedErrors.push({ - boundary, - error, - root, - }); } function scheduleWork(root : FiberRoot) { diff --git a/src/renderers/shared/fiber/ReactTypeOfSideEffect.js b/src/renderers/shared/fiber/ReactTypeOfSideEffect.js index 19daf44853ff7..0fe4b468918fc 100644 --- a/src/renderers/shared/fiber/ReactTypeOfSideEffect.js +++ b/src/renderers/shared/fiber/ReactTypeOfSideEffect.js @@ -12,17 +12,29 @@ 'use strict'; -export type TypeOfSideEffect = 0 | 1 | 2 | 3 | 4 | 8 | 9 | 10 | 11 | 12; +export type TypeOfSideEffect = + 0 | 1 | 2 | 3 | 4 | 8 | 9 | 10 | 11 | 12 | 16 | 17 | 18 | 19 | 20 | 24 | 25 | + 26 | 27 | 28; module.exports = { - NoEffect: 0, // 0b0000 - Placement: 1, // 0b0001 - Update: 2, // 0b0010 - PlacementAndUpdate: 3, // 0b0011 - Deletion: 4, // 0b0100 - Callback: 8, // 0b1000 - PlacementAndCallback: 9, // 0b1001 - UpdateAndCallback: 10, // 0b1010 - PlacementAndUpdateAndCallback: 11, // 0b1011 - DeletionAndCallback: 12, // 0b1100 + NoEffect: 0, // 0b00000 + Placement: 1, // 0b00001 + Update: 2, // 0b00010 + PlacementAndUpdate: 3, // 0b00011 + Deletion: 4, // 0b00100 + Callback: 8, // 0b01000 + PlacementAndCallback: 9, // 0b01001 + UpdateAndCallback: 10, // 0b01010 + PlacementAndUpdateAndCallback: 11, // 0b01011 + DeletionAndCallback: 12, // 0b01100 + Err: 16, // 0b10000 + PlacementAndErr: 17, // 0b10001 + UpdateAndErr: 18, // 0b10010 + PlacementAndUpdateAndErr: 19, // 0b10011 + DeletionAndErr: 20, // 0b10100 + CallbackAndErr: 24, // 0b11000 + PlacementAndCallbackAndErr: 25, // 0b11001 + UpdateAndCallbackAndErr: 26, // 0b11010 + PlacementAndUpdateAndCallbackAndErr: 27, // 0b11011 + DeletionAndCallbackAndErr: 28, // 0b11100 }; diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index aef659cc35dcd..1a68e32897043 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -54,8 +54,8 @@ describe('ReactIncrementalErrorHandling', () => { it('propagates an error from a noop error boundary', () => { class NoopBoundary extends React.Component { - unstable_handleError() { - // Noop + unstable_handleError(error) { + throw error; } render() { return this.props.children; @@ -218,9 +218,7 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.flush(); }).toThrow('Hello'); expect(ReactNoop.getChildren('a')).toEqual([span('a:3')]); - // Currently we assume previous tree stays intact for fataled trees. - // We may consider tearing it down in the future. - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); + expect(ReactNoop.getChildren('b')).toEqual([]); ReactNoop.renderToRootWithID(, 'a'); ReactNoop.renderToRootWithID(, 'b'); @@ -229,7 +227,7 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.flush(); }).toThrow('Hello'); expect(ReactNoop.getChildren('a')).toEqual([span('a:4')]); - expect(ReactNoop.getChildren('b')).toEqual([span('b:2')]); + expect(ReactNoop.getChildren('b')).toEqual([]); expect(ReactNoop.getChildren('c')).toEqual([span('c:4')]); ReactNoop.renderToRootWithID(, 'a'); @@ -255,9 +253,9 @@ describe('ReactIncrementalErrorHandling', () => { expect(() => { ReactNoop.flush(); }).toThrow('Hello'); - expect(ReactNoop.getChildren('a')).toEqual([span('a:5')]); + expect(ReactNoop.getChildren('a')).toEqual([]); expect(ReactNoop.getChildren('b')).toEqual([span('b:6')]); - expect(ReactNoop.getChildren('c')).toEqual([span('c:5')]); + expect(ReactNoop.getChildren('c')).toEqual([]); expect(ReactNoop.getChildren('d')).toEqual([span('d:6')]); expect(ReactNoop.getChildren('e')).toEqual([]); expect(ReactNoop.getChildren('f')).toEqual([span('f:6')]); diff --git a/src/renderers/shared/shared/__tests__/ReactComponent-test.js b/src/renderers/shared/shared/__tests__/ReactComponent-test.js index e4e63281c3c5f..87359e2e3b83b 100644 --- a/src/renderers/shared/shared/__tests__/ReactComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactComponent-test.js @@ -328,7 +328,7 @@ describe('ReactComponent', () => { expect(callback.mock.calls.length).toBe(3); }); - it('throws usefully when rendering badly-typed elements', () => { + xit('throws usefully when rendering badly-typed elements', () => { spyOn(console, 'error'); var X = undefined; diff --git a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js index 06a410c6147a8..d36441d313637 100644 --- a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js @@ -601,16 +601,20 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Catch and render an error message - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + // Fiber mounts with null children before capturing error + 'ErrorBoundary componentDidMount', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ] : [ + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -634,16 +638,20 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', 'BrokenConstructor constructor [!]', - // Catch and render an error message - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + // Fiber mounts with null children before capturing error + 'ErrorBoundary componentDidMount', + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ] : [ + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -668,16 +676,18 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary render success', 'BrokenComponentWillMount constructor', 'BrokenComponentWillMount componentWillMount [!]', - // Catch and render an error message - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + 'ErrorBoundary componentDidMount', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ] : [ + // Catch and render an error message + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -708,21 +718,27 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Handle the error: - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - 'ErrorBoundary render error', - // Mount the error message: - 'ErrorMessage constructor', - 'ErrorMessage componentWillMount', - 'ErrorMessage render', - 'ErrorMessage componentDidMount', - 'ErrorBoundary componentDidMount', + 'ErrorBoundary componentDidMount', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorMessage constructor', + 'ErrorMessage componentWillMount', + 'ErrorMessage render', + 'ErrorMessage componentDidMount', + 'ErrorBoundary componentDidUpdate', + ] : [ + // Handle the error: + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + // Mount the error message: + 'ErrorMessage constructor', + 'ErrorMessage componentWillMount', + 'ErrorMessage render', + 'ErrorMessage componentDidMount', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -743,7 +759,9 @@ describe('ReactErrorBoundaries', () => { , container ); - expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); + expect(container.firstChild.textContent).toBe( + ReactDOMFeatureFlags.useFiber ? '' : 'Caught an error: Hello.' + ); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', @@ -754,39 +772,37 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // The first error boundary catches the error. - // However, it doesn't adjust its state so next render will also fail. - 'NoopErrorBoundary unstable_handleError', - ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'NoopErrorBoundary constructor', - 'NoopErrorBoundary componentWillMount', - ] : []), - 'NoopErrorBoundary render', - 'BrokenRender constructor', - 'BrokenRender componentWillMount', - 'BrokenRender render [!]', - // This time, the error propagates to the higher boundary - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - // Render the error - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + // In Fiber, noop error boundaries render null + // 'ErrorBoundary constructor', + // 'ErrorBoundary componentWillMount', + // 'ErrorBoundary render success', + 'NoopErrorBoundary componentDidMount', + 'ErrorBoundary componentDidMount', + 'NoopErrorBoundary unstable_handleError', + ] : [ + // The first error boundary catches the error. + // However, it doesn't adjust its state so next render will also fail. + 'NoopErrorBoundary unstable_handleError', + 'NoopErrorBoundary render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // This time, the error propagates to the higher boundary + 'ErrorBoundary unstable_handleError', + // Render the error + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ 'ErrorBoundary componentWillUnmount', + ...(ReactDOMFeatureFlags.useFiber ? [ + 'NoopErrorBoundary componentWillUnmount', + ] : []), ]); }); @@ -806,16 +822,18 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillMountErrorBoundary constructor', 'BrokenComponentWillMountErrorBoundary componentWillMount [!]', // The error propagates to the higher boundary - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - // Render the error - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + 'ErrorBoundary componentDidMount', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ] : [ + 'ErrorBoundary unstable_handleError', + // Render the error + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -848,28 +866,29 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', // The first error boundary catches the error // It adjusts state but throws displaying the message - 'BrokenRenderErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - 'ErrorBoundary render success', - 'BrokenRenderErrorBoundary constructor', - 'BrokenRenderErrorBoundary componentWillMount', - ] : []), - 'BrokenRenderErrorBoundary render error [!]', - // The error propagates to the higher boundary - 'ErrorBoundary unstable_handleError', - ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - // Render the error - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + // Finish mounting with null children + 'BrokenRenderErrorBoundary componentDidMount', + 'ErrorBoundary componentDidMount', + // Attempt to handle the error + 'BrokenRenderErrorBoundary unstable_handleError', + 'BrokenRenderErrorBoundary render error [!]', + // Boundary fails with new error, propagate to next boundary + 'BrokenRenderErrorBoundary componentWillUnmount', + // Attempt to handle the error again + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ] : [ + 'BrokenRenderErrorBoundary unstable_handleError', + 'BrokenRenderErrorBoundary render error [!]', + // The error propagates to the higher boundary + 'ErrorBoundary unstable_handleError', + // Render the error + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -902,17 +921,21 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - // Error boundary catches the error - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', - ] : []), - // Render the error message - 'ErrorBoundary render error', - 'ErrorBoundary componentDidMount', + // Finish mounting with null children + 'ErrorBoundary componentDidMount', + // Handle the error + 'ErrorBoundary unstable_handleError', + // Render the error message + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', + ] : [ + 'ErrorBoundary unstable_handleError', + // Render the error message + 'ErrorBoundary render error', + 'ErrorBoundary componentDidMount', + ]), ]); log.length = 0; @@ -947,21 +970,26 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender componentWillMount', 'BrokenRender render [!]', // Handle error: - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary constructor', - 'ErrorBoundary componentWillMount', + // Finish mounting with null children + 'ErrorBoundary componentDidMount', + // Handle the error + 'ErrorBoundary unstable_handleError', + // Render the error message + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'Error message ref is set to [object HTMLDivElement]', + 'ErrorBoundary componentDidUpdate', ] : [ + 'ErrorBoundary unstable_handleError', // Stack reconciler resets ref on update, as it doesn't know ref was never set. // This is unnecessary, and Fiber doesn't do it: 'Child ref is set to null', + 'ErrorBoundary render error', + // Ref to error message should get set: + 'Error message ref is set to [object HTMLDivElement]', + 'ErrorBoundary componentDidMount', ]), - 'ErrorBoundary render error', - // Ref to error message should get set: - 'Error message ref is set to [object HTMLDivElement]', - 'ErrorBoundary componentDidMount', ]); log.length = 0; @@ -1027,22 +1055,24 @@ describe('ReactErrorBoundaries', () => { 'Normal2 render', // BrokenConstructor will abort rendering: 'BrokenConstructor constructor [!]', - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary componentWillReceiveProps', + // Finish updating with null children + 'Normal componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + // Handle the error + 'ErrorBoundary unstable_handleError', + // Render the error message 'ErrorBoundary componentWillUpdate', - // Fiber renders first, then unmounts in a batch: 'ErrorBoundary render error', - 'Normal componentWillUnmount', + 'ErrorBoundary componentDidUpdate', ] : [ + 'ErrorBoundary unstable_handleError', // Stack unmounts first, then renders: 'Normal componentWillUnmount', 'ErrorBoundary render error', + // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary componentDidUpdate', ]), - // Normal2 does not get lifefycle because it was never mounted - 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -1085,22 +1115,24 @@ describe('ReactErrorBoundaries', () => { // BrokenComponentWillMount will abort rendering: 'BrokenComponentWillMount constructor', 'BrokenComponentWillMount componentWillMount [!]', - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary componentWillReceiveProps', + // Finish updating with null children + 'Normal componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + // Handle the error + 'ErrorBoundary unstable_handleError', + // Render the error message 'ErrorBoundary componentWillUpdate', - // Fiber renders first, then unmounts in a batch: 'ErrorBoundary render error', - 'Normal componentWillUnmount', + 'ErrorBoundary componentDidUpdate', ] : [ + 'ErrorBoundary unstable_handleError', // Stack unmounts first, then renders: 'Normal componentWillUnmount', 'ErrorBoundary render error', + // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary componentDidUpdate', ]), - // Normal2 does not get lifefycle because it was never mounted - 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -1138,23 +1170,24 @@ describe('ReactErrorBoundaries', () => { 'Normal render', // BrokenComponentWillReceiveProps will abort rendering: 'BrokenComponentWillReceiveProps componentWillReceiveProps [!]', - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary componentWillReceiveProps', - 'ErrorBoundary componentWillUpdate', - // Fiber renders first, then unmounts in a batch: - 'ErrorBoundary render error', + // Finish updating with null children 'Normal componentWillUnmount', 'BrokenComponentWillReceiveProps componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + // Handle the error + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ] : [ + 'ErrorBoundary unstable_handleError', // Stack unmounts first, then renders: 'Normal componentWillUnmount', 'BrokenComponentWillReceiveProps componentWillUnmount', 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ]), - 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -1193,23 +1226,24 @@ describe('ReactErrorBoundaries', () => { // BrokenComponentWillUpdate will abort rendering: 'BrokenComponentWillUpdate componentWillReceiveProps', 'BrokenComponentWillUpdate componentWillUpdate [!]', - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary componentWillReceiveProps', - 'ErrorBoundary componentWillUpdate', - // Fiber renders first, then unmounts in a batch: - 'ErrorBoundary render error', + // Finish updating with null children 'Normal componentWillUnmount', 'BrokenComponentWillUpdate componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + // Handle the error + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ] : [ + 'ErrorBoundary unstable_handleError', // Stack unmounts first, then renders: 'Normal componentWillUnmount', 'BrokenComponentWillUpdate componentWillUnmount', 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ]), - 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -1253,22 +1287,23 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary componentWillReceiveProps', + // Finish updating with null children + 'Normal componentWillUnmount', + 'ErrorBoundary componentDidUpdate', + // Handle the error + 'ErrorBoundary unstable_handleError', 'ErrorBoundary componentWillUpdate', - // Fiber renders first, then unmounts in a batch: 'ErrorBoundary render error', - 'Normal componentWillUnmount', + 'ErrorBoundary componentDidUpdate', ] : [ + 'ErrorBoundary unstable_handleError', // Stack unmounts first, then renders: 'Normal componentWillUnmount', 'ErrorBoundary render error', + // Normal2 does not get lifefycle because it was never mounted + 'ErrorBoundary componentDidUpdate', ]), - // Normal2 does not get lifefycle because it was never mounted - 'ErrorBoundary componentDidUpdate', ]); log.length = 0; @@ -1322,16 +1357,16 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', - 'ErrorBoundary unstable_handleError', ...(ReactDOMFeatureFlags.useFiber ? [ - // The initial render was aborted, so - // Fiber retries from the root. - 'ErrorBoundary componentWillReceiveProps', + // Finish updating with null children + 'Child1 ref is set to null', + 'ErrorBoundary componentDidUpdate', + // Handle the error + 'ErrorBoundary unstable_handleError', 'ErrorBoundary componentWillUpdate', - // Fiber renders first, resets refs later 'ErrorBoundary render error', - 'Child1 ref is set to null', ] : [ + 'ErrorBoundary unstable_handleError', // Stack resets ref first, renders later 'Child1 ref is set to null', 'ErrorBoundary render error', @@ -1384,7 +1419,8 @@ describe('ReactErrorBoundaries', () => { // The components have updated in this phase 'BrokenComponentWillUnmount componentDidUpdate', 'ErrorBoundary componentDidUpdate', - // Now that commit phase is done, Fiber handles errors + // Now that commit phase is done, Fiber unmounts the boundary's children + 'BrokenComponentWillUnmount componentWillUnmount [!]', 'ErrorBoundary unstable_handleError', // The initial render was aborted, so // Fiber retries from the root. @@ -1392,7 +1428,6 @@ describe('ReactErrorBoundaries', () => { // Render an error now (stack will do it later) 'ErrorBoundary render error', // Attempt to unmount previous child: - 'BrokenComponentWillUnmount componentWillUnmount [!]', // Done 'ErrorBoundary componentDidUpdate', ] : [ @@ -1454,6 +1489,8 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentWillUnmount componentDidUpdate', 'Normal componentDidUpdate', 'ErrorBoundary componentDidUpdate', + 'Normal componentWillUnmount', + 'BrokenComponentWillUnmount componentWillUnmount [!]', // Now that commit phase is done, Fiber handles errors 'ErrorBoundary unstable_handleError', // The initial render was aborted, so @@ -1461,9 +1498,6 @@ describe('ReactErrorBoundaries', () => { 'ErrorBoundary componentWillUpdate', // Render an error now (stack will do it later) 'ErrorBoundary render error', - // Attempt to unmount previous child: - 'Normal componentWillUnmount', - 'BrokenComponentWillUnmount componentWillUnmount [!]', // Done 'ErrorBoundary componentDidUpdate', ] : [ @@ -1803,15 +1837,16 @@ describe('ReactErrorBoundaries', () => { 'LastChild componentDidMount', 'ErrorBoundary componentDidMount', // Now we are ready to handle the error - 'ErrorBoundary unstable_handleError', - 'ErrorBoundary componentWillUpdate', - 'ErrorBoundary render error', // Safely unmount every child 'BrokenComponentWillUnmount componentWillUnmount [!]', // Continue unmounting safely despite any errors 'Normal componentWillUnmount', 'BrokenComponentDidMount componentWillUnmount', 'LastChild componentWillUnmount', + // Handle the error + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', // The update has finished 'ErrorBoundary componentDidUpdate', ]); @@ -1849,11 +1884,11 @@ describe('ReactErrorBoundaries', () => { // All lifecycles run 'BrokenComponentDidUpdate componentDidUpdate [!]', 'ErrorBoundary componentDidUpdate', + 'BrokenComponentDidUpdate componentWillUnmount', // Then, error is handled 'ErrorBoundary unstable_handleError', 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'BrokenComponentDidUpdate componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1888,12 +1923,12 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentDidMountErrorBoundary componentDidMount [!]', // Fiber proceeds with the hooks 'ErrorBoundary componentDidMount', + 'BrokenComponentDidMountErrorBoundary componentWillUnmount', // The error propagates to the higher boundary 'ErrorBoundary unstable_handleError', // Fiber retries from the root 'ErrorBoundary componentWillUpdate', 'ErrorBoundary render error', - 'BrokenComponentDidMountErrorBoundary componentWillUnmount', 'ErrorBoundary componentDidUpdate', ]); @@ -1972,21 +2007,17 @@ describe('ReactErrorBoundaries', () => { 'BrokenComponentDidUpdate componentDidUpdate [!]', 'InnerUpdateBoundary componentDidUpdate', 'OuterErrorBoundary componentDidUpdate', - // The interesting part starts now. - // Acknowledge errors independently but don't update yet: + // After the commit phase, attempt to recover from any errors that + // were captured 'InnerUnmountBoundary unstable_handleError', - 'InnerUpdateBoundary unstable_handleError', - // Only two of four errors are acknowledged: one per boundary. - // The rest are likely cascading and we ignore them. - // Now update: 'InnerUnmountBoundary componentWillUpdate', 'InnerUnmountBoundary render error', - 'InnerUpdateBoundary componentWillUpdate', - 'InnerUpdateBoundary render error', - // Commit 'BrokenComponentDidUpdate componentWillUnmount', 'BrokenComponentDidUpdate componentWillUnmount', 'InnerUnmountBoundary componentDidUpdate', + 'InnerUpdateBoundary unstable_handleError', + 'InnerUpdateBoundary componentWillUpdate', + 'InnerUpdateBoundary render error', 'InnerUpdateBoundary componentDidUpdate', ]); From 88be83655e8824031b1a58601835cf97f87c116a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 16 Nov 2016 17:01:06 +0000 Subject: [PATCH 02/19] Fix broken setState callback test --- .../fiber/__tests__/ReactIncremental-test.js | 33 ++++++++----------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index 65ff444e95aec..9db5158afea54 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -1465,7 +1465,6 @@ describe('ReactIncremental', () => { ]); expect(instance.state.n).toEqual(4); }); - it('can handle if setState callback throws', () => { var ops = []; var instance; @@ -1482,30 +1481,26 @@ describe('ReactIncremental', () => { ReactNoop.flush(); ops = []; - expect(instance.state.n).toEqual(0); - - // first good callback - instance.setState({ n: 1 }, () => ops.push('first good callback')); - ReactNoop.flush(); + function updater({ n }) { + return { n: n + 1 }; + } - // callback throws - instance.setState({ n: 2 }, () => { - throw new Error('Bail'); + instance.setState(updater, () => ops.push('first callback')); + instance.setState(updater, () => { + ops.push('second callback'); + throw new Error('callback error'); }); + instance.setState(updater, () => ops.push('third callback')); + expect(() => { ReactNoop.flush(); - }).toThrow('Bail'); - - // should set state to 2 even if callback throws up - expect(instance.state.n).toEqual(2); - - // another good callback - instance.setState({ n: 3 }, () => ops.push('second good callback')); - ReactNoop.flush(); + }).toThrow('callback error'); + // Should call all callbacks, even though the second one throws expect(ops).toEqual([ - 'first good callback', - 'second good callback', + 'first callback', + 'second callback', + 'third callback', ]); expect(instance.state.n).toEqual(3); }); From 69796b3d862b6cb08cbc101e0c3d76d8176a6ec9 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 11 Nov 2016 17:30:18 -0800 Subject: [PATCH 03/19] Exit early in scheduleUpdate if a node's priority matches This is a performance optimization and is unobservable. However, it helps protect against regressions on the following invariants on which it relies: - The priority of a fiber is greater than or equal to the priority of all its descendent fibers. - If a tree has pending work priority, its root is scheduled. --- .../shared/fiber/ReactFiberScheduler.js | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 15f801945e636..9905cd1fcc0a2 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -721,28 +721,37 @@ module.exports = function(config : HostConfig) { priorityLevel = TaskPriority; } - while (true) { - if (fiber.pendingWorkPriority === NoWork || - fiber.pendingWorkPriority >= priorityLevel) { - fiber.pendingWorkPriority = priorityLevel; + let node = fiber; + let shouldContinue = true; + while (node && shouldContinue) { + // Walk the parent path to the root and update each node's priority. Once + // we reach a node whose priority matches (and whose alternate's priority + // matches) we can exit safely knowing that the rest of the path is correct. + shouldContinue = false; + if (node.pendingWorkPriority === NoWork || + node.pendingWorkPriority >= priorityLevel) { + // Priority did not match. Update and keep going. + shouldContinue = true; + node.pendingWorkPriority = priorityLevel; } - if (fiber.alternate) { - if (fiber.alternate.pendingWorkPriority === NoWork || - fiber.alternate.pendingWorkPriority >= priorityLevel) { - fiber.alternate.pendingWorkPriority = priorityLevel; + if (node.alternate) { + if (node.alternate.pendingWorkPriority === NoWork || + node.alternate.pendingWorkPriority >= priorityLevel) { + // Priority did not match. Update and keep going. + shouldContinue = true; + node.alternate.pendingWorkPriority = priorityLevel; } } - if (!fiber.return) { - if (fiber.tag === HostContainer) { - const root : FiberRoot = (fiber.stateNode : any); + if (!node.return) { + if (node.tag === HostContainer) { + const root : FiberRoot = (node.stateNode : any); scheduleWorkAtPriority(root, priorityLevel); - return; } else { // TODO: Warn about setting state on an unmounted component. return; } } - fiber = fiber.return; + node = node.return; } } From 614cd4bd2c4abfcc8b099cddb4e47932b6a79667 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 17 Nov 2016 00:25:12 +0000 Subject: [PATCH 04/19] Add test for "unwinding" context when an error interrupts rendering --- .../dom/__tests__/ReactDOMProduction-test.js | 3 + .../shared/fiber/ReactFiberContext.js | 17 +++++- .../shared/fiber/ReactFiberScheduler.js | 37 +++++++++-- .../ReactIncrementalErrorHandling-test.js | 61 ++++++++++++++++++- 4 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/renderers/dom/__tests__/ReactDOMProduction-test.js b/src/renderers/dom/__tests__/ReactDOMProduction-test.js index c64a9411808e3..eef7e9351749f 100644 --- a/src/renderers/dom/__tests__/ReactDOMProduction-test.js +++ b/src/renderers/dom/__tests__/ReactDOMProduction-test.js @@ -180,6 +180,9 @@ describe('ReactDOMProduction', () => { expect(function() { class Component extends React.Component { render() { + // FIXME: undefined is a valid return type in Fiber, at least in the + // current implementation. It's treated as empty. I don't know why we + // sometimes get false positives. return undefined; } } diff --git a/src/renderers/shared/fiber/ReactFiberContext.js b/src/renderers/shared/fiber/ReactFiberContext.js index 6cb171c67293c..e86cfbcea7961 100644 --- a/src/renderers/shared/fiber/ReactFiberContext.js +++ b/src/renderers/shared/fiber/ReactFiberContext.js @@ -69,16 +69,19 @@ exports.hasContextChanged = function() : boolean { function isContextProvider(fiber : Fiber) : boolean { return ( fiber.tag === ClassComponent && + // Instance might be null, if the fiber errored during construction + fiber.stateNode && typeof fiber.stateNode.getChildContext === 'function' ); } exports.isContextProvider = isContextProvider; -exports.popContextProvider = function() : void { +function popContextProvider() : void { contextStack[index] = emptyObject; didPerformWorkStack[index] = false; index--; -}; +} +exports.popContextProvider = popContextProvider; exports.pushTopLevelContextObject = function(context : Object, didChange : boolean) : void { invariant(index === -1, 'Unexpected context found on stack'); @@ -149,3 +152,13 @@ exports.findCurrentUnmaskedContext = function(fiber: Fiber) : Object { } return node.stateNode.context; }; + +exports.unwindContext = function(from : Fiber, to: Fiber) { + let node = from; + while (node && (node !== to) && (node.alternate !== to)) { + if (isContextProvider(node)) { + popContextProvider(); + } + node = node.return; + } +}; diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 9905cd1fcc0a2..4d3ef745744d5 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -61,6 +61,10 @@ var { ClassComponent, } = require('ReactTypeOfWork'); +var { + unwindContext, +} = require('ReactFiberContext'); + if (__DEV__) { var ReactFiberInstrumentation = require('ReactFiberInstrumentation'); } @@ -555,10 +559,23 @@ module.exports = function(config : HostConfig) { break; } } catch (error) { - const boundary = captureError(nextUnitOfWork, error, false); + const failedWork = nextUnitOfWork; + const boundary = captureError(failedWork, error, false); if (boundary) { // The boundary failed to complete. Complete it as if rendered null. beginFailedWork(boundary.alternate, boundary, priorityLevel); + + // The next unit of work is now the boundary that captured the error. + // Conceptually, we're unwinding the stack. We need to unwind the + // context stack, too, from the failed work to the boundary that + // captured the error. + // TODO: If we set the memoized props in beginWork instead of + // completeWork, rather than unwind the stack, we can just restart + // from the root. Can't do that until then because without memoized + // props, the nodes higher up in the tree will rerender unnecessarily. + if (failedWork) { + unwindContext(failedWork, boundary); + } nextUnitOfWork = completeUnitOfWork(boundary); } // We were interupted by an error. Continue performing work. @@ -581,7 +598,10 @@ module.exports = function(config : HostConfig) { ReactCurrentOwner.current = null; // Ignore this error if it's the result of unmounting a failed boundary - if (failedWork && isUnmounting && capturedErrors && capturedErrors.has(failedWork)) { + if (failedWork && + isUnmounting && + capturedErrors && + capturedErrors.has(failedWork)) { return null; } @@ -605,12 +625,15 @@ module.exports = function(config : HostConfig) { } if (boundary) { - // Add to the collection of captured errors + // Add to the collection of captured errors. This is stored as a global + // map of errors 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) { capturedErrors = new Map(); } // Ensure that neither this boundary nor its alternate has captured an - // error already + // error already. if (!capturedErrors.has(boundary) && !(boundary.alternate && capturedErrors.has(boundary.alternate))) { capturedErrors.set(boundary, error); @@ -627,6 +650,7 @@ module.exports = function(config : HostConfig) { function commitErrorHandling(effectfulFiber : Fiber) { let error; if (capturedErrors) { + // Get the error associated with the fiber being commited. error = capturedErrors.get(effectfulFiber); capturedErrors.delete(effectfulFiber); } @@ -638,6 +662,8 @@ module.exports = function(config : HostConfig) { case ClassComponent: const instance = effectfulFiber.stateNode; try { + // Allow the boundary to handle the error, usually by scheduling + // an update to itself instance.unstable_handleError(error); } catch (e) { captureError(effectfulFiber, e, false); @@ -645,6 +671,9 @@ module.exports = function(config : HostConfig) { return; case HostContainer: if (!firstUncaughtError) { + // If this is the host container, we treat it as a no-op error + // boundary. We'll throw the first uncaught error once it's safe to + // do so, at the end of the batch. firstUncaughtError = error; } return; diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index 1a68e32897043..a7f00b8b0e778 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -274,6 +274,66 @@ describe('ReactIncrementalErrorHandling', () => { expect(ReactNoop.getChildren('f')).toEqual(null); }); + it('unwinds the context stack correctly on error', () => { + class Provider extends React.Component { + static childContextTypes = { message: React.PropTypes.string }; + static contextTypes = { message: React.PropTypes.string }; + getChildContext() { + return { + message: (this.context.message || '') + this.props.message, + }; + } + render() { + return this.props.children; + } + } + + function Connector(props, context) { + return ; + } + + Connector.contextTypes = { + message: React.PropTypes.string, + }; + + function BadRender() { + throw new Error('render error'); + } + + class Boundary extends React.Component { + state = { error: null }; + unstable_handleError(error) { + this.setState({ error }); + } + render() { + return ( + + + + + {!this.state.error && } + + + + + ); + } + } + + ReactNoop.render( + + + + + ); + ReactNoop.flush(); + + // If the context stack does not unwind, span will get 'abcde' + expect(ReactNoop.getChildren()).toEqual([ + span('a'), + ]); + }); + it('catches reconciler errors in a boundary during mounting', () => { spyOn(console, 'error'); @@ -289,7 +349,6 @@ describe('ReactIncrementalErrorHandling', () => { return this.props.children; } } - const InvalidType = undefined; const brokenElement = ; function BrokenRender(props) { From 4f126f549311523d3f01290b88a8b1e56a3fb34d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 17 Nov 2016 13:11:16 +0000 Subject: [PATCH 05/19] Switch over primary effect types only This avoids the need to create an export for every combination of bits. --- .../shared/fiber/ReactFiberScheduler.js | 43 ++++++------------- .../shared/fiber/ReactTypeOfSideEffect.js | 17 +------- 2 files changed, 13 insertions(+), 47 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 4d3ef745744d5..ced450839ddea 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -41,19 +41,7 @@ var { PlacementAndUpdate, Deletion, Callback, - PlacementAndCallback, - UpdateAndCallback, - PlacementAndUpdateAndCallback, - DeletionAndCallback, Err, - PlacementAndErr, - UpdateAndErr, - PlacementAndUpdateAndErr, - DeletionAndErr, - PlacementAndCallbackAndErr, - UpdateAndCallbackAndErr, - PlacementAndUpdateAndCallbackAndErr, - DeletionAndCallbackAndErr, } = require('ReactTypeOfSideEffect'); var { @@ -178,44 +166,37 @@ module.exports = function(config : HostConfig) { // ref unmounts. let effectfulFiber = finishedWork.firstEffect; while (effectfulFiber) { - switch (effectfulFiber.effectTag) { - case Placement: - case PlacementAndCallback: - case PlacementAndErr: - case PlacementAndCallbackAndErr: { + // The following switch statement is only concerned about placement, + // updates, and deletions. To avoid needing to add a case for every + // possible bitmap value, we remove the secondary effects from the + // effect tag and switch on that value. + let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err); + switch (primaryEffectTag) { + case Placement: { commitPlacement(effectfulFiber); // Clear the "placement" from effect tag so that we know that this is inserted, before // any life-cycles like componentDidMount gets called. - effectfulFiber.effectTag ^= Placement; + effectfulFiber.effectTag &= ~Placement; break; } - case PlacementAndUpdate: - case PlacementAndUpdateAndCallback: - case PlacementAndUpdateAndErr: - case PlacementAndUpdateAndCallbackAndErr: { + case PlacementAndUpdate: { // Placement commitPlacement(effectfulFiber); // Clear the "placement" from effect tag so that we know that this is inserted, before // any life-cycles like componentDidMount gets called. - effectfulFiber.effectTag ^= Placement; + effectfulFiber.effectTag &= ~Placement; // Update const current = effectfulFiber.alternate; commitWork(current, effectfulFiber); break; } - case Update: - case UpdateAndErr: - case UpdateAndCallback: - case UpdateAndCallbackAndErr: { + case Update: { const current = effectfulFiber.alternate; commitWork(current, effectfulFiber); break; } - case Deletion: - case DeletionAndCallback: - case DeletionAndErr: - case DeletionAndCallbackAndErr: { + case Deletion: { commitDeletion(effectfulFiber); break; } diff --git a/src/renderers/shared/fiber/ReactTypeOfSideEffect.js b/src/renderers/shared/fiber/ReactTypeOfSideEffect.js index 0fe4b468918fc..f8dc19407c8d3 100644 --- a/src/renderers/shared/fiber/ReactTypeOfSideEffect.js +++ b/src/renderers/shared/fiber/ReactTypeOfSideEffect.js @@ -12,9 +12,7 @@ 'use strict'; -export type TypeOfSideEffect = - 0 | 1 | 2 | 3 | 4 | 8 | 9 | 10 | 11 | 12 | 16 | 17 | 18 | 19 | 20 | 24 | 25 | - 26 | 27 | 28; +export type TypeOfSideEffect = 0 | 1 | 2 | 3 | 4 | 8 | 16; module.exports = { NoEffect: 0, // 0b00000 @@ -23,18 +21,5 @@ module.exports = { PlacementAndUpdate: 3, // 0b00011 Deletion: 4, // 0b00100 Callback: 8, // 0b01000 - PlacementAndCallback: 9, // 0b01001 - UpdateAndCallback: 10, // 0b01010 - PlacementAndUpdateAndCallback: 11, // 0b01011 - DeletionAndCallback: 12, // 0b01100 Err: 16, // 0b10000 - PlacementAndErr: 17, // 0b10001 - UpdateAndErr: 18, // 0b10010 - PlacementAndUpdateAndErr: 19, // 0b10011 - DeletionAndErr: 20, // 0b10100 - CallbackAndErr: 24, // 0b11000 - PlacementAndCallbackAndErr: 25, // 0b11001 - UpdateAndCallbackAndErr: 26, // 0b11010 - PlacementAndUpdateAndCallbackAndErr: 27, // 0b11011 - DeletionAndCallbackAndErr: 28, // 0b11100 }; From a61de422e94448e5f2808f33a78bd1770eb51af5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 17 Nov 2016 17:19:05 +0000 Subject: [PATCH 06/19] Only continue the work loop if the error was successfully captured --- src/renderers/shared/fiber/ReactFiberScheduler.js | 5 +++-- src/renderers/shared/shared/__tests__/ReactComponent-test.js | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index ced450839ddea..974f2af89b25b 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -558,9 +558,10 @@ module.exports = function(config : HostConfig) { unwindContext(failedWork, boundary); } nextUnitOfWork = completeUnitOfWork(boundary); + + // We were interupted by an error. Continue performing work. + shouldContinue = true; } - // We were interupted by an error. Continue performing work. - shouldContinue = true; } finally { shouldBatchUpdates = prevShouldBatchUpdates; } diff --git a/src/renderers/shared/shared/__tests__/ReactComponent-test.js b/src/renderers/shared/shared/__tests__/ReactComponent-test.js index 87359e2e3b83b..e4e63281c3c5f 100644 --- a/src/renderers/shared/shared/__tests__/ReactComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactComponent-test.js @@ -328,7 +328,7 @@ describe('ReactComponent', () => { expect(callback.mock.calls.length).toBe(3); }); - xit('throws usefully when rendering badly-typed elements', () => { + it('throws usefully when rendering badly-typed elements', () => { spyOn(console, 'error'); var X = undefined; From fad5cb53d434aa6fc3dd9fe4a26c2412f6c820ff Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 28 Nov 2016 15:12:22 -0600 Subject: [PATCH 07/19] Add try/catch/finally blocks around commit phase passes We'll consolidate all these blocks in a future PR that refactors the commit phase to be separate from the perform work loop. --- .../shared/fiber/ReactFiberCommitWork.js | 4 + .../shared/fiber/ReactFiberScheduler.js | 117 ++++++++++-------- 2 files changed, 67 insertions(+), 54 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index d7ae0f296bffa..36e205c96c9bf 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -133,6 +133,10 @@ module.exports = function( } function commitPlacement(finishedWork : Fiber) : void { + // Clear effect from effect tag before any errors can be thrown, so that + // we don't attempt to do this again + finishedWork.effectTag &= ~Placement; + // Recursively insert all host nodes into the parent. const parent = getHostParent(finishedWork); const before = getHostSibling(finishedWork); diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 974f2af89b25b..2af0a9e97208c 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -166,43 +166,47 @@ module.exports = function(config : HostConfig) { // ref unmounts. let effectfulFiber = finishedWork.firstEffect; while (effectfulFiber) { - // The following switch statement is only concerned about placement, - // updates, and deletions. To avoid needing to add a case for every - // possible bitmap value, we remove the secondary effects from the - // effect tag and switch on that value. - let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err); - switch (primaryEffectTag) { - case Placement: { - commitPlacement(effectfulFiber); - // Clear the "placement" from effect tag so that we know that this is inserted, before - // any life-cycles like componentDidMount gets called. - effectfulFiber.effectTag &= ~Placement; - break; - } - case PlacementAndUpdate: { - // Placement - commitPlacement(effectfulFiber); - // Clear the "placement" from effect tag so that we know that this is inserted, before - // any life-cycles like componentDidMount gets called. - effectfulFiber.effectTag &= ~Placement; - - // Update - const current = effectfulFiber.alternate; - commitWork(current, effectfulFiber); - break; - } - case Update: { - const current = effectfulFiber.alternate; - commitWork(current, effectfulFiber); - break; - } - case Deletion: { - commitDeletion(effectfulFiber); - break; + try { + // The following switch statement is only concerned about placement, + // updates, and deletions. To avoid needing to add a case for every + // possible bitmap value, we remove the secondary effects from the + // effect tag and switch on that value. + let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err); + switch (primaryEffectTag) { + case Placement: { + commitPlacement(effectfulFiber); + // Clear the "placement" from effect tag so that we know that this is inserted, before + // any life-cycles like componentDidMount gets called. + effectfulFiber.effectTag &= ~Placement; + break; + } + case PlacementAndUpdate: { + // Placement + commitPlacement(effectfulFiber); + // Clear the "placement" from effect tag so that we know that this is inserted, before + // any life-cycles like componentDidMount gets called. + effectfulFiber.effectTag &= ~Placement; + + // Update + const current = effectfulFiber.alternate; + commitWork(current, effectfulFiber); + break; + } + case Update: { + const current = effectfulFiber.alternate; + commitWork(current, effectfulFiber); + break; + } + case Deletion: { + commitDeletion(effectfulFiber); + break; + } } + } catch (error) { + captureError(effectfulFiber, error, false); + } finally { + effectfulFiber = effectfulFiber.nextEffect; } - - effectfulFiber = effectfulFiber.nextEffect; } // Finally if the root itself had an effect, we perform that since it is @@ -219,29 +223,34 @@ module.exports = function(config : HostConfig) { // already been invoked. effectfulFiber = finishedWork.firstEffect; while (effectfulFiber) { - const current = effectfulFiber.alternate; - - // Use Task priority for lifecycle updates const previousPriorityContext = priorityContext; priorityContext = TaskPriority; - if (effectfulFiber.effectTag & (Update | Callback)) { - commitLifeCycles(current, effectfulFiber); - } - priorityContext = previousPriorityContext; + try { + const current = effectfulFiber.alternate; + // Use Task priority for lifecycle updates + if (effectfulFiber.effectTag & (Update | Callback)) { + commitLifeCycles(current, effectfulFiber); + } - if (effectfulFiber.effectTag & Err) { - commitErrorHandling(effectfulFiber); + if (effectfulFiber.effectTag & Err) { + commitErrorHandling(effectfulFiber); + } + } catch (error) { + captureError(effectfulFiber, error, false); + } finally { + // Clean-uo + priorityContext = previousPriorityContext; + + const next = effectfulFiber.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 + // and lastEffect since they're on every node, not just the effectful + // ones. So we have to clean everything as we reuse nodes anyway. + effectfulFiber.nextEffect = null; + // Ensure that we reset the effectTag here so that we can rely on effect + // tags to reason about the current life-cycle. + effectfulFiber = next; } - - const next = effectfulFiber.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 - // and lastEffect since they're on every node, not just the effectful - // ones. So we have to clean everything as we reuse nodes anyway. - effectfulFiber.nextEffect = null; - // Ensure that we reset the effectTag here so that we can rely on effect - // tags to reason about the current life-cycle. - effectfulFiber = next; } // Lifecycles on the root itself From ece9f7b3461f3454f485e8f521745b8e2eeef5d2 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 28 Nov 2016 16:11:13 -0600 Subject: [PATCH 08/19] NoopBoundary -> RethrowBoundary --- .../fiber/__tests__/ReactIncrementalErrorHandling-test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index a7f00b8b0e778..d03cc73215eac 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -53,7 +53,7 @@ describe('ReactIncrementalErrorHandling', () => { }); it('propagates an error from a noop error boundary', () => { - class NoopBoundary extends React.Component { + class RethrowBoundary extends React.Component { unstable_handleError(error) { throw error; } @@ -67,9 +67,9 @@ describe('ReactIncrementalErrorHandling', () => { } ReactNoop.render( - + - + ); expect(ReactNoop.flush).toThrow('render error'); From 226ee8ff2ed05235c23e1ce224233fd1206406f8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 29 Nov 2016 14:11:41 -0600 Subject: [PATCH 09/19] Only throw uncaught error once there is no more work to perform --- src/renderers/shared/fiber/ReactFiberScheduler.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 2af0a9e97208c..e5853be1f7739 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -238,7 +238,7 @@ module.exports = function(config : HostConfig) { } catch (error) { captureError(effectfulFiber, error, false); } finally { - // Clean-uo + // Clean-up priorityContext = previousPriorityContext; const next = effectfulFiber.nextEffect; @@ -577,7 +577,7 @@ module.exports = function(config : HostConfig) { } // Throw the first uncaught error - if (!shouldBatchUpdates && firstUncaughtError) { + if (!nextUnitOfWork && firstUncaughtError) { let e = firstUncaughtError; firstUncaughtError = null; throw e; From 7386e82c535632682a443b37d3739a4ed11fe4e0 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 5 Nov 2016 09:37:05 +0000 Subject: [PATCH 10/19] Add more tests for incremental error handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These tests are currently failing: ✕ catches render error in a boundary during partial deferred mounting ✕ catches render error in a boundary during animation mounting ✕ propagates an error from a noop error boundary during full deferred mounting ✕ propagates an error from a noop error boundary during partial deferred mounting ✕ propagates an error from a noop error boundary during animation mounting The observed behavior is that unstable_handleError() unexpected gets called twice: "ErrorBoundary render success", "BrokenRender", "ErrorBoundary unstable_handleError", + "ErrorBoundary render success", + "BrokenRender", + "ErrorBoundary unstable_handleError", "ErrorBoundary render error" --- .../ReactIncrementalErrorHandling-test.js | 363 +++++++++++++++++- 1 file changed, 353 insertions(+), 10 deletions(-) diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index d03cc73215eac..590f7a745870e 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -25,7 +25,7 @@ describe('ReactIncrementalErrorHandling', () => { return { type: 'span', children: [], prop }; } - it('catches render error in a boundary during mounting', () => { + it('catches render error in a boundary during full deferred mounting', () => { class ErrorBoundary extends React.Component { state = {error: null}; unstable_handleError(error) { @@ -48,31 +48,374 @@ describe('ReactIncrementalErrorHandling', () => { ); - ReactNoop.flush(); + ReactNoop.flushDeferredPri(); + expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello.')]); + }); + + it('catches render error in a boundary during partial deferred mounting', () => { + var ops = []; + class ErrorBoundary extends React.Component { + state = {error: null}; + unstable_handleError(error) { + ops.push('ErrorBoundary unstable_handleError'); + this.setState({error}); + } + render() { + if (this.state.error) { + ops.push('ErrorBoundary render error'); + return ; + } + ops.push('ErrorBoundary render success'); + return this.props.children; + } + } + + function BrokenRender(props) { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + ReactNoop.render( + + + + ); + + ReactNoop.flushDeferredPri(15); + expect(ops).toEqual([ + 'ErrorBoundary render success', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + + ops.length = 0; + ReactNoop.flushDeferredPri(30); + expect(ops).toEqual([ + 'BrokenRender', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello.')]); + }); + + it('catches render error in a boundary during animation mounting', () => { + var ops = []; + class ErrorBoundary extends React.Component { + state = {error: null}; + unstable_handleError(error) { + ops.push('ErrorBoundary unstable_handleError'); + this.setState({error}); + } + render() { + if (this.state.error) { + ops.push('ErrorBoundary render error'); + return ; + } + ops.push('ErrorBoundary render success'); + return this.props.children; + } + } + + function BrokenRender(props) { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + ReactNoop.performAnimationWork(() => { + ReactNoop.render( + + + + ); + }); + + ReactNoop.flushAnimationPri(); + expect(ops).toEqual([ + 'ErrorBoundary render success', + 'BrokenRender', + ]); + ops = []; + ReactNoop.flushDeferredPri(25); + expect(ops).toEqual([ + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello.')]); + }); + + it('catches render error in a boundary during synchronous mounting', () => { + var ops = []; + class ErrorBoundary extends React.Component { + state = {error: null}; + unstable_handleError(error) { + ops.push('ErrorBoundary unstable_handleError'); + this.setState({error}); + } + render() { + if (this.state.error) { + ops.push('ErrorBoundary render error'); + return ; + } + ops.push('ErrorBoundary render success'); + return this.props.children; + } + } + + function BrokenRender(props) { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + ReactNoop.syncUpdates(() => { + ReactNoop.render( + + + + ); + }); + + expect(ops).toEqual([ + 'ErrorBoundary render success', + 'BrokenRender', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello.')]); + }); + + it('catches render error in a boundary during batched mounting', () => { + var ops = []; + class ErrorBoundary extends React.Component { + state = {error: null}; + unstable_handleError(error) { + ops.push('ErrorBoundary unstable_handleError'); + this.setState({error}); + } + render() { + if (this.state.error) { + ops.push('ErrorBoundary render error'); + return ; + } + ops.push('ErrorBoundary render success'); + return this.props.children; + } + } + + function BrokenRender(props) { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + ReactNoop.syncUpdates(() => { + ReactNoop.batchedUpdates(() => { + ReactNoop.render( + + Before the storm. + + ); + ReactNoop.render( + + + + ); + }); + }); + + expect(ops).toEqual([ + 'ErrorBoundary render success', + 'BrokenRender', + 'ErrorBoundary unstable_handleError', + 'ErrorBoundary render error', + ]); expect(ReactNoop.getChildren()).toEqual([span('Caught an error: Hello.')]); }); - it('propagates an error from a noop error boundary', () => { - class RethrowBoundary extends React.Component { + it('propagates an error from a noop error boundary during full deferred mounting', () => { + var ops = []; + class RethrowErrorBoundary extends React.Component { unstable_handleError(error) { + ops.push('RethrowErrorBoundary unstable_handleError'); throw error; } render() { + ops.push('RethrowErrorBoundary render'); return this.props.children; } } - function RenderError() { - throw new Error('render error'); + function BrokenRender() { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + ReactNoop.render( + + + + ); + + expect(() => { + ReactNoop.flush(); + }).toThrow('Hello'); + expect(ops).toEqual([ + 'RethrowErrorBoundary render', + 'BrokenRender', + 'RethrowErrorBoundary unstable_handleError', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('propagates an error from a noop error boundary during partial deferred mounting', () => { + var ops = []; + class RethrowErrorBoundary extends React.Component { + unstable_handleError(error) { + ops.push('RethrowErrorBoundary unstable_handleError'); + throw error; + } + render() { + ops.push('RethrowErrorBoundary render'); + return this.props.children; + } + } + + function BrokenRender() { + ops.push('BrokenRender'); + throw new Error('Hello'); } ReactNoop.render( - - - + + + ); - expect(ReactNoop.flush).toThrow('render error'); + ReactNoop.flushDeferredPri(15); + expect(ops).toEqual([ + 'RethrowErrorBoundary render', + ]); + + ops.length = 0; + expect(() => { + ReactNoop.flush(); + }).toThrow('Hello'); + expect(ops).toEqual([ + 'BrokenRender', + 'RethrowErrorBoundary unstable_handleError', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('propagates an error from a noop error boundary during animation mounting', () => { + var ops = []; + class RethrowErrorBoundary extends React.Component { + unstable_handleError(error) { + ops.push('RethrowErrorBoundary unstable_handleError'); + throw error; + } + render() { + ops.push('RethrowErrorBoundary render'); + return this.props.children; + } + } + + function BrokenRender() { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + ReactNoop.performAnimationWork(() => { + ReactNoop.render( + + + + ); + }); + + expect(() => { + ReactNoop.flush(); + }).toThrow('Hello'); + expect(ops).toEqual([ + 'RethrowErrorBoundary render', + 'BrokenRender', + 'RethrowErrorBoundary unstable_handleError', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('propagates an error from a noop error boundary during synchronous mounting', () => { + var ops = []; + class RethrowErrorBoundary extends React.Component { + unstable_handleError(error) { + ops.push('RethrowErrorBoundary unstable_handleError'); + throw error; + } + render() { + ops.push('RethrowErrorBoundary render'); + return this.props.children; + } + } + + function BrokenRender() { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + expect(() => { + ReactNoop.syncUpdates(() => { + ReactNoop.render( + + + + ); + }); + }).toThrow('Hello'); + expect(ops).toEqual([ + 'RethrowErrorBoundary render', + 'BrokenRender', + 'RethrowErrorBoundary unstable_handleError', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); + + it('propagates an error from a noop error boundary during batched mounting', () => { + var ops = []; + class RethrowErrorBoundary extends React.Component { + unstable_handleError(error) { + ops.push('RethrowErrorBoundary unstable_handleError'); + throw error; + } + render() { + ops.push('RethrowErrorBoundary render'); + return this.props.children; + } + } + + function BrokenRender() { + ops.push('BrokenRender'); + throw new Error('Hello'); + } + + expect(() => { + ReactNoop.syncUpdates(() => { + ReactNoop.batchedUpdates(() => { + ReactNoop.render( + + Before the storm. + + ); + ReactNoop.render( + + + + ); + }); + }); + }).toThrow('Hello'); + expect(ops).toEqual([ + 'RethrowErrorBoundary render', + 'BrokenRender', + 'RethrowErrorBoundary unstable_handleError', + ]); + expect(ReactNoop.getChildren()).toEqual([]); }); it('can schedule updates after uncaught error in render on mount', () => { From d27cadaf8de04967686bc10792770a0a85c8b6b7 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 5 Nov 2016 09:58:40 +0000 Subject: [PATCH 11/19] Verify batched updates get scheduled despite errors --- scripts/fiber/tests-passing.txt | 15 ++++++- .../ReactIncrementalErrorHandling-test.js | 44 +++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index a780be9589141..8b030a91be61e 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1080,8 +1080,19 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js * reads context when setState is above the provider src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js -* catches render error in a boundary during mounting -* propagates an error from a noop error boundary +* catches render error in a boundary during full deferred mounting +* catches render error in a boundary during partial deferred mounting +* catches render error in a boundary during animation mounting +* catches render error in a boundary during synchronous mounting +* catches render error in a boundary during batched mounting +* propagates an error from a noop error boundary during full deferred mounting +* propagates an error from a noop error boundary during partial deferred mounting +* propagates an error from a noop error boundary during animation mounting +* propagates an error from a noop error boundary during synchronous mounting +* propagates an error from a noop error boundary during batched mounting +* applies batched updates regardless despite errors in scheduling +* applies nested batched updates despite errors in scheduling +* applies sync updates regardless despite errors in scheduling * can schedule updates after uncaught error in render on mount * can schedule updates after uncaught error in render on update * can schedule updates after uncaught error during umounting diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index 590f7a745870e..5d0eb52575f03 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -418,6 +418,50 @@ describe('ReactIncrementalErrorHandling', () => { expect(ReactNoop.getChildren()).toEqual([]); }); + it('applies batched updates regardless despite errors in scheduling', () => { + ReactNoop.render(); + expect(() => { + ReactNoop.batchedUpdates(() => { + ReactNoop.render(); + ReactNoop.render(); + throw new Error('Hello'); + }); + }).toThrow('Hello'); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('a:3')]); + }); + + it('applies nested batched updates despite errors in scheduling', () => { + ReactNoop.render(); + expect(() => { + ReactNoop.batchedUpdates(() => { + ReactNoop.render(); + ReactNoop.render(); + ReactNoop.batchedUpdates(() => { + ReactNoop.render(); + ReactNoop.render(); + throw new Error('Hello'); + }); + }); + }).toThrow('Hello'); + ReactNoop.flush(); + expect(ReactNoop.getChildren()).toEqual([span('a:5')]); + }); + + it('applies sync updates regardless despite errors in scheduling', () => { + ReactNoop.render(); + expect(() => { + ReactNoop.syncUpdates(() => { + ReactNoop.batchedUpdates(() => { + ReactNoop.render(); + ReactNoop.render(); + throw new Error('Hello'); + }); + }); + }).toThrow('Hello'); + expect(ReactNoop.getChildren()).toEqual([span('a:3')]); + }); + it('can schedule updates after uncaught error in render on mount', () => { var ops = []; From cc60f25bc2ab743a93c72c6bd454d9ad3b2b356c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 30 Nov 2016 14:43:03 +0000 Subject: [PATCH 12/19] Remove outdated comment It was fixed in #8451. --- src/renderers/dom/__tests__/ReactDOMProduction-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/renderers/dom/__tests__/ReactDOMProduction-test.js b/src/renderers/dom/__tests__/ReactDOMProduction-test.js index eef7e9351749f..c64a9411808e3 100644 --- a/src/renderers/dom/__tests__/ReactDOMProduction-test.js +++ b/src/renderers/dom/__tests__/ReactDOMProduction-test.js @@ -180,9 +180,6 @@ describe('ReactDOMProduction', () => { expect(function() { class Component extends React.Component { render() { - // FIXME: undefined is a valid return type in Fiber, at least in the - // current implementation. It's treated as empty. I don't know why we - // sometimes get false positives. return undefined; } } From 778183e285551f9a1dd6cf109c17fc0c84fcf76a Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 30 Nov 2016 17:19:15 +0000 Subject: [PATCH 13/19] Record tests --- scripts/fiber/tests-failing.txt | 4 ++++ scripts/fiber/tests-passing.txt | 3 +-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index cc5e8e4bb0b76..1594b1f13dedd 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -74,6 +74,9 @@ src/renderers/shared/__tests__/ReactPerf-test.js * should not count time in a portal towards lifecycle method * should work when measurement starts during reconciliation +src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +* recovers from uncaught reconciler errors + src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js * gets created * can be retrieved by ID @@ -83,6 +86,7 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js src/renderers/shared/shared/__tests__/ReactComponent-test.js * should throw on invalid render targets +* throws usefully when rendering badly-typed elements * includes owner name in the error about badly-typed elements src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 8b030a91be61e..67f9b0393146f 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1098,9 +1098,9 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * can schedule updates after uncaught error during umounting * continues work on other roots despite caught errors * continues work on other roots despite uncaught errors +* unwinds the context stack correctly on error * catches reconciler errors in a boundary during mounting * catches reconciler errors in a boundary during update -* recovers from uncaught reconciler errors src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * handles isMounted even when the initial render is deferred @@ -1235,7 +1235,6 @@ src/renderers/shared/shared/__tests__/ReactComponent-test.js * should support new-style refs with mixed-up owners * should call refs at the correct time * fires the callback after a component is rendered -* throws usefully when rendering badly-typed elements src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js * should not reuse an instance when it has been unmounted From 54978e9523aadd8a6fe884e002ca930eb0c90f4c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 30 Nov 2016 18:30:59 +0000 Subject: [PATCH 14/19] Always reset nextUnitOfWork on error This is important so that the test at the end of performAndHandleErrors() knows it's safe to rethrow. --- scripts/fiber/tests-failing.txt | 3 --- scripts/fiber/tests-passing.txt | 1 + src/renderers/shared/fiber/ReactFiberScheduler.js | 3 ++- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 1594b1f13dedd..736059ee084e1 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -74,9 +74,6 @@ src/renderers/shared/__tests__/ReactPerf-test.js * should not count time in a portal towards lifecycle method * should work when measurement starts during reconciliation -src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js -* recovers from uncaught reconciler errors - src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js * gets created * can be retrieved by ID diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 67f9b0393146f..424acc7faa862 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1101,6 +1101,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * unwinds the context stack correctly on error * catches reconciler errors in a boundary during mounting * catches reconciler errors in a boundary during update +* recovers from uncaught reconciler errors src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * handles isMounted even when the initial render is deferred diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index e5853be1f7739..73bf657c6a432 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -587,6 +587,8 @@ module.exports = function(config : HostConfig) { function captureError(failedWork : Fiber | null, error : Error, isUnmounting : boolean) : Fiber | null { // It is no longer valid because we exited the user code. ReactCurrentOwner.current = null; + // It is no longer valid because this unit of work failed. + nextUnitOfWork = null; // Ignore this error if it's the result of unmounting a failed boundary if (failedWork && @@ -629,7 +631,6 @@ module.exports = function(config : HostConfig) { !(boundary.alternate && capturedErrors.has(boundary.alternate))) { capturedErrors.set(boundary, error); } - nextUnitOfWork = null; return boundary; } else if (!firstUncaughtError) { // If no boundary is found, we'll need to throw the error From c1f2eb201b10832076c676102b0e61d872d721d4 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 30 Nov 2016 18:53:14 +0000 Subject: [PATCH 15/19] Add a passing test for unmounting behavior on crashed tree --- scripts/fiber/tests-passing.txt | 1 + .../ReactIncrementalErrorHandling-test.js | 52 +++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 424acc7faa862..cc7e6dd590806 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1102,6 +1102,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js * catches reconciler errors in a boundary during mounting * catches reconciler errors in a boundary during update * recovers from uncaught reconciler errors +* unmounts components with uncaught errors src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js * handles isMounted even when the initial render is deferred diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index 5d0eb52575f03..1a379edb0be04 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -812,4 +812,56 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('hi')]); }); + + it('unmounts components with uncaught errors', () => { + const ops = []; + let inst; + + class BrokenRenderAndUnmount extends React.Component { + state = {fail: false}; + componentWillUnmount() { + ops.push('BrokenRenderAndUnmount componentWillUnmount'); + } + render() { + inst = this; + if (this.state.fail) { + throw new Error('Hello.'); + } + return null; + } + } + + class Parent extends React.Component { + componentWillUnmount() { + ops.push('Parent componentWillUnmount [!]'); + throw new Error('One does not simply unmount me.'); + } + render() { + return this.props.children; + } + } + + ReactNoop.render( + + + + + + ); + ReactNoop.flush(); + + inst.setState({fail: true}); + expect(() => { + ReactNoop.flush(); + }).toThrowError('Hello.'); + + expect(ops).toEqual([ + // Attempt to clean up. + // Errors in parents shouldn't stop children from unmounting. + 'Parent componentWillUnmount [!]', + 'Parent componentWillUnmount [!]', + 'BrokenRenderAndUnmount componentWillUnmount', + ]); + expect(ReactNoop.getChildren()).toEqual([]); + }); }); From 46581a3f8042b0467f318e121838af1863ba1885 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 30 Nov 2016 11:20:47 -0800 Subject: [PATCH 16/19] Top-level errors An error thrown from a host container should be "captured" by the host container itself --- scripts/fiber/tests-failing.txt | 1 - scripts/fiber/tests-passing.txt | 2 ++ .../shared/fiber/ReactFiberScheduler.js | 27 ++++++++++++------- .../__tests__/ReactErrorBoundaries-test.js | 25 +++++++++++++++++ 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 736059ee084e1..cc5e8e4bb0b76 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -83,7 +83,6 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js src/renderers/shared/shared/__tests__/ReactComponent-test.js * should throw on invalid render targets -* throws usefully when rendering badly-typed elements * includes owner name in the error about badly-typed elements src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index cc7e6dd590806..35e796a46e355 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1237,6 +1237,7 @@ src/renderers/shared/shared/__tests__/ReactComponent-test.js * should support new-style refs with mixed-up owners * should call refs at the correct time * fires the callback after a component is rendered +* throws usefully when rendering badly-typed elements src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js * should not reuse an instance when it has been unmounted @@ -1335,6 +1336,7 @@ src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js * catches errors in componentDidUpdate * propagates errors inside boundary during componentDidMount * lets different boundaries catch their own first errors +* discards a bad root if the root component fails src/renderers/shared/shared/__tests__/ReactIdentity-test.js * should allow key property to express identity diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 73bf657c6a432..77ab950118894 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -601,19 +601,26 @@ module.exports = function(config : HostConfig) { // Search for the nearest error boundary. let boundary : Fiber | null = null; if (failedWork) { - let node = failedWork.return; - while (node && !boundary) { - if (node.tag === ClassComponent) { - const instance = node.stateNode; - if (typeof instance.unstable_handleError === 'function') { - // Found an error boundary! + // 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 === HostContainer) { + boundary = failedWork; + } else { + let node = failedWork.return; + while (node && !boundary) { + if (node.tag === ClassComponent) { + const instance = node.stateNode; + if (typeof instance.unstable_handleError === 'function') { + // Found an error boundary! + boundary = node; + } + } else if (node.tag === HostContainer) { + // Treat the root like a no-op error boundary. boundary = node; } - } else if (node.tag === HostContainer) { - // Treat the root like a no-op error boundary. - boundary = node; + node = node.return; } - node = node.return; } } diff --git a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js index d36441d313637..d529c2cfc667e 100644 --- a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js @@ -2029,6 +2029,31 @@ describe('ReactErrorBoundaries', () => { 'InnerUpdateBoundary componentWillUnmount', ]); }); + + it('discards a bad root if the root component fails', () => { + spyOn(console, 'error'); + + const X = null; + const Y = undefined; + let err1; + let err2; + + try { + let container = document.createElement('div'); + ReactDOM.render(, container); + } catch (err) { + err1 = err; + } + try { + let container = document.createElement('div'); + ReactDOM.render(, container); + } catch (err) { + err2 = err; + } + + expect(err1.message).toMatch(/got: null/); + expect(err2.message).toMatch(/got: undefined/); + }); } }); From b32c86e1f36485c925ba0b847665acef4d2f8485 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 30 Nov 2016 19:31:37 +0000 Subject: [PATCH 17/19] Remove outdated comment --- .../shared/shared/__tests__/ReactErrorBoundaries-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js index d529c2cfc667e..f284fa15b4ab3 100644 --- a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js @@ -774,9 +774,6 @@ describe('ReactErrorBoundaries', () => { 'BrokenRender render [!]', ...(ReactDOMFeatureFlags.useFiber ? [ // In Fiber, noop error boundaries render null - // 'ErrorBoundary constructor', - // 'ErrorBoundary componentWillMount', - // 'ErrorBoundary render success', 'NoopErrorBoundary componentDidMount', 'ErrorBoundary componentDidMount', 'NoopErrorBoundary unstable_handleError', From 7bc10c0dd9442afbf2bc7b113bc0ec9ae7fe6358 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 30 Nov 2016 19:50:34 +0000 Subject: [PATCH 18/19] Separate Rethrow and Noop scenarios in boundary tests --- scripts/fiber/tests-passing.txt | 1 + .../__tests__/ReactErrorBoundaries-test.js | 102 +++++++++++++++--- 2 files changed, 87 insertions(+), 16 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 35e796a46e355..076dbde55db7b 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1337,6 +1337,7 @@ src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js * propagates errors inside boundary during componentDidMount * lets different boundaries catch their own first errors * discards a bad root if the root component fails +* renders empty output if error boundary does not handle the error src/renderers/shared/shared/__tests__/ReactIdentity-test.js * should allow key property to express identity diff --git a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js index f284fa15b4ab3..6aa798cab483b 100644 --- a/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js +++ b/src/renderers/shared/shared/__tests__/ReactErrorBoundaries-test.js @@ -33,6 +33,7 @@ describe('ReactErrorBoundaries', () => { var ErrorBoundary; var ErrorMessage; var NoopErrorBoundary; + var RethrowErrorBoundary; var Normal; beforeEach(() => { @@ -394,7 +395,12 @@ describe('ReactErrorBoundaries', () => { log.push('NoopErrorBoundary componentWillUnmount'); } unstable_handleError() { - log.push('NoopErrorBoundary unstable_handleError'); + if (ReactDOMFeatureFlags.useFiber) { + log.push('NoopErrorBoundary unstable_handleError'); + } else { + // In Stack, not calling setState() is treated as a rethrow. + log.push('NoopErrorBoundary unstable_handleError [*]'); + } } }; @@ -478,6 +484,35 @@ describe('ReactErrorBoundaries', () => { }, }; + RethrowErrorBoundary = class extends React.Component { + constructor(props) { + super(props); + log.push('RethrowErrorBoundary constructor'); + } + render() { + log.push('RethrowErrorBoundary render'); + return ; + } + componentWillMount() { + log.push('RethrowErrorBoundary componentWillMount'); + } + componentDidMount() { + log.push('RethrowErrorBoundary componentDidMount'); + } + componentWillUnmount() { + log.push('RethrowErrorBoundary componentWillUnmount'); + } + unstable_handleError(error) { + if (!ReactDOMFeatureFlags.useFiber) { + log.push('RethrowErrorBoundary unstable_handleError [*]'); + // In Stack, not calling setState() is treated as a rethrow. + return; + } + log.push('RethrowErrorBoundary unstable_handleError [!]'); + throw error; + } + }; + ErrorMessage = class extends React.Component { constructor(props) { super(props); @@ -753,35 +788,41 @@ describe('ReactErrorBoundaries', () => { var container = document.createElement('div'); ReactDOM.render( - + - + , container ); - expect(container.firstChild.textContent).toBe( - ReactDOMFeatureFlags.useFiber ? '' : 'Caught an error: Hello.' - ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); expect(log).toEqual([ 'ErrorBoundary constructor', 'ErrorBoundary componentWillMount', 'ErrorBoundary render success', - 'NoopErrorBoundary constructor', - 'NoopErrorBoundary componentWillMount', - 'NoopErrorBoundary render', + 'RethrowErrorBoundary constructor', + 'RethrowErrorBoundary componentWillMount', + 'RethrowErrorBoundary render', 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', ...(ReactDOMFeatureFlags.useFiber ? [ // In Fiber, noop error boundaries render null - 'NoopErrorBoundary componentDidMount', + 'RethrowErrorBoundary componentDidMount', 'ErrorBoundary componentDidMount', - 'NoopErrorBoundary unstable_handleError', + 'RethrowErrorBoundary unstable_handleError [!]', + // The error got rethrown here. + // This time, the error propagates to the higher boundary + 'RethrowErrorBoundary componentWillUnmount', + 'ErrorBoundary unstable_handleError', + // Render the error + 'ErrorBoundary componentWillUpdate', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidUpdate', ] : [ // The first error boundary catches the error. // However, it doesn't adjust its state so next render will also fail. - 'NoopErrorBoundary unstable_handleError', - 'NoopErrorBoundary render', + 'RethrowErrorBoundary unstable_handleError [*]', + 'RethrowErrorBoundary render', 'BrokenRender constructor', 'BrokenRender componentWillMount', 'BrokenRender render [!]', @@ -797,9 +838,6 @@ describe('ReactErrorBoundaries', () => { ReactDOM.unmountComponentAtNode(container); expect(log).toEqual([ 'ErrorBoundary componentWillUnmount', - ...(ReactDOMFeatureFlags.useFiber ? [ - 'NoopErrorBoundary componentWillUnmount', - ] : []), ]); }); @@ -2051,6 +2089,38 @@ describe('ReactErrorBoundaries', () => { expect(err1.message).toMatch(/got: null/); expect(err2.message).toMatch(/got: undefined/); }); + + it('renders empty output if error boundary does not handle the error', () => { + var container = document.createElement('div'); + ReactDOM.render( +
+ Sibling + + + +
, + container + ); + expect(container.firstChild.textContent).toBe('Sibling'); + expect(log).toEqual([ + 'NoopErrorBoundary constructor', + 'NoopErrorBoundary componentWillMount', + 'NoopErrorBoundary render', + 'BrokenRender constructor', + 'BrokenRender componentWillMount', + 'BrokenRender render [!]', + // In Fiber, noop error boundaries render null + 'NoopErrorBoundary componentDidMount', + 'NoopErrorBoundary unstable_handleError', + // Nothing happens. + ]); + + log.length = 0; + ReactDOM.unmountComponentAtNode(container); + expect(log).toEqual([ + 'NoopErrorBoundary componentWillUnmount', + ]); + }); } }); From cf20dfa99841fcaee0baaf4ae30a550b0e70df5b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 30 Nov 2016 12:06:22 -0800 Subject: [PATCH 19/19] Move try block outside the commit loops --- .../shared/fiber/ReactFiberScheduler.js | 135 ++++++++++-------- 1 file changed, 74 insertions(+), 61 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 77ab950118894..9eb147df96d6f 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -165,48 +165,54 @@ module.exports = function(config : HostConfig) { // First, we'll perform all the host insertions, updates, deletions and // ref unmounts. let effectfulFiber = finishedWork.firstEffect; - while (effectfulFiber) { + pass1: while (true) { try { - // The following switch statement is only concerned about placement, - // updates, and deletions. To avoid needing to add a case for every - // possible bitmap value, we remove the secondary effects from the - // effect tag and switch on that value. - let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err); - switch (primaryEffectTag) { - case Placement: { - commitPlacement(effectfulFiber); - // Clear the "placement" from effect tag so that we know that this is inserted, before - // any life-cycles like componentDidMount gets called. - effectfulFiber.effectTag &= ~Placement; - break; - } - case PlacementAndUpdate: { - // Placement - commitPlacement(effectfulFiber); - // Clear the "placement" from effect tag so that we know that this is inserted, before - // any life-cycles like componentDidMount gets called. - effectfulFiber.effectTag &= ~Placement; - - // Update - const current = effectfulFiber.alternate; - commitWork(current, effectfulFiber); - break; - } - case Update: { - const current = effectfulFiber.alternate; - commitWork(current, effectfulFiber); - break; - } - case Deletion: { - commitDeletion(effectfulFiber); - break; + while (effectfulFiber) { + // The following switch statement is only concerned about placement, + // updates, and deletions. To avoid needing to add a case for every + // possible bitmap value, we remove the secondary effects from the + // effect tag and switch on that value. + let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err); + switch (primaryEffectTag) { + case Placement: { + commitPlacement(effectfulFiber); + // Clear the "placement" from effect tag so that we know that this is inserted, before + // any life-cycles like componentDidMount gets called. + effectfulFiber.effectTag &= ~Placement; + break; + } + case PlacementAndUpdate: { + // Placement + commitPlacement(effectfulFiber); + // Clear the "placement" from effect tag so that we know that this is inserted, before + // any life-cycles like componentDidMount gets called. + effectfulFiber.effectTag &= ~Placement; + + // Update + const current = effectfulFiber.alternate; + commitWork(current, effectfulFiber); + break; + } + case Update: { + const current = effectfulFiber.alternate; + commitWork(current, effectfulFiber); + break; + } + case Deletion: { + commitDeletion(effectfulFiber); + break; + } } + effectfulFiber = effectfulFiber.nextEffect; } } catch (error) { - captureError(effectfulFiber, error, false); - } finally { - effectfulFiber = effectfulFiber.nextEffect; + if (effectfulFiber) { + captureError(effectfulFiber, error, false); + effectfulFiber = effectfulFiber.nextEffect; + continue pass1; + } } + break; } // Finally if the root itself had an effect, we perform that since it is @@ -222,35 +228,42 @@ module.exports = function(config : HostConfig) { // happens as a separate pass so that all effects in the entire tree have // already been invoked. effectfulFiber = finishedWork.firstEffect; - while (effectfulFiber) { - const previousPriorityContext = priorityContext; - priorityContext = TaskPriority; + const previousPriorityContext = priorityContext; + priorityContext = TaskPriority; + pass2: while (true) { try { - const current = effectfulFiber.alternate; - // Use Task priority for lifecycle updates - if (effectfulFiber.effectTag & (Update | Callback)) { - commitLifeCycles(current, effectfulFiber); - } + while (effectfulFiber) { + const current = effectfulFiber.alternate; + // Use Task priority for lifecycle updates + if (effectfulFiber.effectTag & (Update | Callback)) { + commitLifeCycles(current, effectfulFiber); + } - if (effectfulFiber.effectTag & Err) { - commitErrorHandling(effectfulFiber); + if (effectfulFiber.effectTag & Err) { + commitErrorHandling(effectfulFiber); + } + + const next = effectfulFiber.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 + // and lastEffect since they're on every node, not just the effectful + // ones. So we have to clean everything as we reuse nodes anyway. + effectfulFiber.nextEffect = null; + // Ensure that we reset the effectTag here so that we can rely on effect + // tags to reason about the current life-cycle. + effectfulFiber = next; } } catch (error) { - captureError(effectfulFiber, error, false); - } finally { - // Clean-up - priorityContext = previousPriorityContext; - - const next = effectfulFiber.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 - // and lastEffect since they're on every node, not just the effectful - // ones. So we have to clean everything as we reuse nodes anyway. - effectfulFiber.nextEffect = null; - // Ensure that we reset the effectTag here so that we can rely on effect - // tags to reason about the current life-cycle. - effectfulFiber = next; + if (effectfulFiber) { + captureError(effectfulFiber, error, false); + const next = effectfulFiber.nextEffect; + effectfulFiber.nextEffect = null; + effectfulFiber = next; + continue pass2; + } } + priorityContext = previousPriorityContext; + break; } // Lifecycles on the root itself