-
Notifications
You must be signed in to change notification settings - Fork 46.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Flush all passive destroy fns before calling create fns #17947
Conversation
effect.nextEffect = null; | ||
effect = nextNextEffect; | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the old code path. If we flip the deferPassiveEffectCleanupDuringUnmount
killswitch off, it will restore the previous behavior.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 9ca1b2d:
|
Details of bundled changes.Comparing: 529e58a...9ca1b2d react-art
react-native-renderer
react-dom
react-reconciler
react-test-renderer
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
Details of bundled changes.Comparing: 529e58a...9ca1b2d react-native-renderer
react-test-renderer
react-reconciler
react-dom
react-art
ReactDOM: size: 0.0%, gzip: -0.1% Size changes (experimental) |
fa54752
to
0fb7bba
Compare
Rebased after merging PR #17925. |
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If multiple fibers errored, this check only works for the last one. So it doesn't seem worth it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. I would prefer just not carrying this behavior forward anyway. I'll remove it.
I was thinking the way you could do this is by maintaining two arrays: one for destroy effects, another for create/init effects. During the sync commit phase, traverse over the hook effect list and push to the appropriate array. Then during The advantage of this approach is it's less total work because in the passive phase, you no longer have to visit the entire effect list. The disadvantage is it sometimes shifts more work to the layout phase, because you can't skip over a fiber that only has passive effects scheduled on it. But the other advantage is that it saves on code size, which is why I'm leaning toward always pushing to an array. cc @sebmarkbage for input. |
That could bring things more inline with the way I'm deferring destroy functions on unmount... (That could just be handled by this mechanism as well.) I'm open to making this change, but I'm going to hold off to see what @sebmarkbage thinks since it would require a bit of code juggling (especially since we want to keep everything behind a feature flag for now). Hm...I think we'd need to track the effect's corresponding fiber too, for error tracking purposes. |
@acdlite I took a pass at this locally (diff below). It has an added benefit of calling more unmount functions in the case of one throwing, but in order to maintain the expected error behavior- I think it would also be a bit more code than maybe you expected? (We'd have to track the hook effect + Fiber, unless I'm overlooking something.) (Happy to push this commit though if you think the approach is better or if it would be easier to review than a diff.) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
index c6fc36be3..5eeb7d290 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 fdb0875f8..851e22f8f 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 ae4db267b..b015e63de 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -14,7 +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 {Hook} from './ReactFiberHooks';
+import type {Effect as HookEffect, Hook} from './ReactFiberHooks';
import {
warnAboutDeprecatedLifecycles,
@@ -133,8 +133,6 @@ import {
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
commitLifeCycles as commitLayoutEffectOnFiber,
commitPassiveHookEffects,
- commitPassiveHookUnmountEffects,
- commitPassiveHookMountEffects,
commitPlacement,
commitWork,
commitDeletion,
@@ -260,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<HookEffect | Fiber> = [];
+let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
let rootsWithPendingDiscreteUpdates: Map<
FiberRoot,
@@ -2180,11 +2179,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, () => {
@@ -2195,6 +2211,11 @@ export function enqueuePendingPassiveEffectDestroyFn(
}
}
+function invokePassiveEffectCreate(effect: HookEffect): void {
+ const create = effect.create;
+ effect.destroy = create();
+}
+
function flushPassiveEffectsImpl() {
if (rootWithPendingPassiveEffects === null) {
return false;
@@ -2213,18 +2234,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.
@@ -2233,70 +2242,60 @@ 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;
+ if (typeof destroy === 'function') {
+ if (__DEV__) {
+ setCurrentDebugFiberInDEV(fiber);
+ invokeGuardedCallback(null, destroy, null);
+ effect.destroy = undefined;
+ 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);
+ } finally {
+ effect.destroy = undefined;
+ }
}
}
- 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 cc3eee28d..438a748fb 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]',
]); |
I'm confused about the review process here. The previous PR includes everything from this PR and is merged. #17925 |
Ugh, the rebase history got messed up somehow. This PR builds on top of the previous PR (not the other way around). I'll clean up the history now, but the file change/delta is accurate here. |
44e2b96
to
41e0752
Compare
Okay @sebmarkbage @acdlite I manually cleaned up the branch history. (Sorry for the rebase confusion.) There are two commits here now: |
effect: HookEffect, | ||
): void { | ||
if (deferPassiveEffectCleanupDuringUnmount) { | ||
pendingPassiveHookEffectsMount.push(effect, fiber); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retaining both the effect and Fiber is kind of gross, particularly in the same array. I think we'd need to track them both somewhere though because of how we track and report errors.
// not block the subsequent create functions from being run. | ||
expect(Scheduler).toHaveYielded([ | ||
'Oops!', | ||
'Unmount B [0]', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main positive change wrt 41e0752.
Ping~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up, we might want to refactor several parts of the commit phase but those are unobservable differences that we can do later. So I'm not too concerned about the implementation details here as long as we can test the semantics.
Thanks for the heads up, Seb. Andrew and I chatted a little offline about this yesterday too, so I think I'm at least aware of some of the planned changes. Happy to help with the refactoring when it's time. |
41e0752
to
674bfc4
Compare
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).
This change offers a small advantage over the way we did things previous: it continues invoking destroy functions even after a previous one errored.
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).It also carries forward a behavior of not running mount functions for a Fiber that's had an unmount function error. Open to discussion on whether this behavior is something we want to carry forward.The
deferPassiveEffectCleanupDuringUnmount
flag still remains off in master. We'll want to flip it on for Facebook during an upcoming sync.Resolves issue #17945. Builds on top of PR #17925.