diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 99a95dde732b8..9d9d220f33d52 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -110,7 +110,8 @@ import { captureCommitPhaseError, resolveRetryThenable, markCommitTimeOfFallback, - enqueuePendingPassiveEffectDestroyFn, + enqueuePendingPassiveHookEffectMount, + enqueuePendingPassiveHookEffectUnmount, } from './ReactFiberWorkLoop'; import { NoEffect as NoHookEffect, @@ -396,6 +397,28 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { } } +function schedulePassiveEffects(finishedWork: Fiber) { + if (deferPassiveEffectCleanupDuringUnmount) { + const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any); + let lastEffect = updateQueue !== null ? updateQueue.lastEffect : null; + if (lastEffect !== null) { + const firstEffect = lastEffect.next; + let effect = firstEffect; + do { + const {next, tag} = effect; + if ( + (tag & HookPassive) !== NoHookEffect && + (tag & HookHasEffect) !== NoHookEffect + ) { + enqueuePendingPassiveHookEffectUnmount(finishedWork, effect); + enqueuePendingPassiveHookEffectMount(finishedWork, effect); + } + effect = next; + } while (effect !== firstEffect); + } + } +} + export function commitPassiveHookEffects(finishedWork: Fiber): void { if ((finishedWork.effectTag & Passive) !== NoEffect) { switch (finishedWork.tag) { @@ -432,6 +455,10 @@ function commitLifeCycles( // e.g. a destroy function in one component should never override a ref set // by a create function in another component during the same commit. commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); + + if (deferPassiveEffectCleanupDuringUnmount) { + schedulePassiveEffects(finishedWork); + } return; } case ClassComponent: { @@ -774,7 +801,7 @@ function commitUnmount( const {destroy, tag} = effect; if (destroy !== undefined) { if ((tag & HookPassive) !== NoHookEffect) { - enqueuePendingPassiveEffectDestroyFn(destroy); + enqueuePendingPassiveHookEffectUnmount(current, effect); } else { safelyCallDestroy(current, destroy); } diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index fdb0875f8bb5c..851e22f8fe835 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -144,7 +144,7 @@ export type Hook = {| next: Hook | null, |}; -type Effect = {| +export type Effect = {| tag: HookEffectTag, create: () => (() => void) | void, destroy: (() => void) | void, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index efd4a788c0b81..f647ccb88b2ce 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './SchedulerWithReactIntegration'; import type {Interaction} from 'scheduler/src/Tracing'; import type {SuspenseConfig} from './ReactFiberSuspenseConfig'; import type {SuspenseState} from './ReactFiberSuspenseComponent'; +import type {Effect as HookEffect} from './ReactFiberHooks'; import { warnAboutDeprecatedLifecycles, @@ -257,7 +258,8 @@ let rootDoesHavePassiveEffects: boolean = false; let rootWithPendingPassiveEffects: FiberRoot | null = null; let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority; let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork; -let pendingUnmountedPassiveEffectDestroyFunctions: Array<() => void> = []; +let pendingPassiveHookEffectsMount: Array = []; +let pendingPassiveHookEffectsUnmount: Array = []; let rootsWithPendingDiscreteUpdates: Map< FiberRoot, @@ -2168,11 +2170,28 @@ export function flushPassiveEffects() { } } -export function enqueuePendingPassiveEffectDestroyFn( - destroy: () => void, +export function enqueuePendingPassiveHookEffectMount( + fiber: Fiber, + effect: HookEffect, +): void { + if (deferPassiveEffectCleanupDuringUnmount) { + pendingPassiveHookEffectsMount.push(effect, fiber); + if (!rootDoesHavePassiveEffects) { + rootDoesHavePassiveEffects = true; + scheduleCallback(NormalPriority, () => { + flushPassiveEffects(); + return null; + }); + } + } +} + +export function enqueuePendingPassiveHookEffectUnmount( + fiber: Fiber, + effect: HookEffect, ): void { if (deferPassiveEffectCleanupDuringUnmount) { - pendingUnmountedPassiveEffectDestroyFunctions.push(destroy); + pendingPassiveHookEffectsUnmount.push(effect, fiber); if (!rootDoesHavePassiveEffects) { rootDoesHavePassiveEffects = true; scheduleCallback(NormalPriority, () => { @@ -2183,6 +2202,11 @@ export function enqueuePendingPassiveEffectDestroyFn( } } +function invokePassiveEffectCreate(effect: HookEffect): void { + const create = effect.create; + effect.destroy = create(); +} + function flushPassiveEffectsImpl() { if (rootWithPendingPassiveEffects === null) { return false; @@ -2201,45 +2225,95 @@ function flushPassiveEffectsImpl() { const prevInteractions = pushInteractions(root); if (deferPassiveEffectCleanupDuringUnmount) { - // Flush any pending passive effect destroy functions that belong to - // components that were unmounted during the most recent commit. - for ( - let i = 0; - i < pendingUnmountedPassiveEffectDestroyFunctions.length; - i++ - ) { - const destroy = pendingUnmountedPassiveEffectDestroyFunctions[i]; - invokeGuardedCallback(null, destroy, null); + // 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. + let unmountEffects = pendingPassiveHookEffectsUnmount; + pendingPassiveHookEffectsUnmount = []; + for (let i = 0; i < unmountEffects.length; i += 2) { + const effect = ((unmountEffects[i]: any): HookEffect); + const fiber = ((unmountEffects[i + 1]: any): Fiber); + const destroy = effect.destroy; + effect.destroy = undefined; + if (typeof destroy === 'function') { + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback(null, destroy, null); + if (hasCaughtError()) { + invariant(fiber !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + destroy(); + } catch (error) { + invariant(fiber !== null, 'Should be working on an effect.'); + captureCommitPhaseError(fiber, error); + } + } + } } - 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); + // Second pass: Create new passive effects. + let mountEffects = pendingPassiveHookEffectsMount; + pendingPassiveHookEffectsMount = []; + for (let i = 0; i < mountEffects.length; i += 2) { + const effect = ((mountEffects[i]: any): HookEffect); + const fiber = ((mountEffects[i + 1]: any): Fiber); + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect); + if (hasCaughtError()) { + invariant(fiber !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + const create = effect.create; + effect.destroy = create(); + } catch (error) { + invariant(fiber !== null, 'Should be working on an effect.'); + captureCommitPhaseError(fiber, error); + } } - resetCurrentDebugFiberInDEV(); - } else { - try { - commitPassiveHookEffects(effect); - } catch (error) { - invariant(effect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(effect, error); + } + } 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..438a748fb75a4 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,18 +1739,30 @@ 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'), ); expect(Scheduler).toFlushAndYieldThrough(['Count: 1', 'Sync effect']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); expect(() => ReactNoop.flushPassiveEffects()).toThrow('Oops'); - expect(Scheduler).toHaveYielded(['Oops!']); + + // This tests enables a feature flag that flushes all passive destroys in a + // separate pass before flushing any passive creates. + // A result of this two-pass flush is that an error thrown from unmount does + // not block the subsequent create functions from being run. + expect(Scheduler).toHaveYielded([ + 'Oops!', + 'Unmount B [0]', + 'Mount A [1]', + 'Mount B [1]', + ]); }); - // 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 - expect(Scheduler).toHaveYielded(['Unmount B [0]']); + + // gets unmounted because an error is thrown above. + // The remaining destroy functions are run later on unmount, since they're passive. + // In this case, one of them throws again (because of how the test is written). + expect(Scheduler).toHaveYielded(['Oops!', 'Unmount B [1]']); expect(ReactNoop.getChildren()).toEqual([]); });