Don't show destroy function state update warning when updating ancestors (#18409)
React can't directly detect a memory leak, but there are some clues that warn about one. One of these clues is when an unmounted React component tries to update its state. For example, if a component forgets to remove an event listener when unmounting, that listener may be called later and try to update state, at which point React would warn about the potential leak. Warning signals like this are more useful if they're strong. For this reason, it's good to always avoid updating state from inside of an effect's cleanup function. Even when you know there is no potential leak, React has no way to know and so it will warn anyway. In most cases we suggest moving state updates to the useEffect() body instead (to avoid triggering the warning). This works so long as the component is updating its own state (or the state of a descendant). However this will not work when a component updates its parent state in a cleanup function. If such a component is unmounted but its parent remains mounted, the state will be incorrect. For this reason, we now avoid showing the warning if a component is updating an ancestor.
Brian Vaughn committed
Mar 28, 2020 at 10:18 UTC
be4c8b19c16a7558e2939a6665399f6f3202668e
2 files changed
+113
-10
packages/react-reconciler/src/ReactFiberWorkLoop.js
+31
-8
@@ -161,6 +161,7 @@ import {
161
import getComponentName from 'shared/getComponentName';
162
import ReactStrictModeWarnings from './ReactStrictModeWarnings';
163
import {
164
+ current as currentDebugFiberInDEV,
165
isRendering as ReactCurrentDebugFiberIsRenderingInDEV,
166
resetCurrentFiber as resetCurrentDebugFiberInDEV,
167
setCurrentFiber as setCurrentDebugFiberInDEV,
@@ -2822,14 +2823,36 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
2823
2824
// If we are currently flushing passive effects, change the warning text.
2825
if (isFlushingPassiveEffects) {
2825
- console.error(
2826
- "Can't perform a React state update from within a useEffect cleanup function. " +
2827
- 'To fix, move state updates to the useEffect() body in %s.%s',
2828
- tag === ClassComponent
2829
- ? 'the componentWillUnmount method'
2830
- : 'a useEffect cleanup function',
2831
- getStackByFiberInDevAndProd(fiber),
2832
- );
2826
+ // React can't directly detect a memory leak, but there are some clues that warn about one.
2827
+ // One of these clues is when an unmounted React component tries to update its state.
2828
+ // For example, if a component forgets to remove an event listener when unmounting,
2829
+ // that listener may be called later and try to update state, at which point React would warn about the potential leak.
2830
+ //
2831
+ // Warning signals like this are more useful if they're strong.
2832
+ // For this reason, it's good to always avoid updating state from inside of an effect's cleanup function.
2833
+ // Even when you know there is no potential leak, React has no way to know and so it will warn anyway.
2834
+ // In most cases we suggest moving state updates to the useEffect() body instead.
2835
+ // This works so long as the component is updating its own state (or the state of a descendant).
2836
+ //
2837
+ // However this will not work when a component updates its parent state in a cleanup function.
2838
+ // If such a component is unmounted but its parent remains mounted, the state will be out of sync.
2839
+ // For this reason, we avoid showing the warning if a component is updating an ancestor.
2840
+ let currentTargetFiber = fiber;
2841
+ while (currentTargetFiber !== null) {
2842
+ // currentDebugFiberInDEV owns the effect destroy function currently being invoked.
2843
+ if (
2844
+ currentDebugFiberInDEV === currentTargetFiber ||
2845
+ currentDebugFiberInDEV === currentTargetFiber.alternate
2846
+ ) {
2847
+ console.error(
2848
+ "Can't perform a React state update from within a useEffect cleanup function. " +
2849
+ 'To fix, move state updates to the useEffect() body.%s',
2850
+ getStackByFiberInDevAndProd(((currentDebugFiberInDEV: any): Fiber)),
2851
+ );
2852
+ break;
2853
+ }
2854
+ currentTargetFiber = currentTargetFiber.return;
2855
+ }
2856
} else {
2857
console.error(
2858
"Can't perform a React state update on an unmounted component. This " +
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+82
-2
@@ -1305,7 +1305,7 @@ describe('ReactHooksWithNoopRenderer', () => {
1305
});
1306
});
1307
1308
- it('shows a unique warning for state updates from within passive unmount function', () => {
1308
+ it('shows a warning when a component updates its own state from within passive unmount function', () => {
1309
function Component() {
1310
Scheduler.unstable_yieldValue('Component');
1311
const [didLoad, setDidLoad] = React.useState(false);
@@ -1334,10 +1334,90 @@ describe('ReactHooksWithNoopRenderer', () => {
1334
expect(() => {
1335
expect(Scheduler).toFlushAndYield(['passive destroy']);
1336
}).toErrorDev(
1337
- "Warning: Can't perform a React state update from within a useEffect cleanup function.",
1337
+ "Warning: Can't perform a React state update from within a useEffect cleanup function. " +
1338
+ 'To fix, move state updates to the useEffect() body.\n' +
1339
+ ' in Component (at **)',
1340
);
1341
});
1342
});
1343
+
1344
+ it('shows a warning when a component updates a childs state from within passive unmount function', () => {
1345
+ function Parent() {
1346
+ Scheduler.unstable_yieldValue('Parent');
1347
+ const updaterRef = React.useRef(null);
1348
+ React.useEffect(() => {
1349
+ Scheduler.unstable_yieldValue('Parent passive create');
1350
+ return () => {
1351
+ updaterRef.current(true);
1352
+ Scheduler.unstable_yieldValue('Parent passive destroy');
1353
+ };
1354
+ }, []);
1355
+ return <Child updaterRef={updaterRef} />;
1356
+ }
1357
+
1358
+ function Child({updaterRef}) {
1359
+ Scheduler.unstable_yieldValue('Child');
1360
+ const [state, setState] = React.useState(false);
1361
+ React.useEffect(() => {
1362
+ Scheduler.unstable_yieldValue('Child passive create');
1363
+ updaterRef.current = setState;
1364
+ }, []);
1365
+ return state;
1366
+ }
1367
+
1368
+ act(() => {
1369
+ ReactNoop.renderToRootWithID(<Parent />, 'root');
1370
+ expect(Scheduler).toFlushAndYieldThrough([
1371
+ 'Parent',
1372
+ 'Child',
1373
+ 'Child passive create',
1374
+ 'Parent passive create',
1375
+ ]);
1376
+
1377
+ // Unmount but don't process pending passive destroy function
1378
+ ReactNoop.unmountRootWithID('root');
1379
+ expect(() => {
1380
+ expect(Scheduler).toFlushAndYield(['Parent passive destroy']);
1381
+ }).toErrorDev(
1382
+ "Warning: Can't perform a React state update from within a useEffect cleanup function. " +
1383
+ 'To fix, move state updates to the useEffect() body.\n' +
1384
+ ' in Parent (at **)',
1385
+ );
1386
+ });
1387
+ });
1388
+
1389
+ it('does not show a warning when a component updates a parents state from within passive unmount function', () => {
1390
+ function Parent() {
1391
+ const [state, setState] = React.useState(false);
1392
+ Scheduler.unstable_yieldValue('Parent');
1393
+ return <Child setState={setState} state={state} />;
1394
+ }
1395
+
1396
+ function Child({setState, state}) {
1397
+ Scheduler.unstable_yieldValue('Child');
1398
+ React.useEffect(() => {
1399
+ Scheduler.unstable_yieldValue('Child passive create');
1400
+ return () => {
1401
+ Scheduler.unstable_yieldValue('Child passive destroy');
1402
+ setState(true);
1403
+ };
1404
+ }, []);
1405
+ return state;
1406
+ }
1407
+
1408
+ act(() => {
1409
+ ReactNoop.renderToRootWithID(<Parent />, 'root');
1410
+ expect(Scheduler).toFlushAndYieldThrough([
1411
+ 'Parent',
1412
+ 'Child',
1413
+ 'Child passive create',
1414
+ ]);
1415
+
1416
+ // Unmount but don't process pending passive destroy function
1417
+ ReactNoop.unmountRootWithID('root');
1418
+ expect(Scheduler).toFlushAndYield(['Child passive destroy']);
1419
+ });
1420
+ });
1421
}
1422
1423
it('updates have async priority', () => {