Don't show destroy function state update warning when updating ancestors (facebook#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.

Author: Brian Vaughn

Diff for: 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 for: 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 <Child updaterRef={updaterRef} />;
+        }
+
+        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(<Parent />, '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 <Child setState={setState} state={state} />;
+        }
+
+        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(<Parent />, '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', () => {