Remove state update warning for passive effect cleanup functions (#18453)
Brian Vaughn committed
Apr 1, 2020 at 10:49 UTC
5ee0efe832769b845c98a41df8736e09a4f1061f
3 files changed
+56
-45
packages/react-reconciler/src/ReactFiberWorkLoop.js
+9
-28
@@ -160,7 +160,6 @@ import {
160
import getComponentName from 'shared/getComponentName';
161
import ReactStrictModeWarnings from './ReactStrictModeWarnings';
162
import {
163
- current as currentDebugFiberInDEV,
163
isRendering as ReactCurrentDebugFiberIsRenderingInDEV,
164
resetCurrentFiber as resetCurrentDebugFiberInDEV,
165
setCurrentFiber as setCurrentDebugFiberInDEV,
@@ -2736,38 +2735,20 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) {
2735
didWarnStateUpdateForUnmountedComponent = new Set([componentName]);
2736
}
2737
2739
- // If we are currently flushing passive effects, change the warning text.
2738
if (isFlushingPassiveEffects) {
2739
+ // Do not warn if we are currently flushing passive effects!
2740
+ //
2741
// React can't directly detect a memory leak, but there are some clues that warn about one.
2742
// One of these clues is when an unmounted React component tries to update its state.
2743
// For example, if a component forgets to remove an event listener when unmounting,
2744
- // that listener may be called later and try to update state, at which point React would warn about the potential leak.
2745
- //
2746
- // Warning signals like this are more useful if they're strong.
2747
- // For this reason, it's good to always avoid updating state from inside of an effect's cleanup function.
2748
- // Even when you know there is no potential leak, React has no way to know and so it will warn anyway.
2749
- // In most cases we suggest moving state updates to the useEffect() body instead.
2750
- // This works so long as the component is updating its own state (or the state of a descendant).
2744
+ // that listener may be called later and try to update state,
2745
+ // at which point React would warn about the potential leak.
2746
//
2752
- // However this will not work when a component updates its parent state in a cleanup function.
2753
- // If such a component is unmounted but its parent remains mounted, the state will be out of sync.
2754
- // For this reason, we avoid showing the warning if a component is updating an ancestor.
2755
- let currentTargetFiber = fiber;
2756
- while (currentTargetFiber !== null) {
2757
- // currentDebugFiberInDEV owns the effect destroy function currently being invoked.
2758
- if (
2759
- currentDebugFiberInDEV === currentTargetFiber ||
2760
- currentDebugFiberInDEV === currentTargetFiber.alternate
2761
- ) {
2762
- console.error(
2763
- "Can't perform a React state update from within a useEffect cleanup function. " +
2764
- 'To fix, move state updates to the useEffect() body.%s',
2765
- getStackByFiberInDevAndProd(((currentDebugFiberInDEV: any): Fiber)),
2766
- );
2767
- break;
2768
- }
2769
- currentTargetFiber = currentTargetFiber.return;
2770
- }
2747
+ // Warning signals are the most useful when they're strong.
2748
+ // (So we should avoid false positive warnings.)
2749
+ // Updating state from within an effect cleanup function is sometimes a necessary pattern, e.g.:
2750
+ // 1. Updating an ancestor that a component had registered itself with on mount.
2751
+ // 2. Resetting state when a component is hidden after going offscreen.
2752
} else {
2753
console.error(
2754
"Can't perform a React state update on an unmounted component. This " +
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+46
-16
@@ -1307,7 +1307,49 @@ describe('ReactHooksWithNoopRenderer', () => {
1307
});
1308
});
1309
1310
- it('shows a warning when a component updates its own state from within passive unmount function', () => {
1310
+ it('still warns if there are updates after pending passive unmount effects have been flushed', () => {
1311
+ let updaterFunction;
1312
+
1313
+ function Component() {
1314
+ Scheduler.unstable_yieldValue('Component');
1315
+ const [state, setState] = React.useState(false);
1316
+ updaterFunction = setState;
1317
+ React.useEffect(() => {
1318
+ Scheduler.unstable_yieldValue('passive create');
1319
+ return () => {
1320
+ Scheduler.unstable_yieldValue('passive destroy');
1321
+ };
1322
+ }, []);
1323
+ return state;
1324
+ }
1325
+
1326
+ act(() => {
1327
+ ReactNoop.renderToRootWithID(<Component />, 'root', () =>
1328
+ Scheduler.unstable_yieldValue('Sync effect'),
1329
+ );
1330
+ });
1331
+ expect(Scheduler).toHaveYielded([
1332
+ 'Component',
1333
+ 'Sync effect',
1334
+ 'passive create',
1335
+ ]);
1336
+
1337
+ ReactNoop.unmountRootWithID('root');
1338
+ expect(Scheduler).toFlushAndYield(['passive destroy']);
1339
+
1340
+ act(() => {
1341
+ expect(() => {
1342
+ updaterFunction(true);
1343
+ }).toErrorDev(
1344
+ "Warning: Can't perform a React state update on an unmounted component. " +
1345
+ 'This is a no-op, but it indicates a memory leak in your application. ' +
1346
+ 'To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.\n' +
1347
+ ' in Component (at **)',
1348
+ );
1349
+ });
1350
+ });
1351
+
1352
+ it('does not show a warning when a component updates its own state from within passive unmount function', () => {
1353
function Component() {
1354
Scheduler.unstable_yieldValue('Component');
1355
const [didLoad, setDidLoad] = React.useState(false);
@@ -1333,17 +1375,11 @@ describe('ReactHooksWithNoopRenderer', () => {
1375
1376
// Unmount but don't process pending passive destroy function
1377
ReactNoop.unmountRootWithID('root');
1336
- expect(() => {
1337
- expect(Scheduler).toFlushAndYield(['passive destroy']);
1338
- }).toErrorDev(
1339
- "Warning: Can't perform a React state update from within a useEffect cleanup function. " +
1340
- 'To fix, move state updates to the useEffect() body.\n' +
1341
- ' in Component (at **)',
1342
- );
1378
+ expect(Scheduler).toFlushAndYield(['passive destroy']);
1379
});
1380
});
1381
1346
- it('shows a warning when a component updates a childs state from within passive unmount function', () => {
1382
+ it('does not show a warning when a component updates a childs state from within passive unmount function', () => {
1383
function Parent() {
1384
Scheduler.unstable_yieldValue('Parent');
1385
const updaterRef = React.useRef(null);
@@ -1378,13 +1414,7 @@ describe('ReactHooksWithNoopRenderer', () => {
1414
1415
// Unmount but don't process pending passive destroy function
1416
ReactNoop.unmountRootWithID('root');
1381
- expect(() => {
1382
- expect(Scheduler).toFlushAndYield(['Parent passive destroy']);
1383
- }).toErrorDev(
1384
- "Warning: Can't perform a React state update from within a useEffect cleanup function. " +
1385
- 'To fix, move state updates to the useEffect() body.\n' +
1386
- ' in Parent (at **)',
1387
- );
1417
+ expect(Scheduler).toFlushAndYield(['Parent passive destroy']);
1418
});
1419
});
1420
scripts/jest/matchers/toWarnDev.js
+1
-1
@@ -1,6 +1,6 @@
1
'use strict';
2
3
-const jestDiff = require('jest-diff').default;
3
+const jestDiff = require('jest-diff');
4
const util = require('util');
5
const shouldIgnoreConsoleError = require('../shouldIgnoreConsoleError');
6