Skip to content

Commit 84fb700

Browse files
author
Brian Vaughn
committed
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 the alternates.
1 parent b680174 commit 84fb700

File tree

3 files changed

+133
-2
lines changed

3 files changed

+133
-2
lines changed

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,6 +251,7 @@ let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
251251
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
252252
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
253253
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
254+
let pendingPassiveHookEffectsUnmountAlternatesDEV: Array<Fiber> = [];
254255
let pendingPassiveProfilerEffects: Array<Fiber> = [];
255256

256257
let rootsWithPendingDiscreteUpdates: Map<
@@ -2208,6 +2209,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
22082209
): void {
22092210
if (runAllPassiveEffectDestroysBeforeCreates) {
22102211
pendingPassiveHookEffectsUnmount.push(effect, fiber);
2212+
if (__DEV__) {
2213+
if (deferPassiveEffectCleanupDuringUnmount) {
2214+
if (fiber.alternate !== null) {
2215+
// Alternate pointers get disconnected during unmount.
2216+
// Track alternates explicitly here to avoid warning about state updates for unmounted components.
2217+
pendingPassiveHookEffectsUnmountAlternatesDEV.push(fiber.alternate);
2218+
}
2219+
}
2220+
}
22112221
if (!rootDoesHavePassiveEffects) {
22122222
rootDoesHavePassiveEffects = true;
22132223
scheduleCallback(NormalPriority, () => {
@@ -2257,6 +2267,14 @@ function flushPassiveEffectsImpl() {
22572267
// First pass: Destroy stale passive effects.
22582268
const unmountEffects = pendingPassiveHookEffectsUnmount;
22592269
pendingPassiveHookEffectsUnmount = [];
2270+
if (__DEV__) {
2271+
if (
2272+
deferPassiveEffectCleanupDuringUnmount &&
2273+
runAllPassiveEffectDestroysBeforeCreates
2274+
) {
2275+
pendingPassiveHookEffectsUnmountAlternatesDEV = [];
2276+
}
2277+
}
22602278
for (let i = 0; i < unmountEffects.length; i += 2) {
22612279
const effect = ((unmountEffects[i]: any): HookEffect);
22622280
const fiber = ((unmountEffects[i + 1]: any): Fiber);
@@ -2735,7 +2753,10 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
27352753
) {
27362754
// If there are pending passive effects unmounts for this Fiber,
27372755
// we can assume that they would have prevented this update.
2738-
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
2756+
if (
2757+
pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0 ||
2758+
pendingPassiveHookEffectsUnmountAlternatesDEV.indexOf(fiber) >= 0
2759+
) {
27392760
return;
27402761
}
27412762
}

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

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,7 @@ let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
249249
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
250250
let pendingPassiveHookEffectsMount: Array<HookEffect | Fiber> = [];
251251
let pendingPassiveHookEffectsUnmount: Array<HookEffect | Fiber> = [];
252+
let pendingPassiveHookEffectsUnmountAlternatesDEV: Array<Fiber> = [];
252253
let pendingPassiveProfilerEffects: Array<Fiber> = [];
253254

254255
let rootsWithPendingDiscreteUpdates: Map<
@@ -2227,6 +2228,15 @@ export function enqueuePendingPassiveHookEffectUnmount(
22272228
): void {
22282229
if (runAllPassiveEffectDestroysBeforeCreates) {
22292230
pendingPassiveHookEffectsUnmount.push(effect, fiber);
2231+
if (__DEV__) {
2232+
if (deferPassiveEffectCleanupDuringUnmount) {
2233+
if (fiber.alternate !== null) {
2234+
// Alternate pointers get disconnected during unmount.
2235+
// Track alternates explicitly here to avoid warning about state updates for unmounted components.
2236+
pendingPassiveHookEffectsUnmountAlternatesDEV.push(fiber.alternate);
2237+
}
2238+
}
2239+
}
22302240
if (!rootDoesHavePassiveEffects) {
22312241
rootDoesHavePassiveEffects = true;
22322242
scheduleCallback(NormalPriority, () => {
@@ -2276,6 +2286,14 @@ function flushPassiveEffectsImpl() {
22762286
// First pass: Destroy stale passive effects.
22772287
const unmountEffects = pendingPassiveHookEffectsUnmount;
22782288
pendingPassiveHookEffectsUnmount = [];
2289+
if (__DEV__) {
2290+
if (
2291+
deferPassiveEffectCleanupDuringUnmount &&
2292+
runAllPassiveEffectDestroysBeforeCreates
2293+
) {
2294+
pendingPassiveHookEffectsUnmountAlternatesDEV = [];
2295+
}
2296+
}
22792297
for (let i = 0; i < unmountEffects.length; i += 2) {
22802298
const effect = ((unmountEffects[i]: any): HookEffect);
22812299
const fiber = ((unmountEffects[i + 1]: any): Fiber);
@@ -2757,7 +2775,10 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
27572775
) {
27582776
// If there are pending passive effects unmounts for this Fiber,
27592777
// we can assume that they would have prevented this update.
2760-
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
2778+
if (
2779+
pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0 ||
2780+
pendingPassiveHookEffectsUnmountAlternatesDEV.indexOf(fiber) >= 0
2781+
) {
27612782
return;
27622783
}
27632784
}

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)