From 5ee0efe832769b845c98a41df8736e09a4f1061f Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 1 Apr 2020 10:49:24 -0700 Subject: [PATCH] Remove state update warning for passive effect cleanup functions (#18453) --- .../src/ReactFiberWorkLoop.js | 37 +++-------- ...eactHooksWithNoopRenderer-test.internal.js | 62 ++++++++++++++----- scripts/jest/matchers/toWarnDev.js | 2 +- 3 files changed, 56 insertions(+), 45 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 276795d2cee93..b3584bfff8f97 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -160,7 +160,6 @@ import { import getComponentName from 'shared/getComponentName'; import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import { - current as currentDebugFiberInDEV, isRendering as ReactCurrentDebugFiberIsRenderingInDEV, resetCurrentFiber as resetCurrentDebugFiberInDEV, setCurrentFiber as setCurrentDebugFiberInDEV, @@ -2736,38 +2735,20 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) { didWarnStateUpdateForUnmountedComponent = new Set([componentName]); } - // If we are currently flushing passive effects, change the warning text. if (isFlushingPassiveEffects) { + // Do not warn if we are currently flushing passive effects! + // // 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. - // This works so long as the component is updating its own state (or the state of a descendant). + // that listener may be called later and try to update state, + // at which point React would warn about the potential leak. // - // 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 out of sync. - // For this reason, we avoid showing the warning if a component is updating an ancestor. - let currentTargetFiber = fiber; - while (currentTargetFiber !== null) { - // currentDebugFiberInDEV owns the effect destroy function currently being invoked. - if ( - currentDebugFiberInDEV === currentTargetFiber || - currentDebugFiberInDEV === currentTargetFiber.alternate - ) { - console.error( - "Can't perform a React state update from within a useEffect cleanup function. " + - 'To fix, move state updates to the useEffect() body.%s', - getStackByFiberInDevAndProd(((currentDebugFiberInDEV: any): Fiber)), - ); - break; - } - currentTargetFiber = currentTargetFiber.return; - } + // Warning signals are the most useful when they're strong. + // (So we should avoid false positive warnings.) + // Updating state from within an effect cleanup function is sometimes a necessary pattern, e.g.: + // 1. Updating an ancestor that a component had registered itself with on mount. + // 2. Resetting state when a component is hidden after going offscreen. } else { console.error( "Can't perform a React state update on an unmounted component. This " + diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index a04578381d891..3b037531ceecb 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1307,7 +1307,49 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); - it('shows a warning when a component updates its own state from within passive unmount function', () => { + it('still warns if there are updates after pending passive unmount effects have been flushed', () => { + let updaterFunction; + + function Component() { + Scheduler.unstable_yieldValue('Component'); + const [state, setState] = React.useState(false); + updaterFunction = setState; + React.useEffect(() => { + Scheduler.unstable_yieldValue('passive create'); + return () => { + Scheduler.unstable_yieldValue('passive destroy'); + }; + }, []); + return state; + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root', () => + Scheduler.unstable_yieldValue('Sync effect'), + ); + }); + expect(Scheduler).toHaveYielded([ + 'Component', + 'Sync effect', + 'passive create', + ]); + + ReactNoop.unmountRootWithID('root'); + expect(Scheduler).toFlushAndYield(['passive destroy']); + + act(() => { + expect(() => { + updaterFunction(true); + }).toErrorDev( + "Warning: Can't perform a React state update on an unmounted component. " + + 'This is a no-op, but it indicates a memory leak in your application. ' + + 'To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.\n' + + ' in Component (at **)', + ); + }); + }); + + it('does not show a warning when a component updates its own state from within passive unmount function', () => { function Component() { Scheduler.unstable_yieldValue('Component'); const [didLoad, setDidLoad] = React.useState(false); @@ -1333,17 +1375,11 @@ describe('ReactHooksWithNoopRenderer', () => { // Unmount but don't process pending passive destroy function ReactNoop.unmountRootWithID('root'); - expect(() => { - expect(Scheduler).toFlushAndYield(['passive destroy']); - }).toErrorDev( - "Warning: Can't perform a React state update from within a useEffect cleanup function. " + - 'To fix, move state updates to the useEffect() body.\n' + - ' in Component (at **)', - ); + expect(Scheduler).toFlushAndYield(['passive destroy']); }); }); - it('shows a warning when a component updates a childs state from within passive unmount function', () => { + it('does not show a warning when a component updates a childs state from within passive unmount function', () => { function Parent() { Scheduler.unstable_yieldValue('Parent'); const updaterRef = React.useRef(null); @@ -1378,13 +1414,7 @@ describe('ReactHooksWithNoopRenderer', () => { // Unmount but don't process pending passive destroy function ReactNoop.unmountRootWithID('root'); - expect(() => { - expect(Scheduler).toFlushAndYield(['Parent passive destroy']); - }).toErrorDev( - "Warning: Can't perform a React state update from within a useEffect cleanup function. " + - 'To fix, move state updates to the useEffect() body.\n' + - ' in Parent (at **)', - ); + expect(Scheduler).toFlushAndYield(['Parent passive destroy']); }); }); diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 44ec4e56ed4a7..133b4f08a04c9 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -1,6 +1,6 @@ 'use strict'; -const jestDiff = require('jest-diff').default; +const jestDiff = require('jest-diff'); const util = require('util'); const shouldIgnoreConsoleError = require('../shouldIgnoreConsoleError');