diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js index 794f48ce676fb..8858997cb97de 100644 --- a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js @@ -1667,16 +1667,28 @@ describe('DOMPluginEventSystem', () => { function Test() { React.useEffect(() => { - setClick1(buttonRef.current, targetListener1); - setClick2(buttonRef.current, targetListener2); - setClick3(buttonRef.current, targetListener3); - setClick4(buttonRef.current, targetListener4); + const clearClick1 = setClick1( + buttonRef.current, + targetListener1, + ); + const clearClick2 = setClick2( + buttonRef.current, + targetListener2, + ); + const clearClick3 = setClick3( + buttonRef.current, + targetListener3, + ); + const clearClick4 = setClick4( + buttonRef.current, + targetListener4, + ); return () => { - setClick1(); - setClick2(); - setClick3(); - setClick4(); + clearClick1(); + clearClick2(); + clearClick3(); + clearClick4(); }; }); @@ -1701,16 +1713,28 @@ describe('DOMPluginEventSystem', () => { function Test2() { React.useEffect(() => { - setClick1(buttonRef.current, targetListener1); - setClick2(buttonRef.current, targetListener2); - setClick3(buttonRef.current, targetListener3); - setClick4(buttonRef.current, targetListener4); + const clearClick1 = setClick1( + buttonRef.current, + targetListener1, + ); + const clearClick2 = setClick2( + buttonRef.current, + targetListener2, + ); + const clearClick3 = setClick3( + buttonRef.current, + targetListener3, + ); + const clearClick4 = setClick4( + buttonRef.current, + targetListener4, + ); return () => { - setClick1(); - setClick2(); - setClick3(); - setClick4(); + clearClick1(); + clearClick2(); + clearClick3(); + clearClick4(); }; }); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 79002ff6ae629..f086e47cf93b1 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -173,13 +173,13 @@ function safelyCallComponentWillUnmount(current, instance) { ); if (hasCaughtError()) { const unmountError = clearCaughtError(); - captureCommitPhaseError(current, unmountError); + captureCommitPhaseError(current, current.return, unmountError); } } else { try { callComponentWillUnmountWithTimer(current, instance); } catch (unmountError) { - captureCommitPhaseError(current, unmountError); + captureCommitPhaseError(current, current.return, unmountError); } } } @@ -192,13 +192,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 { @@ -207,18 +207,22 @@ function safelyDetachRef(current: Fiber) { } } -export function safelyCallDestroy(current: Fiber, destroy: () => void) { +export 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); } } } @@ -337,10 +341,10 @@ function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) { // TODO: Remove this duplication. function commitHookEffectListUnmount2( - // 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, fiber: Fiber, + nearestMountedAncestor: Fiber | null, ): void { const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any); const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; @@ -359,10 +363,10 @@ function commitHookEffectListUnmount2( fiber.mode & ProfileMode ) { startPassiveEffectTimer(); - safelyCallDestroy(fiber, destroy); + safelyCallDestroy(fiber, nearestMountedAncestor, destroy); recordPassiveEffectDuration(fiber); } else { - safelyCallDestroy(fiber, destroy); + safelyCallDestroy(fiber, nearestMountedAncestor, destroy); } } } @@ -465,7 +469,7 @@ function commitHookEffectListMount2(fiber: Fiber): void { if (hasCaughtError()) { invariant(fiber !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } else { try { @@ -488,7 +492,7 @@ function commitHookEffectListMount2(fiber: Fiber): void { // The warning refers to useEffect but only applies to useLayoutEffect. } catch (error) { invariant(fiber !== null, 'Should be working on an effect.'); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } } @@ -997,10 +1001,10 @@ function commitUnmount( current.mode & ProfileMode ) { startLayoutEffectTimer(); - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, current.return, destroy); recordLayoutEffectDuration(current); } else { - safelyCallDestroy(current, destroy); + safelyCallDestroy(current, current.return, destroy); } } } @@ -1842,18 +1846,29 @@ function commitPassiveWork(finishedWork: Fiber): void { case ForwardRef: case SimpleMemoComponent: case Block: { - commitHookEffectListUnmount2(HookPassive | HookHasEffect, finishedWork); + commitHookEffectListUnmount2( + HookPassive | HookHasEffect, + finishedWork, + finishedWork.return, + ); } } } -function commitPassiveUnmount(current: Fiber): void { +function commitPassiveUnmount( + current: Fiber, + nearestMountedAncestor: Fiber | null, +): void { switch (current.tag) { case FunctionComponent: case ForwardRef: case SimpleMemoComponent: case Block: - commitHookEffectListUnmount2(HookPassive, current); + commitHookEffectListUnmount2( + HookPassive, + current, + nearestMountedAncestor, + ); } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 63f20c123a396..87da91d58c5f4 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2393,14 +2393,14 @@ function commitBeforeMutationEffects(firstChild: Fiber) { invokeGuardedCallback(null, commitBeforeMutationEffectsImpl, null, fiber); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitBeforeMutationEffectsImpl(fiber); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } fiber = fiber.sibling; @@ -2490,14 +2490,14 @@ function commitMutationEffects( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitMutationEffectsImpl(fiber, root, renderPriorityLevel); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } fiber = fiber.sibling; @@ -2593,13 +2593,13 @@ function commitMutationEffectsDeletions( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(childToDelete, error); + captureCommitPhaseError(childToDelete, childToDelete.return, error); } } else { try { commitDeletion(root, childToDelete, renderPriorityLevel); } catch (error) { - captureCommitPhaseError(childToDelete, error); + captureCommitPhaseError(childToDelete, childToDelete.return, error); } } } @@ -2641,14 +2641,14 @@ function commitLayoutEffects( ); if (hasCaughtError()) { const error = clearCaughtError(); - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } resetCurrentDebugFiberInDEV(); } else { try { commitLayoutEffectsImpl(fiber, root, committedLanes); } catch (error) { - captureCommitPhaseError(fiber, error); + captureCommitPhaseError(fiber, fiber.return, error); } } fiber = fiber.sibling; @@ -2748,7 +2748,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const fiberToDelete = deletions[i]; - flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete); + flushPassiveUnmountEffectsInsideOfDeletedTree(fiberToDelete, fiber); // Now that passive effects have been processed, it's safe to detach lingering pointers. detachFiberAfterEffects(fiberToDelete); @@ -2780,6 +2780,7 @@ function flushPassiveUnmountEffects(firstChild: Fiber): void { function flushPassiveUnmountEffectsInsideOfDeletedTree( fiberToDelete: Fiber, + nearestMountedAncestor: Fiber, ): void { if ((fiberToDelete.subtreeTag & PassiveStaticSubtreeTag) !== NoSubtreeTag) { // If any children have passive effects then traverse the subtree. @@ -2788,14 +2789,17 @@ function flushPassiveUnmountEffectsInsideOfDeletedTree( // since that would not cover passive effects in siblings. let child = fiberToDelete.child; while (child !== null) { - flushPassiveUnmountEffectsInsideOfDeletedTree(child); + flushPassiveUnmountEffectsInsideOfDeletedTree( + child, + nearestMountedAncestor, + ); child = child.sibling; } } if ((fiberToDelete.effectTag & PassiveStatic) !== NoEffect) { setCurrentDebugFiberInDEV(fiberToDelete); - commitPassiveUnmount(fiberToDelete); + commitPassiveUnmount(fiberToDelete, nearestMountedAncestor); resetCurrentDebugFiberInDEV(); } } @@ -2922,7 +2926,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. @@ -2930,7 +2938,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/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index eab3949bde820..84188fea37427 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2850,6 +2850,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..7ff61126f648a 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,10 +2352,32 @@ 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; + 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; } }; @@ -2366,12 +2389,13 @@ describe('ReactHooksWithNoopRenderer', () => { } render() { Scheduler.unstable_yieldValue(`LogOnlyErrorBoundary render`); - return this.props.children; + return this.props.children || null; } }; }); - 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,268 @@ 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', () => { + 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([]); + }); }); });