Skip to content

Commit fbd5194

Browse files
author
Brian Vaughn
committed
Don't warn about unmounted updates if pending passive unmount
I recently landed a change to the timing of passive effect cleanup functions during unmount (see #17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects). Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending.
1 parent 65bbda7 commit fbd5194

File tree

2 files changed

+191
-0
lines changed

2 files changed

+191
-0
lines changed

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import type {Effect as HookEffect} from './ReactFiberHooks';
1818

1919
import {
2020
warnAboutDeprecatedLifecycles,
21+
deferPassiveEffectCleanupDuringUnmount,
2122
runAllPassiveEffectDestroysBeforeCreates,
2223
enableUserTimingAPI,
2324
enableSuspenseServerRenderer,
@@ -2687,6 +2688,18 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
26872688
// Only warn for user-defined components, not internal ones like Suspense.
26882689
return;
26892690
}
2691+
2692+
if (
2693+
deferPassiveEffectCleanupDuringUnmount &&
2694+
runAllPassiveEffectDestroysBeforeCreates
2695+
) {
2696+
// If there are pending passive effects unmounts for this Fiber,
2697+
// we can assume that they would have prevented this update.
2698+
if (pendingPassiveHookEffectsUnmount.indexOf(fiber) >= 0) {
2699+
return;
2700+
}
2701+
}
2702+
26902703
// We show the whole stack but dedupe on the top component's name because
26912704
// the problematic code almost always lies inside that component.
26922705
const componentName = getComponentName(fiber.type) || 'ReactComponent';

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

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1144,6 +1144,184 @@ function loadModules({
11441144
]);
11451145
});
11461146
});
1147+
1148+
it('does not warn about state updates for unmounted components with pending passive unmounts', () => {
1149+
let completePendingRequest = null;
1150+
function Component() {
1151+
Scheduler.unstable_yieldValue('Component');
1152+
const [didLoad, setDidLoad] = React.useState(false);
1153+
React.useLayoutEffect(() => {
1154+
Scheduler.unstable_yieldValue('layout create');
1155+
return () => {
1156+
Scheduler.unstable_yieldValue('layout destroy');
1157+
};
1158+
}, []);
1159+
React.useEffect(() => {
1160+
Scheduler.unstable_yieldValue('passive create');
1161+
// Mimic an XHR request with a complete handler that updates state.
1162+
completePendingRequest = () => setDidLoad(true);
1163+
return () => {
1164+
Scheduler.unstable_yieldValue('passive destroy');
1165+
};
1166+
}, []);
1167+
return didLoad;
1168+
}
1169+
1170+
act(() => {
1171+
ReactNoop.renderToRootWithID(<Component />, 'root', () =>
1172+
Scheduler.unstable_yieldValue('Sync effect'),
1173+
);
1174+
expect(Scheduler).toFlushAndYieldThrough([
1175+
'Component',
1176+
'layout create',
1177+
'Sync effect',
1178+
]);
1179+
ReactNoop.flushPassiveEffects();
1180+
expect(Scheduler).toHaveYielded(['passive create']);
1181+
1182+
// Unmount but don't process pending passive destroy function
1183+
ReactNoop.unmountRootWithID('root');
1184+
expect(Scheduler).toFlushAndYieldThrough(['layout destroy']);
1185+
1186+
// Simulate an XHR completing, which will cause a state update-
1187+
// but should not log a warning.
1188+
completePendingRequest();
1189+
1190+
ReactNoop.flushPassiveEffects();
1191+
expect(Scheduler).toHaveYielded(['passive destroy']);
1192+
});
1193+
});
1194+
1195+
it('still warns about state updates for unmounted components with no pending passive unmounts', () => {
1196+
let completePendingRequest = null;
1197+
function Component() {
1198+
Scheduler.unstable_yieldValue('Component');
1199+
const [didLoad, setDidLoad] = React.useState(false);
1200+
React.useLayoutEffect(() => {
1201+
Scheduler.unstable_yieldValue('layout create');
1202+
// Mimic an XHR request with a complete handler that updates state.
1203+
completePendingRequest = () => setDidLoad(true);
1204+
return () => {
1205+
Scheduler.unstable_yieldValue('layout destroy');
1206+
};
1207+
}, []);
1208+
return didLoad;
1209+
}
1210+
1211+
act(() => {
1212+
ReactNoop.renderToRootWithID(<Component />, 'root', () =>
1213+
Scheduler.unstable_yieldValue('Sync effect'),
1214+
);
1215+
expect(Scheduler).toFlushAndYieldThrough([
1216+
'Component',
1217+
'layout create',
1218+
'Sync effect',
1219+
]);
1220+
1221+
// Unmount but don't process pending passive destroy function
1222+
ReactNoop.unmountRootWithID('root');
1223+
expect(Scheduler).toFlushAndYieldThrough(['layout destroy']);
1224+
1225+
// Simulate an XHR completing.
1226+
expect(completePendingRequest).toErrorDev(
1227+
"Warning: Can't perform a React state update on an unmounted component.",
1228+
);
1229+
});
1230+
});
1231+
1232+
it('still warns if there are pending passive unmount effects but not for the current fiber', () => {
1233+
let completePendingRequest = null;
1234+
function ComponentWithXHR() {
1235+
Scheduler.unstable_yieldValue('Component');
1236+
const [didLoad, setDidLoad] = React.useState(false);
1237+
React.useLayoutEffect(() => {
1238+
Scheduler.unstable_yieldValue('a:layout create');
1239+
return () => {
1240+
Scheduler.unstable_yieldValue('a:layout destroy');
1241+
};
1242+
}, []);
1243+
React.useEffect(() => {
1244+
Scheduler.unstable_yieldValue('a:passive create');
1245+
// Mimic an XHR request with a complete handler that updates state.
1246+
completePendingRequest = () => setDidLoad(true);
1247+
}, []);
1248+
return didLoad;
1249+
}
1250+
1251+
function ComponentWithPendingPassiveUnmount() {
1252+
React.useEffect(() => {
1253+
Scheduler.unstable_yieldValue('b:passive create');
1254+
return () => {
1255+
Scheduler.unstable_yieldValue('b:passive destroy');
1256+
};
1257+
}, []);
1258+
return null;
1259+
}
1260+
1261+
act(() => {
1262+
ReactNoop.renderToRootWithID(
1263+
<>
1264+
<ComponentWithXHR />
1265+
<ComponentWithPendingPassiveUnmount />
1266+
</>,
1267+
'root',
1268+
() => Scheduler.unstable_yieldValue('Sync effect'),
1269+
);
1270+
expect(Scheduler).toFlushAndYieldThrough([
1271+
'Component',
1272+
'a:layout create',
1273+
'Sync effect',
1274+
]);
1275+
ReactNoop.flushPassiveEffects();
1276+
expect(Scheduler).toHaveYielded([
1277+
'a:passive create',
1278+
'b:passive create',
1279+
]);
1280+
1281+
// Unmount but don't process pending passive destroy function
1282+
ReactNoop.unmountRootWithID('root');
1283+
expect(Scheduler).toFlushAndYieldThrough(['a:layout destroy']);
1284+
1285+
// Simulate an XHR completing in the component without a pending passive effect..
1286+
expect(completePendingRequest).toErrorDev(
1287+
"Warning: Can't perform a React state update on an unmounted component.",
1288+
);
1289+
});
1290+
});
1291+
1292+
it('still warns about state updates from within passive unmount function', () => {
1293+
function Component() {
1294+
Scheduler.unstable_yieldValue('Component');
1295+
const [didLoad, setDidLoad] = React.useState(false);
1296+
React.useEffect(() => {
1297+
Scheduler.unstable_yieldValue('passive create');
1298+
return () => {
1299+
setDidLoad(true);
1300+
Scheduler.unstable_yieldValue('passive destroy');
1301+
};
1302+
}, []);
1303+
return didLoad;
1304+
}
1305+
1306+
act(() => {
1307+
ReactNoop.renderToRootWithID(<Component />, 'root', () =>
1308+
Scheduler.unstable_yieldValue('Sync effect'),
1309+
);
1310+
expect(Scheduler).toFlushAndYieldThrough([
1311+
'Component',
1312+
'Sync effect',
1313+
'passive create',
1314+
]);
1315+
1316+
// Unmount but don't process pending passive destroy function
1317+
ReactNoop.unmountRootWithID('root');
1318+
expect(() => {
1319+
expect(Scheduler).toFlushAndYield(['passive destroy']);
1320+
}).toErrorDev(
1321+
"Warning: Can't perform a React state update on an unmounted component.",
1322+
);
1323+
});
1324+
});
11471325
}
11481326

11491327
it('updates have async priority', () => {

0 commit comments

Comments
 (0)