diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index c8bad6581c6d8..ec80c6c91c451 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -161,6 +161,7 @@ import { import getComponentName from 'shared/getComponentName'; import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import { + current as currentDebugFiberInDEV, isRendering as ReactCurrentDebugFiberIsRenderingInDEV, resetCurrentFiber as resetCurrentDebugFiberInDEV, setCurrentFiber as setCurrentDebugFiberInDEV, @@ -2822,14 +2823,36 @@ function warnAboutUpdateOnUnmountedFiberInDEV(fiber) { // If we are currently flushing passive effects, change the warning text. if (isFlushingPassiveEffects) { - console.error( - "Can't perform a React state update from within a useEffect cleanup function. " + - 'To fix, move state updates to the useEffect() body in %s.%s', - tag === ClassComponent - ? 'the componentWillUnmount method' - : 'a useEffect cleanup function', - getStackByFiberInDevAndProd(fiber), - ); + // 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). + // + // 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; + } } 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 e24008eb54d2d..dec9258996237 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -1305,7 +1305,7 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); - it('shows a unique warning for state updates from within passive unmount function', () => { + it('shows 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); @@ -1334,10 +1334,90 @@ describe('ReactHooksWithNoopRenderer', () => { expect(() => { expect(Scheduler).toFlushAndYield(['passive destroy']); }).toErrorDev( - "Warning: Can't perform a React state update from within a useEffect cleanup function.", + "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 **)', ); }); }); + + it('shows 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); + React.useEffect(() => { + Scheduler.unstable_yieldValue('Parent passive create'); + return () => { + updaterRef.current(true); + Scheduler.unstable_yieldValue('Parent passive destroy'); + }; + }, []); + return ; + } + + function Child({updaterRef}) { + Scheduler.unstable_yieldValue('Child'); + const [state, setState] = React.useState(false); + React.useEffect(() => { + Scheduler.unstable_yieldValue('Child passive create'); + updaterRef.current = setState; + }, []); + return state; + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root'); + expect(Scheduler).toFlushAndYieldThrough([ + 'Parent', + 'Child', + 'Child passive create', + 'Parent passive create', + ]); + + // 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 **)', + ); + }); + }); + + it('does not show a warning when a component updates a parents state from within passive unmount function', () => { + function Parent() { + const [state, setState] = React.useState(false); + Scheduler.unstable_yieldValue('Parent'); + return ; + } + + function Child({setState, state}) { + Scheduler.unstable_yieldValue('Child'); + React.useEffect(() => { + Scheduler.unstable_yieldValue('Child passive create'); + return () => { + Scheduler.unstable_yieldValue('Child passive destroy'); + setState(true); + }; + }, []); + return state; + } + + act(() => { + ReactNoop.renderToRootWithID(, 'root'); + expect(Scheduler).toFlushAndYieldThrough([ + 'Parent', + 'Child', + 'Child passive create', + ]); + + // Unmount but don't process pending passive destroy function + ReactNoop.unmountRootWithID('root'); + expect(Scheduler).toFlushAndYield(['Child passive destroy']); + }); + }); } it('updates have async priority', () => {