From 674bfc46c6b34c5a9ba63c9f4768b9c1076d5cb7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 4 Feb 2020 09:02:37 -0800 Subject: [PATCH] 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]', ]);