From c74790fd1ed2b7a654e87cc20195758bf892d849 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 17 Aug 2020 11:18:37 -0400 Subject: [PATCH 1/3] Always skip unmounted/unmounting error boundaries The behavior of error boundaries for passive effects that throw during cleanup was recently changed so that React ignores boundaries which are also unmounting in favor of still-mounted boundaries. This commit implements that same behavior for layout effects (useLayoutEffect and componentWillUnmount). --- .../ReactErrorBoundaries-test.internal.js | 84 +++++++++++++++++ .../src/ReactFiberCommitWork.new.js | 79 ++++++++++++---- .../src/ReactFiberCommitWork.old.js | 93 ++++++++++++++----- .../src/ReactFiberWorkLoop.new.js | 22 ++++- .../src/ReactFiberWorkLoop.old.js | 40 +++++--- .../ReactHooksWithNoopRenderer-test.js | 78 ++++++++++++++++ ...tIncrementalErrorHandling-test.internal.js | 17 ++-- 7 files changed, 351 insertions(+), 62 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 06f51b0f39741..bf8fd72b730cc 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2473,4 +2473,88 @@ describe('ReactErrorBoundaries', () => { 'Caught an error: gotta catch em all.', ); }); + + it('catches errors thrown in componentWillUnmount', () => { + class LocalErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + const {children, id, fallbackID} = this.props; + const {error} = this.state; + if (error) { + Scheduler.unstable_yieldValue(`${id} render error`); + return ; + } + Scheduler.unstable_yieldValue(`${id} render success`); + return children || null; + } + } + + class Component extends React.Component { + render() { + const {id} = this.props; + Scheduler.unstable_yieldValue('Component render ' + id); + return id; + } + } + + class LocalBrokenComponentWillUnmount extends React.Component { + componentWillUnmount() { + Scheduler.unstable_yieldValue( + 'BrokenComponentWillUnmount componentWillUnmount', + ); + throw Error('Expected'); + } + + render() { + Scheduler.unstable_yieldValue('BrokenComponentWillUnmount render'); + return 'broken'; + } + } + + const container = document.createElement('div'); + + ReactDOM.render( + + + + + + , + container, + ); + + expect(container.firstChild.textContent).toBe('sibling'); + expect(container.lastChild.textContent).toBe('broken'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'InnerBoundary render success', + 'BrokenComponentWillUnmount render', + ]); + + ReactDOM.render( + + + , + container, + ); + + // React should skip over the unmounting boundary and find the nearest still-mounted boundary. + expect(container.firstChild.textContent).toBe('OuterFallback'); + expect(container.lastChild.textContent).toBe('OuterFallback'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'BrokenComponentWillUnmount componentWillUnmount', + 'ErrorBoundary static getDerivedStateFromError', + 'OuterBoundary render error', + 'Component render OuterFallback', + ]); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index f086e47cf93b1..c451585a0745f 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -162,7 +162,11 @@ const callComponentWillUnmountWithTimer = function(current, instance) { }; // Capture errors so they don't interrupt unmounting. -function safelyCallComponentWillUnmount(current, instance) { +function safelyCallComponentWillUnmount( + current: Fiber, + instance: any, + nearestMountedAncestor: Fiber, +) { if (__DEV__) { invokeGuardedCallback( null, @@ -173,13 +177,13 @@ function safelyCallComponentWillUnmount(current, instance) { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(current, current.return, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } else { try { callComponentWillUnmountWithTimer(current, instance); } catch (unmountError) { - captureCommitPhaseError(current, current.return, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } } @@ -974,6 +978,7 @@ function commitDetachRef(current: Fiber) { function commitUnmount( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { onCommitUnmount(current); @@ -1001,10 +1006,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, current.return, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, current.return, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); } } } @@ -1018,7 +1023,11 @@ function commitUnmount( safelyDetachRef(current); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount(current, instance); + safelyCallComponentWillUnmount( + current, + instance, + nearestMountedAncestor, + ); } return; } @@ -1031,7 +1040,12 @@ function commitUnmount( // We are also not using this parent because // the portal will get pushed immediately. if (supportsMutation) { - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else if (supportsPersistence) { emptyPortalContainer(current); } @@ -1071,6 +1085,7 @@ function commitUnmount( function commitNestedUnmounts( finishedRoot: FiberRoot, root: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { // While we're inside a removed host node we don't want to call @@ -1080,7 +1095,12 @@ function commitNestedUnmounts( // we do an inner loop while we're still inside the host node. let node: Fiber = root; while (true) { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because they may contain more composite or host nodes. // Skip portals because commitUnmount() currently visits them recursively. if ( @@ -1361,9 +1381,10 @@ function insertOrAppendPlacementNode( } function unmountHostComponents( - finishedRoot, - current, - renderPriorityLevel, + finishedRoot: FiberRoot, + current: Fiber, + nearestMountedAncestor: Fiber, + renderPriorityLevel: ReactPriorityLevel, ): void { // We only have the top Fiber that was deleted but we need to recurse down its // children to find all the terminal nodes. @@ -1412,7 +1433,12 @@ function unmountHostComponents( } if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1429,7 +1455,12 @@ function unmountHostComponents( // Don't visit children because we already visited them. } else if (enableFundamentalAPI && node.tag === FundamentalComponent) { const fundamentalNode = node.stateNode.instance; - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1481,7 +1512,12 @@ function unmountHostComponents( continue; } } else { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because we may find more host components below. if (node.child !== null) { node.child.return = node; @@ -1511,15 +1547,26 @@ function unmountHostComponents( function commitDeletion( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { if (supportsMutation) { // Recursively delete all host nodes from the parent. // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else { // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(finishedRoot, current, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } const alternate = current.alternate; detachFiberMutation(current); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 8e280e1600510..81b226670f179 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -153,7 +153,11 @@ const callComponentWillUnmountWithTimer = function(current, instance) { }; // Capture errors so they don't interrupt unmounting. -function safelyCallComponentWillUnmount(current, instance) { +function safelyCallComponentWillUnmount( + current: Fiber, + instance: any, + nearestMountedAncestor: Fiber | null, +) { if (__DEV__) { invokeGuardedCallback( null, @@ -164,13 +168,13 @@ function safelyCallComponentWillUnmount(current, instance) { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(current, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } else { try { callComponentWillUnmountWithTimer(current, instance); } catch (unmountError) { - captureCommitPhaseError(current, unmountError); + captureCommitPhaseError(current, nearestMountedAncestor, unmountError); } } } @@ -183,13 +187,13 @@ function safelyDetachRef(current: Fiber) { invokeGuardedCallback(null, ref, null, null); if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, refError); + captureCommitPhaseError(current, current.return, refError); } } else { try { ref(null); } catch (refError) { - captureCommitPhaseError(current, refError); + captureCommitPhaseError(current, current.return, refError); } } } else { @@ -198,18 +202,22 @@ function safelyDetachRef(current: Fiber) { } } -function safelyCallDestroy(current, destroy) { +function safelyCallDestroy( + current: Fiber, + nearestMountedAncestor: Fiber | null, + destroy: () => void, +) { if (__DEV__) { invokeGuardedCallback(null, destroy, null); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(current, error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } else { try { destroy(); } catch (error) { - captureCommitPhaseError(current, error); + captureCommitPhaseError(current, nearestMountedAncestor, error); } } } @@ -866,6 +874,7 @@ function commitDetachRef(current: Fiber) { function commitUnmount( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber | null, renderPriorityLevel: ReactPriorityLevel, ): void { onCommitUnmount(current); @@ -895,10 +904,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); } } } @@ -912,7 +921,11 @@ function commitUnmount( safelyDetachRef(current); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount(current, instance); + safelyCallComponentWillUnmount( + current, + instance, + nearestMountedAncestor, + ); } return; } @@ -925,7 +938,12 @@ function commitUnmount( // We are also not using this parent because // the portal will get pushed immediately. if (supportsMutation) { - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else if (supportsPersistence) { emptyPortalContainer(current); } @@ -965,6 +983,7 @@ function commitUnmount( function commitNestedUnmounts( finishedRoot: FiberRoot, root: Fiber, + nearestMountedAncestor: Fiber | null, renderPriorityLevel: ReactPriorityLevel, ): void { // While we're inside a removed host node we don't want to call @@ -974,7 +993,12 @@ function commitNestedUnmounts( // we do an inner loop while we're still inside the host node. let node: Fiber = root; while (true) { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because they may contain more composite or host nodes. // Skip portals because commitUnmount() currently visits them recursively. if ( @@ -1263,9 +1287,10 @@ function insertOrAppendPlacementNode( } function unmountHostComponents( - finishedRoot, - current, - renderPriorityLevel, + finishedRoot: FiberRoot, + current: Fiber, + nearestMountedAncestor: Fiber | null, + renderPriorityLevel: ReactPriorityLevel, ): void { // We only have the top Fiber that was deleted but we need to recurse down its // children to find all the terminal nodes. @@ -1314,7 +1339,12 @@ function unmountHostComponents( } if (node.tag === HostComponent || node.tag === HostText) { - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1331,7 +1361,12 @@ function unmountHostComponents( // Don't visit children because we already visited them. } else if (enableFundamentalAPI && node.tag === FundamentalComponent) { const fundamentalNode = node.stateNode.instance; - commitNestedUnmounts(finishedRoot, node, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // After all the children have unmounted, it is now safe to remove the // node from the tree. if (currentParentIsContainer) { @@ -1383,7 +1418,12 @@ function unmountHostComponents( continue; } } else { - commitUnmount(finishedRoot, node, renderPriorityLevel); + commitUnmount( + finishedRoot, + node, + nearestMountedAncestor, + renderPriorityLevel, + ); // Visit children because we may find more host components below. if (node.child !== null) { node.child.return = node; @@ -1413,15 +1453,26 @@ function unmountHostComponents( function commitDeletion( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber | null, renderPriorityLevel: ReactPriorityLevel, ): void { if (supportsMutation) { // Recursively delete all host nodes from the parent. // Detach refs and call componentWillUnmount() on the whole subtree. - unmountHostComponents(finishedRoot, current, renderPriorityLevel); + unmountHostComponents( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } else { // Detach refs and call componentWillUnmount() on the whole subtree. - commitNestedUnmounts(finishedRoot, current, renderPriorityLevel); + commitNestedUnmounts( + finishedRoot, + current, + nearestMountedAncestor, + renderPriorityLevel, + ); } const alternate = current.alternate; detachFiberMutation(current); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 87da91d58c5f4..578ffd427c794 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2462,13 +2462,18 @@ function commitBeforeMutationEffectsDeletions(deletions: Array) { function commitMutationEffects( firstChild: Fiber, root: FiberRoot, - renderPriorityLevel, + renderPriorityLevel: ReactPriorityLevel, ) { let fiber = firstChild; while (fiber !== null) { const deletions = fiber.deletions; if (deletions !== null) { - commitMutationEffectsDeletions(deletions, root, renderPriorityLevel); + commitMutationEffectsDeletions( + deletions, + fiber, + root, + renderPriorityLevel, + ); } if (fiber.child !== null) { @@ -2577,6 +2582,7 @@ function commitMutationEffectsImpl( function commitMutationEffectsDeletions( deletions: Array, + nearestMountedAncestor: Fiber, root: FiberRoot, renderPriorityLevel, ) { @@ -2589,17 +2595,23 @@ function commitMutationEffectsDeletions( null, root, childToDelete, + nearestMountedAncestor, renderPriorityLevel, ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(childToDelete, childToDelete.return, error); + captureCommitPhaseError(childToDelete, nearestMountedAncestor, error); } } else { try { - commitDeletion(root, childToDelete, renderPriorityLevel); + commitDeletion( + root, + childToDelete, + nearestMountedAncestor, + renderPriorityLevel, + ); } catch (error) { - captureCommitPhaseError(childToDelete, childToDelete.return, error); + captureCommitPhaseError(childToDelete, nearestMountedAncestor, error); } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 84188fea37427..d40a164b94abc 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2084,7 +2084,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } else { @@ -2092,7 +2092,7 @@ function commitRootImpl(root, renderPriorityLevel) { commitBeforeMutationEffects(); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } @@ -2121,7 +2121,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } else { @@ -2129,7 +2129,7 @@ function commitRootImpl(root, renderPriorityLevel) { commitMutationEffects(root, renderPriorityLevel); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } @@ -2156,7 +2156,7 @@ function commitRootImpl(root, renderPriorityLevel) { if (hasCaughtError()) { invariant(nextEffect !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } else { @@ -2164,7 +2164,7 @@ function commitRootImpl(root, renderPriorityLevel) { commitLayoutEffects(root, lanes); } catch (error) { invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); + captureCommitPhaseError(nextEffect, nextEffect.return, error); nextEffect = nextEffect.nextEffect; } } @@ -2365,7 +2365,10 @@ function commitBeforeMutationEffects() { } } -function commitMutationEffects(root: FiberRoot, renderPriorityLevel) { +function commitMutationEffects( + root: FiberRoot, + renderPriorityLevel: ReactPriorityLevel, +) { // TODO: Should probably move the bulk of this function to commitWork. while (nextEffect !== null) { setCurrentDebugFiberInDEV(nextEffect); @@ -2436,7 +2439,12 @@ function commitMutationEffects(root: FiberRoot, renderPriorityLevel) { break; } case Deletion: { - commitDeletion(root, nextEffect, renderPriorityLevel); + commitDeletion( + root, + nextEffect, + nextEffect.return, + renderPriorityLevel, + ); break; } } @@ -2647,7 +2655,7 @@ function flushPassiveEffectsImpl() { if (hasCaughtError()) { invariant(fiber !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { @@ -2668,7 +2676,7 @@ function flushPassiveEffectsImpl() { } } catch (error) { invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2695,7 +2703,7 @@ function flushPassiveEffectsImpl() { if (hasCaughtError()) { invariant(fiber !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { @@ -2717,7 +2725,7 @@ function flushPassiveEffectsImpl() { } } catch (error) { invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2816,7 +2824,11 @@ function captureCommitPhaseErrorOnRoot( } } -export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { +export function captureCommitPhaseError( + sourceFiber: Fiber, + nearestMountedAncestor: Fiber | null, + error: mixed, +) { if (sourceFiber.tag === HostRoot) { // Error was thrown at the root. There is no parent, so the root // itself should capture it. @@ -2824,7 +2836,7 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { return; } - let fiber = sourceFiber.return; + let fiber = nearestMountedAncestor; while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 7ff61126f648a..75e52dd951b79 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2797,6 +2797,84 @@ describe('ReactHooksWithNoopRenderer', () => { 'Mount normal [current: 1]', ]); }); + + it('catches errors thrown in useLayoutEffect', () => { + class ErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + const {children, id, fallbackID} = this.props; + const {error} = this.state; + if (error) { + Scheduler.unstable_yieldValue(`${id} render error`); + return ; + } + Scheduler.unstable_yieldValue(`${id} render success`); + return children || null; + } + } + + function Component({id}) { + Scheduler.unstable_yieldValue('Component render ' + id); + return ; + } + + function BrokenLayoutEffectDestroy() { + useLayoutEffect(() => { + return () => { + Scheduler.unstable_yieldValue( + 'BrokenLayoutEffectDestroy useLayoutEffect destroy', + ); + throw Error('Expected'); + }; + }, []); + + Scheduler.unstable_yieldValue('BrokenLayoutEffectDestroy render'); + return ; + } + + ReactNoop.render( + + + + + + , + ); + + expect(Scheduler).toFlushAndYield([ + 'OuterBoundary render success', + 'Component render sibling', + 'InnerBoundary render success', + 'BrokenLayoutEffectDestroy render', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('sibling'), + span('broken'), + ]); + + ReactNoop.render( + + + , + ); + + // React should skip over the unmounting boundary and find the nearest still-mounted boundary. + expect(Scheduler).toFlushAndYield([ + 'OuterBoundary render success', + 'Component render sibling', + 'BrokenLayoutEffectDestroy useLayoutEffect destroy', + 'ErrorBoundary static getDerivedStateFromError', + 'OuterBoundary render error', + 'Component render OuterFallback', + ]); + expect(ReactNoop.getChildren()).toEqual([span('OuterFallback')]); + }); }); describe('useCallback', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 503dc98f0de41..f41c9f661a14b 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -992,12 +992,17 @@ describe('ReactIncrementalErrorHandling', () => { ReactNoop.render(); expect(Scheduler).toFlushWithoutYielding(); - ReactNoop.render(null); - expect(Scheduler).toFlushAndYield([ - // Parent unmounts before the error is thrown. - 'Parent componentWillUnmount', - 'ThrowsOnUnmount componentWillUnmount', - ]); + + // Because the error boundary is also unmounting, + // an error in ThrowsOnUnmount should be rethrown. + expect(() => { + ReactNoop.render(null); + expect(Scheduler).toFlushAndYield([ + 'Parent componentWillUnmount', + 'ThrowsOnUnmount componentWillUnmount', + ]); + }).toThrow('unmount error'); + ReactNoop.render(); }); From a994a537cfd0aff8bc38668d15af5eebc551b61f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 17 Aug 2020 12:10:09 -0400 Subject: [PATCH 2/3] Move skip-unmounting boundaries behavior behind a feature flag --- .../src/__tests__/ReactErrorBoundaries-test.internal.js | 1 + packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 9 ++++++++- packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 9 ++++++++- .../src/__tests__/ReactHooksWithNoopRenderer-test.js | 1 + .../ReactIncrementalErrorHandling-test.internal.js | 1 + packages/shared/ReactFeatureFlags.js | 6 ++++++ packages/shared/forks/ReactFeatureFlags.native-fb.js | 1 + packages/shared/forks/ReactFeatureFlags.native-oss.js | 1 + packages/shared/forks/ReactFeatureFlags.test-renderer.js | 1 + .../forks/ReactFeatureFlags.test-renderer.native.js | 1 + .../shared/forks/ReactFeatureFlags.test-renderer.www.js | 1 + packages/shared/forks/ReactFeatureFlags.testing.js | 1 + packages/shared/forks/ReactFeatureFlags.testing.www.js | 1 + packages/shared/forks/ReactFeatureFlags.www-dynamic.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 1 + 15 files changed, 34 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index bf8fd72b730cc..9799f0f0453ae 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2474,6 +2474,7 @@ describe('ReactErrorBoundaries', () => { ); }); + // @gate skipUnmountedBoundaries it('catches errors thrown in componentWillUnmount', () => { class LocalErrorBoundary extends React.Component { state = {error: null}; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 578ffd427c794..c7d1faef4cfbf 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -30,6 +30,7 @@ import { enableDebugTracing, enableSchedulingProfiler, enableScopeAPI, + skipUnmountedBoundaries, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2950,7 +2951,13 @@ export function captureCommitPhaseError( return; } - let fiber = nearestMountedAncestor; + let fiber = null; + if (skipUnmountedBoundaries) { + fiber = nearestMountedAncestor; + } else { + fiber = sourceFiber.return; + } + while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index d40a164b94abc..ff3fd6a9d0e41 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -30,6 +30,7 @@ import { enableDebugTracing, enableSchedulingProfiler, enableScopeAPI, + skipUnmountedBoundaries, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2836,7 +2837,13 @@ export function captureCommitPhaseError( return; } - let fiber = nearestMountedAncestor; + let fiber = null; + if (skipUnmountedBoundaries) { + fiber = nearestMountedAncestor; + } else { + fiber = sourceFiber.return; + } + while (fiber !== null) { if (fiber.tag === HostRoot) { captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 75e52dd951b79..7fa56978e74a2 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2798,6 +2798,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + // @gate skipUnmountedBoundaries it('catches errors thrown in useLayoutEffect', () => { class ErrorBoundary extends React.Component { state = {error: null}; diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index f41c9f661a14b..06668b4eb3896 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -961,6 +961,7 @@ describe('ReactIncrementalErrorHandling', () => { expect(Scheduler).toFlushAndYield(['Foo']); }); + // @gate skipUnmountedBoundaries it('should not attempt to recover an unmounting error boundary', () => { class Parent extends React.Component { componentWillUnmount() { diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 634c19b14ea52..2ae1805af95c1 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -92,6 +92,12 @@ export const enableComponentStackLocations = true; export const enableNewReconciler = false; +// Errors that are thrown while unmounting (or after in the case of passive effects) +// should bypass any error boundaries that are also unmounting (or have unmounted) +// and be handled by the nearest still-mounted boundary. +// If there are no still-mounted boundaries, the errors should be rethrown. +export const skipUnmountedBoundaries = false; + // -------------------------- // Future APIs to be deprecated // -------------------------- diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 664a11a43b981..6b61818c6d044 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -43,6 +43,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 9632b8b7da08b..c8598c23cd924 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 6f348ed149b22..47a729c1d0a09 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index ea8f7d4e0a97e..4af72e9d901a7 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 3e5046f0fccd8..7c25d50d07de9 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 8d4fe939dc09b..5e530afbd9db7 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = false; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index d3796937a3028..e11f9f9d13381 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -42,6 +42,7 @@ export const warnAboutSpreadingKeyToJSX = false; export const enableComponentStackLocations = true; export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; +export const skipUnmountedBoundaries = __EXPERIMENTAL__; export const enableNewReconciler = false; export const deferRenderPhaseUpdateToNextBatch = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 9b23645a3d87e..c7df70d192989 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -18,6 +18,7 @@ export const disableInputAttributeSyncing = __VARIANT__; export const enableFilterEmptyStringAttributesDOM = __VARIANT__; export const enableLegacyFBSupport = __VARIANT__; export const decoupleUpdatePriorityFromScheduler = __VARIANT__; +export const skipUnmountedBoundaries = __VARIANT__; // Enable this flag to help with concurrent mode debugging. // It logs information to the console about React scheduling, rendering, and commit phases. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 1c637762fe6b9..35f5767413fc3 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -26,6 +26,7 @@ export const { deferRenderPhaseUpdateToNextBatch, decoupleUpdatePriorityFromScheduler, enableDebugTracing, + skipUnmountedBoundaries, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. From a048c8536b9213b4283ac8f74242cf4a150e0bc1 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 17 Aug 2020 14:49:41 -0400 Subject: [PATCH 3/3] Skip unmounted error boundaries for errors thrown during ref cleanup --- .../ReactErrorBoundaries-test.internal.js | 86 +++++++++++++++++++ .../src/ReactFiberCommitWork.new.js | 12 +-- .../src/ReactFiberCommitWork.old.js | 12 +-- 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 9799f0f0453ae..204c67ba52846 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -2558,4 +2558,90 @@ describe('ReactErrorBoundaries', () => { 'Component render OuterFallback', ]); }); + + // @gate skipUnmountedBoundaries + it('catches errors thrown while detaching refs', () => { + class LocalErrorBoundary extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + const {children, id, fallbackID} = this.props; + const {error} = this.state; + if (error) { + Scheduler.unstable_yieldValue(`${id} render error`); + return ; + } + Scheduler.unstable_yieldValue(`${id} render success`); + return children || null; + } + } + + class Component extends React.Component { + render() { + const {id} = this.props; + Scheduler.unstable_yieldValue('Component render ' + id); + return id; + } + } + + class LocalBrokenCallbackRef extends React.Component { + _ref = ref => { + Scheduler.unstable_yieldValue('LocalBrokenCallbackRef ref ' + !!ref); + if (ref === null) { + throw Error('Expected'); + } + }; + + render() { + Scheduler.unstable_yieldValue('LocalBrokenCallbackRef render'); + return
ref
; + } + } + + const container = document.createElement('div'); + + ReactDOM.render( + + + + + + , + container, + ); + + expect(container.firstChild.textContent).toBe('sibling'); + expect(container.lastChild.textContent).toBe('ref'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'InnerBoundary render success', + 'LocalBrokenCallbackRef render', + 'LocalBrokenCallbackRef ref true', + ]); + + ReactDOM.render( + + + , + container, + ); + + // React should skip over the unmounting boundary and find the nearest still-mounted boundary. + expect(container.firstChild.textContent).toBe('OuterFallback'); + expect(container.lastChild.textContent).toBe('OuterFallback'); + expect(Scheduler).toHaveYielded([ + 'OuterBoundary render success', + 'Component render sibling', + 'LocalBrokenCallbackRef ref false', + 'ErrorBoundary static getDerivedStateFromError', + 'OuterBoundary render error', + 'Component render OuterFallback', + ]); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index c451585a0745f..727df9a08ffac 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -188,7 +188,7 @@ function safelyCallComponentWillUnmount( } } -function safelyDetachRef(current: Fiber) { +function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber) { const ref = current.ref; if (ref !== null) { if (typeof ref === 'function') { @@ -196,13 +196,13 @@ function safelyDetachRef(current: Fiber) { invokeGuardedCallback(null, ref, null, null); if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } else { try { ref(null); } catch (refError) { - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } } else { @@ -1020,7 +1020,7 @@ function commitUnmount( return; } case ClassComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { safelyCallComponentWillUnmount( @@ -1032,7 +1032,7 @@ function commitUnmount( return; } case HostComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); return; } case HostPortal: { @@ -1075,7 +1075,7 @@ function commitUnmount( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); } return; } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 81b226670f179..3265edd9cf584 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -179,7 +179,7 @@ function safelyCallComponentWillUnmount( } } -function safelyDetachRef(current: Fiber) { +function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { const ref = current.ref; if (ref !== null) { if (typeof ref === 'function') { @@ -187,13 +187,13 @@ function safelyDetachRef(current: Fiber) { invokeGuardedCallback(null, ref, null, null); if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } else { try { ref(null); } catch (refError) { - captureCommitPhaseError(current, current.return, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } } else { @@ -918,7 +918,7 @@ function commitUnmount( return; } case ClassComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { safelyCallComponentWillUnmount( @@ -930,7 +930,7 @@ function commitUnmount( return; } case HostComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); return; } case HostPortal: { @@ -973,7 +973,7 @@ function commitUnmount( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); } return; }