diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index ec23dcd995b88..d22f2d296a733 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -414,6 +414,38 @@ export function commitPassiveHookEffects(finishedWork: Fiber): void { } } +export function commitPassiveHookUnmountEffects(finishedWork: Fiber): void { + if ((finishedWork.effectTag & Passive) !== NoEffect) { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Chunk: { + commitHookEffectList(HookPassive, NoHookEffect, finishedWork); + break; + } + default: + break; + } + } +} + +export function commitPassiveHookMountEffects(finishedWork: Fiber): void { + if ((finishedWork.effectTag & Passive) !== NoEffect) { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: + case Chunk: { + commitHookEffectList(NoHookEffect, HookPassive, finishedWork); + break; + } + default: + break; + } + } +} + function commitLifeCycles( finishedRoot: FiberRoot, current: Fiber | null, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 4c067de698ec1..dd25a2cee256c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -133,6 +133,8 @@ import { commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber, commitLifeCycles as commitLayoutEffectOnFiber, commitPassiveHookEffects, + commitPassiveHookUnmountEffects, + commitPassiveHookMountEffects, commitPlacement, commitWork, commitDeletion, @@ -2222,34 +2224,116 @@ function flushPassiveEffectsImpl() { invokeGuardedCallback(null, destroy, null); } pendingUnmountedPassiveEffectDestroyFunctions.length = 0; - } - // Note: This currently assumes there are no passive effects on the root - // fiber, because the root is not part of its own effect list. This could - // change in the future. - let effect = root.current.firstEffect; - while (effect !== null) { - if (__DEV__) { - setCurrentDebugFiberInDEV(effect); - invokeGuardedCallback(null, commitPassiveHookEffects, null, effect); - if (hasCaughtError()) { - invariant(effect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(effect, error); + // It's important that ALL pending passive effect destroy functions are called + // before ANY passive effect create functions are called. + // Otherwise effects in sibling components might interfere with each other. + // e.g. a destroy function in one component may unintentionally override a ref + // value set by a create function in another component. + // Layout effects have the same constraint. + + // First pass: Destroy stale passive effects. + // + // Note: This currently assumes there are no passive effects on the root fiber + // because the root is not part of its own effect list. + // This could change in the future. + let effect = root.current.firstEffect; + let effectWithErrorDuringUnmount = null; + while (effect !== null) { + if (__DEV__) { + setCurrentDebugFiberInDEV(effect); + invokeGuardedCallback( + null, + commitPassiveHookUnmountEffects, + null, + effect, + ); + if (hasCaughtError()) { + effectWithErrorDuringUnmount = effect; + invariant(effect !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(effect, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitPassiveHookUnmountEffects(effect); + } catch (error) { + effectWithErrorDuringUnmount = effect; + invariant(effect !== null, 'Should be working on an effect.'); + captureCommitPhaseError(effect, error); + } } - resetCurrentDebugFiberInDEV(); - } else { - try { - commitPassiveHookEffects(effect); - } catch (error) { - invariant(effect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(effect, error); + effect = effect.nextEffect; + } + + // Second pass: Create new passive effects. + // + // Note: This currently assumes there are no passive effects on the root fiber + // because the root is not part of its own effect list. + // This could change in the future. + effect = root.current.firstEffect; + while (effect !== null) { + // Don't run create effects for a Fiber that errored during destroy. + // This check is in place to match previous behavior. + // TODO: Rethink whether we want to carry this behavior forward. + if (effectWithErrorDuringUnmount !== effect) { + if (__DEV__) { + setCurrentDebugFiberInDEV(effect); + invokeGuardedCallback( + null, + commitPassiveHookMountEffects, + null, + effect, + ); + if (hasCaughtError()) { + invariant(effect !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(effect, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitPassiveHookMountEffects(effect); + } catch (error) { + invariant(effect !== null, 'Should be working on an effect.'); + captureCommitPhaseError(effect, error); + } + } + } + const nextNextEffect = effect.nextEffect; + // Remove nextEffect pointer to assist GC + effect.nextEffect = null; + effect = nextNextEffect; + } + } else { + // Note: This currently assumes there are no passive effects on the root fiber + // because the root is not part of its own effect list. + // This could change in the future. + let effect = root.current.firstEffect; + while (effect !== null) { + if (__DEV__) { + setCurrentDebugFiberInDEV(effect); + invokeGuardedCallback(null, commitPassiveHookEffects, null, effect); + if (hasCaughtError()) { + invariant(effect !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(effect, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitPassiveHookEffects(effect); + } catch (error) { + invariant(effect !== null, 'Should be working on an effect.'); + captureCommitPhaseError(effect, error); + } } + const nextNextEffect = effect.nextEffect; + // Remove nextEffect pointer to assist GC + effect.nextEffect = null; + effect = nextNextEffect; } - const nextNextEffect = effect.nextEffect; - // Remove nextEffect pointer to assist GC - effect.nextEffect = null; - effect = nextNextEffect; } if (enableSchedulerTracing) { diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index c98dbb1171273..6f933c5880c3e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1558,6 +1558,67 @@ describe('ReactHooksWithNoopRenderer', () => { ]); }); + it('unmounts all previous effects between siblings before creating any new ones', () => { + function Counter({count, label}) { + useEffect(() => { + Scheduler.unstable_yieldValue(`Mount ${label} [${count}]`); + return () => { + Scheduler.unstable_yieldValue(`Unmount ${label} [${count}]`); + }; + }); + return ; + } + act(() => { + ReactNoop.render( + + + + , + () => Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough(['A 0', 'B 0', 'Sync effect']); + expect(ReactNoop.getChildren()).toEqual([span('A 0'), span('B 0')]); + }); + + expect(Scheduler).toHaveYielded(['Mount A [0]', 'Mount B [0]']); + + act(() => { + ReactNoop.render( + + + + , + () => Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough(['A 1', 'B 1', 'Sync effect']); + expect(ReactNoop.getChildren()).toEqual([span('A 1'), span('B 1')]); + }); + expect(Scheduler).toHaveYielded([ + 'Unmount A [0]', + 'Unmount B [0]', + 'Mount A [1]', + 'Mount B [1]', + ]); + + act(() => { + ReactNoop.render( + + + + , + () => Scheduler.unstable_yieldValue('Sync effect'), + ); + expect(Scheduler).toFlushAndYieldThrough(['B 2', 'C 0', 'Sync effect']); + expect(ReactNoop.getChildren()).toEqual([span('B 2'), span('C 0')]); + }); + expect(Scheduler).toHaveYielded([ + 'Unmount A [1]', + 'Unmount B [1]', + 'Mount B [2]', + 'Mount C [0]', + ]); + }); + it('handles errors on mount', () => { function Counter(props) { useEffect(() => { @@ -1656,8 +1717,6 @@ describe('ReactHooksWithNoopRenderer', () => { return () => { Scheduler.unstable_yieldValue('Oops!'); throw new Error('Oops!'); - // eslint-disable-next-line no-unreachable - Scheduler.unstable_yieldValue(`Unmount A [${props.count}]`); }; }); useEffect(() => { @@ -1668,6 +1727,7 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } + act(() => { ReactNoop.render(, () => Scheduler.unstable_yieldValue('Sync effect'), @@ -1679,7 +1739,7 @@ describe('ReactHooksWithNoopRenderer', () => { }); act(() => { - // This update will trigger an error + // This update will trigger an error during passive effect unmount ReactNoop.render(, () => Scheduler.unstable_yieldValue('Sync effect'), ); @@ -1688,8 +1748,9 @@ describe('ReactHooksWithNoopRenderer', () => { expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops'); expect(Scheduler).toHaveYielded(['Oops!']); }); - // B unmounts even though an error was thrown in the previous effect - // B's destroy function runs later on unmount though, since it's passive + + // gets unmounted because an error is thrown above. + // The remaining destroy function gets runs later on unmount, since it's passive expect(Scheduler).toHaveYielded(['Unmount B [0]']); expect(ReactNoop.getChildren()).toEqual([]); });