Skip to content

Commit

Permalink
Fix incorrect unmounted state update warning (#18617)
Browse files Browse the repository at this point in the history 
* Fix incorrect unmounted state update warning

We detach fibers (which nulls the  field) when we commit a deletion, so any state updates scheduled between that point and when we eventually flush passive effect destroys won't have a way to check if there is a pending passive unmount effect scheduled for its alternate unless we also explicitly track this for both the current and the alternate.

This commit adds a new DEV-only effect type, `PendingPassiveUnmountDev`, to handle this case.
  • Loading branch information
Brian Vaughn committed Apr 18, 2020
1 parent 52a0d6b commit 263bc5d
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 20 deletions.
23 changes: 22 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ import {
Snapshot,
Callback,
Passive,
PassiveUnmountPendingDev,
Incomplete,
HostEffectMask,
Hydrating,
Expand Down Expand Up @@ -2310,6 +2311,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
): void {
if (runAllPassiveEffectDestroysBeforeCreates) {
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag |= PassiveUnmountPendingDev;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag |= PassiveUnmountPendingDev;
}
}
}
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand Down Expand Up @@ -2372,6 +2382,17 @@ function flushPassiveEffectsImpl() {
const fiber = ((unmountEffects[i + 1]: any): Fiber);
const destroy = effect.destroy;
effect.destroy = undefined;

if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag &= ~PassiveUnmountPendingDev;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag &= ~PassiveUnmountPendingDev;
}
}
}

if (typeof destroy === 'function') {
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
Expand Down Expand Up @@ -2851,7 +2872,7 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
) {
// If there are pending passive effects unmounts for this Fiber,
// we can assume that they would have prevented this update.
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
if ((fiber.effectTag & PassiveUnmountPendingDev) !== NoEffect) {
return;
}
}
Expand Down
23 changes: 22 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ import {
Snapshot,
Callback,
Passive,
PassiveUnmountPendingDev,
Incomplete,
HostEffectMask,
Hydrating,
Expand Down Expand Up @@ -2329,6 +2330,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
): void {
if (runAllPassiveEffectDestroysBeforeCreates) {
pendingPassiveHookEffectsUnmount.push(effect, fiber);
if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag |= PassiveUnmountPendingDev;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag |= PassiveUnmountPendingDev;
}
}
}
if (!rootDoesHavePassiveEffects) {
rootDoesHavePassiveEffects = true;
scheduleCallback(NormalPriority, () => {
Expand Down Expand Up @@ -2391,6 +2401,17 @@ function flushPassiveEffectsImpl() {
const fiber = ((unmountEffects[i + 1]: any): Fiber);
const destroy = effect.destroy;
effect.destroy = undefined;

if (__DEV__) {
if (deferPassiveEffectCleanupDuringUnmount) {
fiber.effectTag &= ~PassiveUnmountPendingDev;
const alternate = fiber.alternate;
if (alternate !== null) {
alternate.effectTag &= ~PassiveUnmountPendingDev;
}
}
}

if (typeof destroy === 'function') {
if (__DEV__) {
setCurrentDebugFiberInDEV(fiber);
Expand Down Expand Up @@ -2873,7 +2894,7 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
) {
// If there are pending passive effects unmounts for this Fiber,
// we can assume that they would have prevented this update.
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
if ((fiber.effectTag & PassiveUnmountPendingDev) !== NoEffect) {
return;
}
}
Expand Down
37 changes: 19 additions & 18 deletions packages/react-reconciler/src/ReactSideEffectTags.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,29 @@
export type SideEffectTag = number;

// Don't change these two values. They're used by React Dev Tools.
export const NoEffect = /* */ 0b0000000000000;
export const PerformedWork = /* */ 0b0000000000001;
export const NoEffect = /* */ 0b00000000000000;
export const PerformedWork = /* */ 0b00000000000001;

// You can change the rest (and add more).
export const Placement = /* */ 0b0000000000010;
export const Update = /* */ 0b0000000000100;
export const PlacementAndUpdate = /* */ 0b0000000000110;
export const Deletion = /* */ 0b0000000001000;
export const ContentReset = /* */ 0b0000000010000;
export const Callback = /* */ 0b0000000100000;
export const DidCapture = /* */ 0b0000001000000;
export const Ref = /* */ 0b0000010000000;
export const Snapshot = /* */ 0b0000100000000;
export const Passive = /* */ 0b0001000000000;
export const Hydrating = /* */ 0b0010000000000;
export const HydratingAndUpdate = /* */ 0b0010000000100;
export const Placement = /* */ 0b00000000000010;
export const Update = /* */ 0b00000000000100;
export const PlacementAndUpdate = /* */ 0b00000000000110;
export const Deletion = /* */ 0b00000000001000;
export const ContentReset = /* */ 0b00000000010000;
export const Callback = /* */ 0b00000000100000;
export const DidCapture = /* */ 0b00000001000000;
export const Ref = /* */ 0b00000010000000;
export const Snapshot = /* */ 0b00000100000000;
export const Passive = /* */ 0b00001000000000;
export const PassiveUnmountPendingDev = /* */ 0b10000000000000;
export const Hydrating = /* */ 0b00010000000000;
export const HydratingAndUpdate = /* */ 0b00010000000100;

// Passive & Update & Callback & Ref & Snapshot
export const LifecycleEffectMask = /* */ 0b0001110100100;
export const LifecycleEffectMask = /* */ 0b00001110100100;

// Union of all host effects
export const HostEffectMask = /* */ 0b0011111111111;
export const HostEffectMask = /* */ 0b00011111111111;

export const Incomplete = /* */ 0b0100000000000;
export const ShouldCapture = /* */ 0b1000000000000;
export const Incomplete = /* */ 0b00100000000000;
export const ShouldCapture = /* */ 0b01000000000000;
89 changes: 89 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,95 @@ describe('ReactHooksWithNoopRenderer', () => {
});
});

// @gate deferPassiveEffectCleanupDuringUnmount && runAllPassiveEffectDestroysBeforeCreates
it('does not warn about state updates for unmounted components with pending passive unmounts for alternates', () => {
let setParentState = null;
const setChildStates = [];

function Parent() {
const [state, setState] = useState(true);
setParentState = setState;
Scheduler.unstable_yieldValue(`Parent ${state} render`);
useLayoutEffect(() => {
Scheduler.unstable_yieldValue(`Parent ${state} commit`);
});
if (state) {
return (
<>
<Child label="one" />
<Child label="two" />
</>
);
} else {
return null;
}
}

function Child({label}) {
const [state, setState] = useState(0);
useLayoutEffect(() => {
Scheduler.unstable_yieldValue(`Child ${label} commit`);
});
useEffect(() => {
setChildStates.push(setState);
Scheduler.unstable_yieldValue(`Child ${label} passive create`);
return () => {
Scheduler.unstable_yieldValue(`Child ${label} passive destroy`);
};
}, []);
Scheduler.unstable_yieldValue(`Child ${label} render`);
return state;
}

// Schedule debounced state update for child (prob a no-op for this test)
// later tick: schedule unmount for parent
// start process unmount (but don't flush passive effectS)
// State update on child
act(() => {
ReactNoop.render(<Parent />);
expect(Scheduler).toFlushAndYieldThrough([
'Parent true render',
'Child one render',
'Child two render',
'Child one commit',
'Child two commit',
'Parent true commit',
'Child one passive create',
'Child two passive create',
]);

// Update children.
setChildStates.forEach(setChildState => setChildState(1));
expect(Scheduler).toFlushAndYieldThrough([
'Child one render',
'Child two render',
'Child one commit',
'Child two commit',
]);

// Schedule another update for children, and partially process it.
setChildStates.forEach(setChildState => setChildState(2));
expect(Scheduler).toFlushAndYieldThrough(['Child one render']);

// Schedule unmount for the parent that unmounts children with pending update.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => setParentState(false),
);
expect(Scheduler).toFlushAndYieldThrough([
'Parent false render',
'Parent false commit',
]);

// Schedule updates for children too (which should be ignored)
setChildStates.forEach(setChildState => setChildState(2));
expect(Scheduler).toFlushAndYield([
'Child one passive destroy',
'Child two passive destroy',
]);
});
});

it('warns about state updates for unmounted components with no pending passive unmounts', () => {
let completePendingRequest = null;
function Component() {
Expand Down

0 comments on commit 263bc5d

Please sign in to comment.