diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 6fda9c97cd1f0..9047a83fc95f0 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -112,6 +112,7 @@ import { } from './ReactFiberHostConfig'; import { captureCommitPhaseError, + captureCommitPhaseErrorForUnmountedFiber, resolveRetryWakeable, markCommitTimeOfFallback, enqueuePendingPassiveProfilerEffect, @@ -216,6 +217,34 @@ export function safelyCallDestroy(current: Fiber, destroy: () => void) { } } +export function safelyCallDestroyForUnmountedFiber( + current: Fiber, + nearestMountedAncestor: Fiber, + destroy: () => void, +) { + if (__DEV__) { + invokeGuardedCallback(null, destroy, null); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseErrorForUnmountedFiber( + current, + nearestMountedAncestor, + error, + ); + } + } else { + try { + destroy(); + } catch (error) { + captureCommitPhaseErrorForUnmountedFiber( + current, + nearestMountedAncestor, + error, + ); + } + } +} + function commitBeforeMutationLifeCycles( current: Fiber | null, finishedWork: Fiber, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 63ffe2d356aae..b8d3eb69f175d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -212,6 +212,7 @@ import { commitResetTextContent, isSuspenseBoundaryBeingHidden, safelyCallDestroy, + safelyCallDestroyForUnmountedFiber, } from './ReactFiberCommitWork.new'; import {enqueueUpdate} from './ReactUpdateQueue.new'; import {resetContextDependencies} from './ReactFiberNewContext.new'; @@ -2790,7 +2791,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const fiberToDelete = deletions[i]; - flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete); + flushPassiveUnmountEffectsForUnmountedFiber(fiberToDelete, fiber); // Now that passive effects have been processed, it's safe to detach lingering pointers. detachFiberAfterEffects(fiberToDelete); @@ -2825,8 +2826,9 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { } } -function flushPassiveUnmountEffectsInsideOfDeletedTree( +function flushPassiveUnmountEffectsForUnmountedFiber( fiberToDelete: Fiber, + nearestMountedAncestor: Fiber, ): void { if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) { // If any children have passive effects then traverse the subtree. @@ -2835,7 +2837,10 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( // since that would not cover passive effects in siblings. let child = fiberToDelete.child; while (child !== null) { - flushPassiveUnmountEffectsInsideOfDeletedTree(child); + flushPassiveUnmountEffectsForUnmountedFiber( + child, + nearestMountedAncestor, + ); child = child.sibling; } } @@ -2846,16 +2851,64 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( case ForwardRef: case SimpleMemoComponent: case Block: { - flushPassiveUnmountEffectsImpl(fiberToDelete, HookPassive); + flushPassiveUnmountEffectsForUnmountedFiberImpl( + fiberToDelete, + nearestMountedAncestor, + ); } } } } +function flushPassiveUnmountEffectsForUnmountedFiberImpl( + fiber: Fiber, + nearestMountedAncestor: Fiber, +): void { + const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); + const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + setCurrentDebugFiberInDEV(fiber); + + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + if ((tag & HookPassive) === HookPassive) { + const destroy = effect.destroy; + if (destroy !== undefined) { + effect.destroy = undefined; + + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + fiber.mode & ProfileMode + ) { + startPassiveEffectTimer(); + safelyCallDestroyForUnmountedFiber( + fiber, + nearestMountedAncestor, + destroy, + ); + recordPassiveEffectDuration(fiber); + } else { + safelyCallDestroyForUnmountedFiber( + fiber, + nearestMountedAncestor, + destroy, + ); + } + } + } + effect = next; + } while (effect !== firstEffect); + + resetCurrentDebugFiberInDEV(); + } +} + function flushPassiveUnmountEffectsImpl( fiber: Fiber, - // Tags to check for when deciding whether to unmount. e.g. to skip over - // layout effects + // Tags to check for when deciding whether to unmount. e.g. to skip over layout effects hookEffectTag: HookEffectTag, ): void { const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); @@ -3055,6 +3108,45 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { } } +export function captureCommitPhaseErrorForUnmountedFiber( + sourceFiber: Fiber, + nearestMountedAncestor: Fiber, + error: mixed, +) { + let fiber = nearestMountedAncestor.return; + while (fiber !== null) { + if (fiber.tag === HostRoot) { + captureCommitPhaseErrorOnRoot(fiber, sourceFiber, error); + return; + } else if (fiber.tag === ClassComponent) { + const ctor = fiber.type; + const instance = fiber.stateNode; + if ( + typeof ctor.getDerivedStateFromError === 'function' || + (typeof instance.componentDidCatch === 'function' && + !isAlreadyFailedLegacyErrorBoundary(instance)) + ) { + const errorInfo = createCapturedValue(error, sourceFiber); + const update = createClassErrorUpdate( + fiber, + errorInfo, + (SyncLane: Lane), + ); + enqueueUpdate(fiber, update); + const eventTime = requestEventTime(); + const root = markUpdateLaneFromFiberToRoot(fiber, (SyncLane: Lane)); + if (root !== null) { + markRootUpdated(root, SyncLane, eventTime); + ensureRootIsScheduled(root, eventTime); + schedulePendingInteractions(root, SyncLane); + } + return; + } + } + fiber = fiber.return; + } +} + export function pingSuspendedRoot( root: FiberRoot, wakeable: Wakeable, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 36670a257f0ec..0c38f42bc18c4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2806,6 +2806,24 @@ export function captureCommitPhaseError(sourceFiber: Fiber, error: mixed) { markRootUpdated(root, SyncLane, eventTime); ensureRootIsScheduled(root, eventTime); schedulePendingInteractions(root, SyncLane); + } else { + // This component has already been unmounted. + // We can't schedule any follow up work for the root because the fiber is already unmounted, + // but we can still call the log-only boundary so the error isn't swallowed. + // + // TODO This is only a temporary bandaid for the old reconciler fork. + // We can delete this special case once the new fork is merged. + if ( + typeof instance.componentDidCatch === 'function' && + !isAlreadyFailedLegacyErrorBoundary(instance) + ) { + try { + instance.componentDidCatch(error, errorInfo); + } catch (errorToIgnore) { + // TODO Ignore this error? Rethrow it? + // This is kind of an edge case. + } + } } return; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index eaa222456fc66..258995b77628f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2320,6 +2320,7 @@ describe('ReactHooksWithNoopRenderer', () => { describe('errors thrown in passive destroy function within unmounted trees', () => { let BrokenUseEffectCleanup; let ErrorBoundary; + let DerivedStateOnlyErrorBoundary; let LogOnlyErrorBoundary; beforeEach(() => { @@ -2351,13 +2352,35 @@ describe('ReactHooksWithNoopRenderer', () => { render() { if (this.state.error) { Scheduler.unstable_yieldValue('ErrorBoundary render error'); - return 'ErrorBoundary fallback'; + return ; } Scheduler.unstable_yieldValue('ErrorBoundary render success'); return this.props.children; } }; + 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; + } + }; + LogOnlyErrorBoundary = class extends React.Component { componentDidCatch(error, info) { Scheduler.unstable_yieldValue( @@ -2371,7 +2394,8 @@ describe('ReactHooksWithNoopRenderer', () => { }; }); - it('should not error if the nearest unmounted boundary is log-only', () => { + // @gate old + it('should call componentDidCatch() for the nearest unmounted log-only boundary', () => { function Conditional({showChildren}) { if (showChildren) { return ( @@ -2411,10 +2435,280 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toHaveYielded([ 'BrokenUseEffectCleanup useEffect destroy', - // This should call componentDidCatch too, but we'll address that in a follow up. - // 'LogOnlyErrorBoundary componentDidCatch', + 'LogOnlyErrorBoundary componentDidCatch', + ]); + }); + + // @gate old + it('should call componentDidCatch() for the nearest unmounted logging-capable boundary', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'ErrorBoundary render success', + 'ErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + expect(Scheduler).toFlushAndYieldThrough([ + 'ErrorBoundary render success', + ]); + }); + + expect(Scheduler).toHaveYielded([ + 'BrokenUseEffectCleanup useEffect destroy', + 'ErrorBoundary componentDidCatch', ]); }); + + // @gate old + it('should not call getDerivedStateFromError for unmounted error boundaries', () => { + 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([ + 'BrokenUseEffectCleanup useEffect destroy', + 'ErrorBoundary componentDidCatch', + ]); + }); + + // @gate old + it('should not throw if there are no unmounted logging-capable boundaries to call', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ( + + + + ); + } else { + return null; + } + } + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'DerivedStateOnlyErrorBoundary render success', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded([ + 'BrokenUseEffectCleanup useEffect destroy', + ]); + }); + + // @gate new + it('should use the nearest still-mounted boundary if there are no unmounted boundaries', () => { + function Conditional({showChildren}) { + if (showChildren) { + return ; + } else { + return null; + } + } + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'BrokenUseEffectCleanup useEffect', + ]); + + act(() => { + ReactNoop.render( + + + , + ); + }); + + expect(Scheduler).toHaveYielded([ + 'LogOnlyErrorBoundary render', + 'BrokenUseEffectCleanup useEffect destroy', + 'LogOnlyErrorBoundary componentDidCatch', + ]); + }); + + // @gate new + 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 new + 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 new + 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([]); + }); }); });