diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
index f4282ac4fa5b..c6fc36be343a 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 4c067de698ec..ae4db267b638 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,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 c98dbb117127..cc3eee28d8ef 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([]);
});