Skip to content

Commit 263bc5d

Browse files
author
Brian Vaughn
authored
Fix incorrect unmounted state update warning (facebook#18617)
* 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.
1 parent 52a0d6b commit 263bc5d

File tree

4 files changed

+152
-20
lines changed

4 files changed

+152
-20
lines changed

‎packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ import {
116116
Snapshot,
117117
Callback,
118118
Passive,
119+
PassiveUnmountPendingDev,
119120
Incomplete,
120121
HostEffectMask,
121122
Hydrating,
@@ -2310,6 +2311,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
23102311
): void {
23112312
if (runAllPassiveEffectDestroysBeforeCreates) {
23122313
pendingPassiveHookEffectsUnmount.push(effect, fiber);
2314+
if (__DEV__) {
2315+
if (deferPassiveEffectCleanupDuringUnmount) {
2316+
fiber.effectTag |= PassiveUnmountPendingDev;
2317+
const alternate = fiber.alternate;
2318+
if (alternate !== null) {
2319+
alternate.effectTag |= PassiveUnmountPendingDev;
2320+
}
2321+
}
2322+
}
23132323
if (!rootDoesHavePassiveEffects) {
23142324
rootDoesHavePassiveEffects = true;
23152325
scheduleCallback(NormalPriority, () => {
@@ -2372,6 +2382,17 @@ function flushPassiveEffectsImpl() {
23722382
const fiber = ((unmountEffects[i + 1]: any): Fiber);
23732383
const destroy = effect.destroy;
23742384
effect.destroy = undefined;
2385+
2386+
if (__DEV__) {
2387+
if (deferPassiveEffectCleanupDuringUnmount) {
2388+
fiber.effectTag &= ~PassiveUnmountPendingDev;
2389+
const alternate = fiber.alternate;
2390+
if (alternate !== null) {
2391+
alternate.effectTag &= ~PassiveUnmountPendingDev;
2392+
}
2393+
}
2394+
}
2395+
23752396
if (typeof destroy === 'function') {
23762397
if (__DEV__) {
23772398
setCurrentDebugFiberInDEV(fiber);
@@ -2851,7 +2872,7 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
28512872
) {
28522873
// If there are pending passive effects unmounts for this Fiber,
28532874
// we can assume that they would have prevented this update.
2854-
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
2875+
if ((fiber.effectTag & PassiveUnmountPendingDev) !== NoEffect) {
28552876
return;
28562877
}
28572878
}

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,7 @@ import {
116116
Snapshot,
117117
Callback,
118118
Passive,
119+
PassiveUnmountPendingDev,
119120
Incomplete,
120121
HostEffectMask,
121122
Hydrating,
@@ -2329,6 +2330,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
23292330
): void {
23302331
if (runAllPassiveEffectDestroysBeforeCreates) {
23312332
pendingPassiveHookEffectsUnmount.push(effect, fiber);
2333+
if (__DEV__) {
2334+
if (deferPassiveEffectCleanupDuringUnmount) {
2335+
fiber.effectTag |= PassiveUnmountPendingDev;
2336+
const alternate = fiber.alternate;
2337+
if (alternate !== null) {
2338+
alternate.effectTag |= PassiveUnmountPendingDev;
2339+
}
2340+
}
2341+
}
23322342
if (!rootDoesHavePassiveEffects) {
23332343
rootDoesHavePassiveEffects = true;
23342344
scheduleCallback(NormalPriority, () => {
@@ -2391,6 +2401,17 @@ function flushPassiveEffectsImpl() {
23912401
const fiber = ((unmountEffects[i + 1]: any): Fiber);
23922402
const destroy = effect.destroy;
23932403
effect.destroy = undefined;
2404+
2405+
if (__DEV__) {
2406+
if (deferPassiveEffectCleanupDuringUnmount) {
2407+
fiber.effectTag &= ~PassiveUnmountPendingDev;
2408+
const alternate = fiber.alternate;
2409+
if (alternate !== null) {
2410+
alternate.effectTag &= ~PassiveUnmountPendingDev;
2411+
}
2412+
}
2413+
}
2414+
23942415
if (typeof destroy === 'function') {
23952416
if (__DEV__) {
23962417
setCurrentDebugFiberInDEV(fiber);
@@ -2873,7 +2894,7 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
28732894
) {
28742895
// If there are pending passive effects unmounts for this Fiber,
28752896
// we can assume that they would have prevented this update.
2876-
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
2897+
if ((fiber.effectTag & PassiveUnmountPendingDev) !== NoEffect) {
28772898
return;
28782899
}
28792900
}

‎packages/react-reconciler/src/ReactSideEffectTags.js

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -10,28 +10,29 @@
1010
export type SideEffectTag = number;
1111

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

1616
// You can change the rest (and add more).
17-
export const Placement = /* */ 0b0000000000010;
18-
export const Update = /* */ 0b0000000000100;
19-
export const PlacementAndUpdate = /* */ 0b0000000000110;
20-
export const Deletion = /* */ 0b0000000001000;
21-
export const ContentReset = /* */ 0b0000000010000;
22-
export const Callback = /* */ 0b0000000100000;
23-
export const DidCapture = /* */ 0b0000001000000;
24-
export const Ref = /* */ 0b0000010000000;
25-
export const Snapshot = /* */ 0b0000100000000;
26-
export const Passive = /* */ 0b0001000000000;
27-
export const Hydrating = /* */ 0b0010000000000;
28-
export const HydratingAndUpdate = /* */ 0b0010000000100;
17+
export const Placement = /* */ 0b00000000000010;
18+
export const Update = /* */ 0b00000000000100;
19+
export const PlacementAndUpdate = /* */ 0b00000000000110;
20+
export const Deletion = /* */ 0b00000000001000;
21+
export const ContentReset = /* */ 0b00000000010000;
22+
export const Callback = /* */ 0b00000000100000;
23+
export const DidCapture = /* */ 0b00000001000000;
24+
export const Ref = /* */ 0b00000010000000;
25+
export const Snapshot = /* */ 0b00000100000000;
26+
export const Passive = /* */ 0b00001000000000;
27+
export const PassiveUnmountPendingDev = /* */ 0b10000000000000;
28+
export const Hydrating = /* */ 0b00010000000000;
29+
export const HydratingAndUpdate = /* */ 0b00010000000100;
2930

3031
// Passive & Update & Callback & Ref & Snapshot
31-
export const LifecycleEffectMask = /* */ 0b0001110100100;
32+
export const LifecycleEffectMask = /* */ 0b00001110100100;
3233

3334
// Union of all host effects
34-
export const HostEffectMask = /* */ 0b0011111111111;
35+
export const HostEffectMask = /* */ 0b00011111111111;
3536

36-
export const Incomplete = /* */ 0b0100000000000;
37-
export const ShouldCapture = /* */ 0b1000000000000;
37+
export const Incomplete = /* */ 0b00100000000000;
38+
export const ShouldCapture = /* */ 0b01000000000000;

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1252,6 +1252,95 @@ describe('ReactHooksWithNoopRenderer', () => {
12521252
});
12531253
});
12541254

1255+
// @gate deferPassiveEffectCleanupDuringUnmount && runAllPassiveEffectDestroysBeforeCreates
1256+
it('does not warn about state updates for unmounted components with pending passive unmounts for alternates', () => {
1257+
let setParentState = null;
1258+
const setChildStates = [];
1259+
1260+
function Parent() {
1261+
const [state, setState] = useState(true);
1262+
setParentState = setState;
1263+
Scheduler.unstable_yieldValue(`Parent ${state} render`);
1264+
useLayoutEffect(() => {
1265+
Scheduler.unstable_yieldValue(`Parent ${state} commit`);
1266+
});
1267+
if (state) {
1268+
return (
1269+
<>
1270+
<Child label="one" />
1271+
<Child label="two" />
1272+
</>
1273+
);
1274+
} else {
1275+
return null;
1276+
}
1277+
}
1278+
1279+
function Child({label}) {
1280+
const [state, setState] = useState(0);
1281+
useLayoutEffect(() => {
1282+
Scheduler.unstable_yieldValue(`Child ${label} commit`);
1283+
});
1284+
useEffect(() => {
1285+
setChildStates.push(setState);
1286+
Scheduler.unstable_yieldValue(`Child ${label} passive create`);
1287+
return () => {
1288+
Scheduler.unstable_yieldValue(`Child ${label} passive destroy`);
1289+
};
1290+
}, []);
1291+
Scheduler.unstable_yieldValue(`Child ${label} render`);
1292+
return state;
1293+
}
1294+
1295+
// Schedule debounced state update for child (prob a no-op for this test)
1296+
// later tick: schedule unmount for parent
1297+
// start process unmount (but don't flush passive effectS)
1298+
// State update on child
1299+
act(() => {
1300+
ReactNoop.render(<Parent />);
1301+
expect(Scheduler).toFlushAndYieldThrough([
1302+
'Parent true render',
1303+
'Child one render',
1304+
'Child two render',
1305+
'Child one commit',
1306+
'Child two commit',
1307+
'Parent true commit',
1308+
'Child one passive create',
1309+
'Child two passive create',
1310+
]);
1311+
1312+
// Update children.
1313+
setChildStates.forEach(setChildState => setChildState(1));
1314+
expect(Scheduler).toFlushAndYieldThrough([
1315+
'Child one render',
1316+
'Child two render',
1317+
'Child one commit',
1318+
'Child two commit',
1319+
]);
1320+
1321+
// Schedule another update for children, and partially process it.
1322+
setChildStates.forEach(setChildState => setChildState(2));
1323+
expect(Scheduler).toFlushAndYieldThrough(['Child one render']);
1324+
1325+
// Schedule unmount for the parent that unmounts children with pending update.
1326+
Scheduler.unstable_runWithPriority(
1327+
Scheduler.unstable_UserBlockingPriority,
1328+
() => setParentState(false),
1329+
);
1330+
expect(Scheduler).toFlushAndYieldThrough([
1331+
'Parent false render',
1332+
'Parent false commit',
1333+
]);
1334+
1335+
// Schedule updates for children too (which should be ignored)
1336+
setChildStates.forEach(setChildState => setChildState(2));
1337+
expect(Scheduler).toFlushAndYield([
1338+
'Child one passive destroy',
1339+
'Child two passive destroy',
1340+
]);
1341+
});
1342+
});
1343+
12551344
it('warns about state updates for unmounted components with no pending passive unmounts', () => {
12561345
let completePendingRequest = null;
12571346
function Component() {

0 commit comments

Comments
 (0)