From 9af0b3a8f639170bda3139f6c669357cc48b88c8 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 22 Jan 2021 15:24:34 -0500 Subject: [PATCH] Improve error boundary handling for unmounted subtrees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A passive effect's cleanup function may throw after an unmount. Prior to this commit, such an error would be ignored. (React would not notify any error boundaries.) After this commit, React will skip any unmounted boundaries and look for a still-mounted boundary. If one is found, it will call getDerivedStateFromError and/or componentDidCatch (depending on the type of boundary). Unmounted boundaries will be ignored, but as they have been unmounted– this seems appropriate. --- .../ReactErrorBoundaries-test.internal.js | 170 ++++++++++ .../src/ReactFiberCommitWork.new.js | 196 ++++++++--- .../src/ReactFiberCommitWork.old.js | 196 ++++++++--- .../src/ReactFiberWorkLoop.new.js | 15 +- .../src/ReactFiberWorkLoop.old.js | 16 +- .../ReactHooksWithNoopRenderer-test.js | 306 ++++++++++++++++++ ...tIncrementalErrorHandling-test.internal.js | 18 +- packages/shared/ReactFeatureFlags.js | 6 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 17 files changed, 818 insertions(+), 114 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index 50eae32123b71..02e4833685528 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -42,6 +42,7 @@ describe('ReactErrorBoundaries', () => { PropTypes = require('prop-types'); ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; + ReactFeatureFlags.skipUnmountedBoundaries = true; ReactDOM = require('react-dom'); React = require('react'); act = require('react-dom/test-utils').unstable_concurrentAct; @@ -2473,4 +2474,173 @@ 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', + ]); + }); + + 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 2d50f255ee21f..29fbd87720128 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -177,7 +177,11 @@ const callComponentWillUnmountWithTimer = function(current, instance) { }; // Capture errors so they don't interrupt unmounting. -function safelyCallComponentWillUnmount(current: Fiber, instance: any) { +function safelyCallComponentWillUnmount( + current: Fiber, + nearestMountedAncestor: Fiber | null, + instance: any, +) { if (__DEV__) { invokeGuardedCallback( null, @@ -188,18 +192,18 @@ function safelyCallComponentWillUnmount(current: Fiber, instance: any) { ); 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); } } } -function safelyDetachRef(current: Fiber) { +function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { const ref = current.ref; if (ref !== null) { if (typeof ref === 'function') { @@ -218,7 +222,7 @@ function safelyDetachRef(current: Fiber) { if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } else { try { @@ -237,7 +241,7 @@ function safelyDetachRef(current: Fiber) { ref(null); } } catch (refError) { - captureCommitPhaseError(current, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } } else { @@ -246,18 +250,22 @@ function safelyDetachRef(current: Fiber) { } } -function safelyCallDestroy(current: Fiber, destroy: () => void) { +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); } } } @@ -321,14 +329,14 @@ function commitBeforeMutationEffects_complete() { ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitBeforeMutationEffectsOnFiber(fiber); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } @@ -462,7 +470,11 @@ function commitBeforeMutationEffectsDeletion(deletion: Fiber) { } } -function commitHookEffectListUnmount(flags: HookFlags, finishedWork: Fiber) { +function commitHookEffectListUnmount( + flags: HookFlags, + finishedWork: Fiber, + nearestMountedAncestor: Fiber | null, +) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -474,7 +486,7 @@ function commitHookEffectListUnmount(flags: HookFlags, finishedWork: Fiber) { const destroy = effect.destroy; effect.destroy = undefined; if (destroy !== undefined) { - safelyCallDestroy(finishedWork, destroy); + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); } } effect = effect.next; @@ -1049,6 +1061,7 @@ function commitDetachRef(current: Fiber) { function commitUnmount( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { onCommitUnmount(current); @@ -1075,10 +1088,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); } } } @@ -1089,15 +1102,19 @@ function commitUnmount( return; } case ClassComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount(current, instance); + safelyCallComponentWillUnmount( + current, + nearestMountedAncestor, + instance, + ); } return; } case HostComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); return; } case HostPortal: { @@ -1105,7 +1122,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); } @@ -1135,7 +1157,7 @@ function commitUnmount( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); } return; } @@ -1145,6 +1167,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 @@ -1154,7 +1177,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 ( @@ -1455,6 +1483,7 @@ function insertOrAppendPlacementNode( function unmountHostComponents( 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 @@ -1504,7 +1533,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) { @@ -1521,7 +1555,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) { @@ -1573,7 +1612,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; @@ -1603,15 +1647,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); @@ -1642,12 +1697,17 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { commitHookEffectListUnmount( HookLayout | HookHasEffect, finishedWork, + finishedWork.return, ); } finally { recordLayoutEffectDuration(finishedWork); } } else { - commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); } return; } @@ -1701,12 +1761,20 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { ) { try { startLayoutEffectTimer(); - commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); } finally { recordLayoutEffectDuration(finishedWork); } } else { - commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); } return; } @@ -1958,17 +2026,18 @@ function commitMutationEffects_begin( null, root, childToDelete, + fiber, renderPriorityLevel, ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(childToDelete, error); + captureCommitPhaseError(childToDelete, fiber, error); } } else { try { - commitDeletion(root, childToDelete, renderPriorityLevel); + commitDeletion(root, childToDelete, fiber, renderPriorityLevel); } catch (error) { - captureCommitPhaseError(childToDelete, error); + captureCommitPhaseError(childToDelete, fiber, error); } } } @@ -2002,14 +2071,14 @@ function commitMutationEffects_complete( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitMutationEffectsOnFiber(fiber, root, renderPriorityLevel); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } @@ -2144,14 +2213,14 @@ function commitLayoutMountEffects_complete( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitLayoutEffectOnFiber(root, current, fiber, committedLanes); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2211,14 +2280,14 @@ function commitPassiveMountEffects_complete( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitPassiveMountOnFiber(root, fiber); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2282,7 +2351,10 @@ function commitPassiveUnmountEffects_begin() { for (let i = 0; i < deletions.length; i++) { const fiberToDelete = deletions[i]; nextEffect = fiberToDelete; - commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete); + commitPassiveUnmountEffectsInsideOfDeletedTree_begin( + fiberToDelete, + fiber, + ); // Now that passive effects have been processed, it's safe to detach lingering pointers. const alternate = fiberToDelete.alternate; @@ -2343,10 +2415,18 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { finishedWork.mode & ProfileMode ) { startPassiveEffectTimer(); - commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); recordPassiveEffectDuration(finishedWork); } else { - commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); } break; } @@ -2355,6 +2435,7 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( deletedSubtreeRoot: Fiber, + nearestMountedAncestor: Fiber | null, ) { while (nextEffect !== null) { const fiber = nextEffect; @@ -2362,7 +2443,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( // Deletion effects fire in parent -> child order // TODO: Check if fiber has a PassiveStatic flag setCurrentDebugFiberInDEV(fiber); - commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); + commitPassiveUnmountInsideDeletedTreeOnFiber(fiber, nearestMountedAncestor); resetCurrentDebugFiberInDEV(); const child = fiber.child; @@ -2399,7 +2480,10 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( } } -function commitPassiveUnmountInsideDeletedTreeOnFiber(current: Fiber): void { +function commitPassiveUnmountInsideDeletedTreeOnFiber( + current: Fiber, + nearestMountedAncestor: Fiber | null, +): void { switch (current.tag) { case FunctionComponent: case ForwardRef: @@ -2410,10 +2494,18 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber(current: Fiber): void { current.mode & ProfileMode ) { startPassiveEffectTimer(); - commitHookEffectListUnmount(HookPassive, current); + commitHookEffectListUnmount( + HookPassive, + current, + nearestMountedAncestor, + ); recordPassiveEffectDuration(current); } else { - commitHookEffectListUnmount(HookPassive, current); + commitHookEffectListUnmount( + HookPassive, + current, + nearestMountedAncestor, + ); } break; } @@ -2454,7 +2546,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const mountError = clearCaughtError(); - captureCommitPhaseError(fiber, mountError); + captureCommitPhaseError(fiber, fiber.return, mountError); } break; } @@ -2463,7 +2555,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { invokeGuardedCallback(null, instance.componentDidMount, instance); if (hasCaughtError()) { const mountError = clearCaughtError(); - captureCommitPhaseError(fiber, mountError); + captureCommitPhaseError(fiber, fiber.return, mountError); } break; } @@ -2488,7 +2580,7 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const mountError = clearCaughtError(); - captureCommitPhaseError(fiber, mountError); + captureCommitPhaseError(fiber, fiber.return, mountError); } break; } @@ -2514,7 +2606,7 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(fiber, unmountError); + captureCommitPhaseError(fiber, fiber.return, unmountError); } break; } @@ -2526,12 +2618,12 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { safelyCallComponentWillUnmount, null, fiber, - instance, fiber.return, + instance, ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(fiber, unmountError); + captureCommitPhaseError(fiber, fiber.return, unmountError); } } break; @@ -2558,7 +2650,7 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(fiber, unmountError); + captureCommitPhaseError(fiber, fiber.return, unmountError); } break; } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 642f6dce02aa8..d5ce4dd688d63 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -177,7 +177,11 @@ const callComponentWillUnmountWithTimer = function(current, instance) { }; // Capture errors so they don't interrupt unmounting. -function safelyCallComponentWillUnmount(current: Fiber, instance: any) { +function safelyCallComponentWillUnmount( + current: Fiber, + nearestMountedAncestor: Fiber | null, + instance: any, +) { if (__DEV__) { invokeGuardedCallback( null, @@ -188,18 +192,18 @@ function safelyCallComponentWillUnmount(current: Fiber, instance: any) { ); 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); } } } -function safelyDetachRef(current: Fiber) { +function safelyDetachRef(current: Fiber, nearestMountedAncestor: Fiber | null) { const ref = current.ref; if (ref !== null) { if (typeof ref === 'function') { @@ -218,7 +222,7 @@ function safelyDetachRef(current: Fiber) { if (hasCaughtError()) { const refError = clearCaughtError(); - captureCommitPhaseError(current, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } else { try { @@ -237,7 +241,7 @@ function safelyDetachRef(current: Fiber) { ref(null); } } catch (refError) { - captureCommitPhaseError(current, refError); + captureCommitPhaseError(current, nearestMountedAncestor, refError); } } } else { @@ -246,18 +250,22 @@ function safelyDetachRef(current: Fiber) { } } -function safelyCallDestroy(current: Fiber, destroy: () => void) { +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); } } } @@ -321,14 +329,14 @@ function commitBeforeMutationEffects_complete() { ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitBeforeMutationEffectsOnFiber(fiber); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } @@ -462,7 +470,11 @@ function commitBeforeMutationEffectsDeletion(deletion: Fiber) { } } -function commitHookEffectListUnmount(flags: HookFlags, finishedWork: Fiber) { +function commitHookEffectListUnmount( + flags: HookFlags, + finishedWork: Fiber, + nearestMountedAncestor: Fiber | null, +) { const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; if (lastEffect !== null) { @@ -474,7 +486,7 @@ function commitHookEffectListUnmount(flags: HookFlags, finishedWork: Fiber) { const destroy = effect.destroy; effect.destroy = undefined; if (destroy !== undefined) { - safelyCallDestroy(finishedWork, destroy); + safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy); } } effect = effect.next; @@ -1049,6 +1061,7 @@ function commitDetachRef(current: Fiber) { function commitUnmount( finishedRoot: FiberRoot, current: Fiber, + nearestMountedAncestor: Fiber, renderPriorityLevel: ReactPriorityLevel, ): void { onCommitUnmount(current); @@ -1075,10 +1088,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, nearestMountedAncestor, destroy); } } } @@ -1089,15 +1102,19 @@ function commitUnmount( return; } case ClassComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); const instance = current.stateNode; if (typeof instance.componentWillUnmount === 'function') { - safelyCallComponentWillUnmount(current, instance); + safelyCallComponentWillUnmount( + current, + nearestMountedAncestor, + instance, + ); } return; } case HostComponent: { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); return; } case HostPortal: { @@ -1105,7 +1122,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); } @@ -1135,7 +1157,7 @@ function commitUnmount( } case ScopeComponent: { if (enableScopeAPI) { - safelyDetachRef(current); + safelyDetachRef(current, nearestMountedAncestor); } return; } @@ -1145,6 +1167,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 @@ -1154,7 +1177,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 ( @@ -1455,6 +1483,7 @@ function insertOrAppendPlacementNode( function unmountHostComponents( 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 @@ -1504,7 +1533,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) { @@ -1521,7 +1555,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) { @@ -1573,7 +1612,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; @@ -1603,15 +1647,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); @@ -1642,12 +1697,17 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { commitHookEffectListUnmount( HookLayout | HookHasEffect, finishedWork, + finishedWork.return, ); } finally { recordLayoutEffectDuration(finishedWork); } } else { - commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); } return; } @@ -1701,12 +1761,20 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void { ) { try { startLayoutEffectTimer(); - commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); } finally { recordLayoutEffectDuration(finishedWork); } } else { - commitHookEffectListUnmount(HookLayout | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookLayout | HookHasEffect, + finishedWork, + finishedWork.return, + ); } return; } @@ -1958,17 +2026,18 @@ function commitMutationEffects_begin( null, root, childToDelete, + fiber, renderPriorityLevel, ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(childToDelete, error); + captureCommitPhaseError(childToDelete, fiber, error); } } else { try { - commitDeletion(root, childToDelete, renderPriorityLevel); + commitDeletion(root, childToDelete, fiber, renderPriorityLevel); } catch (error) { - captureCommitPhaseError(childToDelete, error); + captureCommitPhaseError(childToDelete, fiber, error); } } } @@ -2002,14 +2071,14 @@ function commitMutationEffects_complete( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitMutationEffectsOnFiber(fiber, root, renderPriorityLevel); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } @@ -2144,14 +2213,14 @@ function commitLayoutMountEffects_complete( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitLayoutEffectOnFiber(root, current, fiber, committedLanes); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2211,14 +2280,14 @@ function commitPassiveMountEffects_complete( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitPassiveMountOnFiber(root, fiber); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -2282,7 +2351,10 @@ function commitPassiveUnmountEffects_begin() { for (let i = 0; i < deletions.length; i++) { const fiberToDelete = deletions[i]; nextEffect = fiberToDelete; - commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete); + commitPassiveUnmountEffectsInsideOfDeletedTree_begin( + fiberToDelete, + fiber, + ); // Now that passive effects have been processed, it's safe to detach lingering pointers. const alternate = fiberToDelete.alternate; @@ -2343,10 +2415,18 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { finishedWork.mode & ProfileMode ) { startPassiveEffectTimer(); - commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); recordPassiveEffectDuration(finishedWork); } else { - commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork); + commitHookEffectListUnmount( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); } break; } @@ -2355,6 +2435,7 @@ function commitPassiveUnmountOnFiber(finishedWork: Fiber): void { function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( deletedSubtreeRoot: Fiber, + nearestMountedAncestor: Fiber | null, ) { while (nextEffect !== null) { const fiber = nextEffect; @@ -2362,7 +2443,7 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( // Deletion effects fire in parent -> child order // TODO: Check if fiber has a PassiveStatic flag setCurrentDebugFiberInDEV(fiber); - commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); + commitPassiveUnmountInsideDeletedTreeOnFiber(fiber, nearestMountedAncestor); resetCurrentDebugFiberInDEV(); const child = fiber.child; @@ -2399,7 +2480,10 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( } } -function commitPassiveUnmountInsideDeletedTreeOnFiber(current: Fiber): void { +function commitPassiveUnmountInsideDeletedTreeOnFiber( + current: Fiber, + nearestMountedAncestor: Fiber | null, +): void { switch (current.tag) { case FunctionComponent: case ForwardRef: @@ -2410,10 +2494,18 @@ function commitPassiveUnmountInsideDeletedTreeOnFiber(current: Fiber): void { current.mode & ProfileMode ) { startPassiveEffectTimer(); - commitHookEffectListUnmount(HookPassive, current); + commitHookEffectListUnmount( + HookPassive, + current, + nearestMountedAncestor, + ); recordPassiveEffectDuration(current); } else { - commitHookEffectListUnmount(HookPassive, current); + commitHookEffectListUnmount( + HookPassive, + current, + nearestMountedAncestor, + ); } break; } @@ -2454,7 +2546,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const mountError = clearCaughtError(); - captureCommitPhaseError(fiber, mountError); + captureCommitPhaseError(fiber, fiber.return, mountError); } break; } @@ -2463,7 +2555,7 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void { invokeGuardedCallback(null, instance.componentDidMount, instance); if (hasCaughtError()) { const mountError = clearCaughtError(); - captureCommitPhaseError(fiber, mountError); + captureCommitPhaseError(fiber, fiber.return, mountError); } break; } @@ -2488,7 +2580,7 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const mountError = clearCaughtError(); - captureCommitPhaseError(fiber, mountError); + captureCommitPhaseError(fiber, fiber.return, mountError); } break; } @@ -2514,7 +2606,7 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(fiber, unmountError); + captureCommitPhaseError(fiber, fiber.return, unmountError); } break; } @@ -2526,12 +2618,12 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void { safelyCallComponentWillUnmount, null, fiber, - instance, fiber.return, + instance, ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(fiber, unmountError); + captureCommitPhaseError(fiber, fiber.return, unmountError); } } break; @@ -2558,7 +2650,7 @@ function invokePassiveEffectUnmountInDEV(fiber: Fiber): void { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(fiber, unmountError); + captureCommitPhaseError(fiber, fiber.return, unmountError); } break; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index dff64f9cae95d..054f363c2468b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -33,6 +33,7 @@ import { enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, enableDoubleInvokingEffects, + skipUnmountedBoundaries, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2292,7 +2293,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. @@ -2300,7 +2305,13 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { return; } - let fiber = sourceFiber.return; + 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 699b47596d0c8..08ab189548757 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -33,6 +33,7 @@ import { enableSchedulingProfiler, disableSchedulerTimeoutInWorkLoop, enableDoubleInvokingEffects, + skipUnmountedBoundaries, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -2292,7 +2293,12 @@ function captureCommitPhaseErrorOnRoot( } } -export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { +export function captureCommitPhaseError( + sourceFiber: Fiber, + nearestMountedAncestor: Fiber | null, + error: mixed, +) { + debugger; if (sourceFiber.tag === HostRoot) { // Error was thrown at the root. There is no parent, so the root // itself should capture it. @@ -2300,7 +2306,13 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { return; } - let fiber = sourceFiber.return; + 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 489928f27d208..693bf187d0f60 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2358,6 +2358,233 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([]); }); + describe('errors thrown in passive destroy function within unmounted trees', () => { + let BrokenUseEffectCleanup; + let ErrorBoundary; + let DerivedStateOnlyErrorBoundary; + let LogOnlyErrorBoundary; + + beforeEach(() => { + BrokenUseEffectCleanup = function() { + useEffect(() => { + Scheduler.unstable_yieldValue('BrokenUseEffectCleanup useEffect'); + return () => { + Scheduler.unstable_yieldValue( + 'BrokenUseEffectCleanup useEffect destroy', + ); + throw new Error('Expected error'); + }; + }, []); + + return 'inner child'; + }; + + ErrorBoundary = class extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `ErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + componentDidCatch(error, info) { + Scheduler.unstable_yieldValue(`ErrorBoundary componentDidCatch`); + } + render() { + if (this.state.error) { + Scheduler.unstable_yieldValue('ErrorBoundary render error'); + return ; + } + Scheduler.unstable_yieldValue('ErrorBoundary render success'); + return this.props.children || null; + } + }; + + DerivedStateOnlyErrorBoundary = class extends React.Component { + state = {error: null}; + static getDerivedStateFromError(error) { + Scheduler.unstable_yieldValue( + `DerivedStateOnlyErrorBoundary static getDerivedStateFromError`, + ); + return {error}; + } + render() { + if (this.state.error) { + Scheduler.unstable_yieldValue( + 'DerivedStateOnlyErrorBoundary render error', + ); + return ; + } + Scheduler.unstable_yieldValue( + 'DerivedStateOnlyErrorBoundary render success', + ); + return this.props.children || null; + } + }; + + LogOnlyErrorBoundary = class extends React.Component { + componentDidCatch(error, info) { + Scheduler.unstable_yieldValue( + `LogOnlyErrorBoundary componentDidCatch`, + ); + } + render() { + Scheduler.unstable_yieldValue(`LogOnlyErrorBoundary render`); + return this.props.children || null; + } + }; + }); + + // @gate skipUnmountedBoundaries + it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => { + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'BrokenUseEffectCleanup useEffect destroy', + 'LogOnlyErrorBoundary componentDidCatch', + ]); + }); + + // @gate skipUnmountedBoundaries + it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'BrokenUseEffectCleanup useEffect destroy', + 'LogOnlyErrorBoundary componentDidCatch', + ]); + }); + + // @gate skipUnmountedBoundaries + it('should call getDerivedStateFromError in the nearest still-mounted boundary', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ; + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect destroy', + 'ErrorBoundary static getDerivedStateFromError', + 'ErrorBoundary render error', + 'ErrorBoundary componentDidCatch', + ]); + + expect(ReactNoop.getChildren()).toEqual([ + span('ErrorBoundary fallback'), + ]); + }); + + // @gate skipUnmountedBoundaries + it('should rethrow error if there are no still-mounted boundaries', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + expect(() => { + act(() => { + ReactNoop.render(); + }); + }).toThrow('Expected error'); + + expect(Scheduler).toHaveYielded([ + 'BrokenUseEffectCleanup useEffect destroy', + ]); + + expect(ReactNoop.getChildren()).toEqual([]); + }); + }); + it('calls passive effect destroy functions for memoized components', () => { const Wrapper = ({children}) => children; function Child() { @@ -2594,6 +2821,85 @@ describe('ReactHooksWithNoopRenderer', () => { 'Mount normal [current: 1]', ]); }); + + // @gate skipUnmountedBoundaries + 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..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() { @@ -992,12 +993,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(); }); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index c3ffb65e96ff2..8611a9da06bc6 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -101,6 +101,12 @@ export const enableNewReconciler = false; export const disableNativeComponentFrames = 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 23c926c65a3fb..0fd02c2fd3f18 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -46,6 +46,7 @@ export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = 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 3ed2d7701c93f..c76048a8b2b44 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -45,6 +45,7 @@ export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = 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 c6ffacb27bab2..447d77effbc4e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -45,6 +45,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = 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 84ea8902f60c2..6a6d8f39c59b4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -45,6 +45,7 @@ export const enableComponentStackLocations = false; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = 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 eb1630fc49bd7..56e7dde89dfbd 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -45,6 +45,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = 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 87133385439f3..537c3fbf1c090 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -45,6 +45,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = false; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = 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 671f44be02952..24dd557047005 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -45,6 +45,7 @@ export const enableComponentStackLocations = true; export const enableLegacyFBSupport = !__EXPERIMENTAL__; export const enableFilterEmptyStringAttributesDOM = false; export const disableNativeComponentFrames = false; +export const skipUnmountedBoundaries = true; 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 cf0ca5ae54393..cf858b767cc3b 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 8b2bda551339e..85ce6eca57284 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, enableDoubleInvokingEffects, enableUseRefAccessWarning, disableNativeComponentFrames,