From ad04363c9c88a597744cbba40fa53917cc1d33d7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 4 Feb 2020 09:00:38 -0800 Subject: [PATCH 1/2] Flush all passive destroy fns before calling create fns Previously we only flushed destroy functions for a single fiber. The reason this is important is that interleaving destroy/create effects between sibling components might cause components to 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). This PR builds on top of the recently added deferPassiveEffectCleanupDuringUnmount kill switch to separate passive effects flushing into two separate phases (similar to layout effects). --- .../src/ReactFiberCommitWork.js | 32 +++++ .../src/ReactFiberWorkLoop.js | 124 ++++++++++++++---- ...eactHooksWithNoopRenderer-test.internal.js | 85 +++++++++++- 3 files changed, 210 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 99a95dde732b8..0206e7471d317 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -416,6 +416,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: { + commitHookEffectListUnmount(HookPassive | HookHasEffect, 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: { + commitHookEffectListMount(HookPassive | HookHasEffect, 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 efd4a788c0b81..16e542d44926e 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -132,6 +132,8 @@ import { commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber, commitLifeCycles as commitLayoutEffectOnFiber, commitPassiveHookEffects, + commitPassiveHookUnmountEffects, + commitPassiveHookMountEffects, commitPlacement, commitWork, commitDeletion, @@ -2212,34 +2214,108 @@ 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; + while (effect !== null) { + if (__DEV__) { + setCurrentDebugFiberInDEV(effect); + invokeGuardedCallback( + null, + commitPassiveHookUnmountEffects, + null, + effect, + ); + if (hasCaughtError()) { + invariant(effect !== null, 'Should be working on an effect.'); + const error = clearCaughtError(); + captureCommitPhaseError(effect, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitPassiveHookUnmountEffects(effect); + } catch (error) { + 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) { + 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..cc3eee28d8ef6 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,29 @@ 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!', + '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([]); }); From 9ca1b2d13a289c007c0a3bf9f6bb31b13ea39692 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 4 Feb 2020 09:02:37 -0800 Subject: [PATCH 2/2] Change passive effect flushing to use arrays instead of lists This change offers a small advantage over the way we did things previous: it continues invoking destroy functions even after a previous one errored. --- .../src/ReactFiberCommitWork.js | 63 ++++----- .../react-reconciler/src/ReactFiberHooks.js | 2 +- .../src/ReactFiberWorkLoop.js | 132 +++++++++--------- ...eactHooksWithNoopRenderer-test.internal.js | 1 + 4 files changed, 96 insertions(+), 102 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 0206e7471d317..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,49 +397,39 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) { } } -export function commitPassiveHookEffects(finishedWork: Fiber): void { - if ((finishedWork.effectTag & Passive) !== NoEffect) { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: - case Chunk: { - // TODO (#17945) We should call all passive destroy functions (for all fibers) - // before calling any create functions. The current approach only serializes - // these for a single fiber. - commitHookEffectListUnmount(HookPassive | HookHasEffect, finishedWork); - commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork); - break; - } - default: - break; +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 commitPassiveHookUnmountEffects(finishedWork: Fiber): void { +export function commitPassiveHookEffects(finishedWork: Fiber): void { if ((finishedWork.effectTag & Passive) !== NoEffect) { switch (finishedWork.tag) { case FunctionComponent: case ForwardRef: case SimpleMemoComponent: case Chunk: { + // TODO (#17945) We should call all passive destroy functions (for all fibers) + // before calling any create functions. The current approach only serializes + // these for a single fiber. commitHookEffectListUnmount(HookPassive | HookHasEffect, 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: { commitHookEffectListMount(HookPassive | HookHasEffect, finishedWork); break; } @@ -464,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: { @@ -806,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 16e542d44926e..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, @@ -132,8 +133,6 @@ import { commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber, commitLifeCycles as commitLayoutEffectOnFiber, commitPassiveHookEffects, - commitPassiveHookUnmountEffects, - commitPassiveHookMountEffects, commitPlacement, commitWork, commitDeletion, @@ -259,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, @@ -2170,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, () => { @@ -2185,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; @@ -2203,18 +2225,6 @@ 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); - } - pendingUnmountedPassiveEffectDestroyFunctions.length = 0; - // 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. @@ -2223,70 +2233,58 @@ function flushPassiveEffectsImpl() { // 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; - while (effect !== null) { - if (__DEV__) { - setCurrentDebugFiberInDEV(effect); - invokeGuardedCallback( - null, - commitPassiveHookUnmountEffects, - null, - effect, - ); - if (hasCaughtError()) { - invariant(effect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(effect, error); - } - resetCurrentDebugFiberInDEV(); - } else { - try { - commitPassiveHookUnmountEffects(effect); - } catch (error) { - invariant(effect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(effect, error); + 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); + } } } - 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) { + 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(effect); - invokeGuardedCallback( - null, - commitPassiveHookMountEffects, - null, - effect, - ); + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback(null, invokePassiveEffectCreate, null, effect); if (hasCaughtError()) { - invariant(effect !== null, 'Should be working on an effect.'); + invariant(fiber !== null, 'Should be working on an effect.'); const error = clearCaughtError(); - captureCommitPhaseError(effect, error); + captureCommitPhaseError(fiber, error); } resetCurrentDebugFiberInDEV(); } else { try { - commitPassiveHookMountEffects(effect); + const create = effect.create; + effect.destroy = create(); } catch (error) { - invariant(effect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(effect, error); + invariant(fiber !== null, 'Should be working on an effect.'); + captureCommitPhaseError(fiber, 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 diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index cc3eee28d8ef6..438a748fb75a4 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1753,6 +1753,7 @@ describe('ReactHooksWithNoopRenderer', () => { // not block the subsequent create functions from being run. expect(Scheduler).toHaveYielded([ 'Oops!', + 'Unmount B [0]', 'Mount A [1]', 'Mount B [1]', ]);