Warn for update on different component in render

This warning already exists for class components, but not for functions. It does not apply to render phase updates to the same component, which have special semantics that we do support.
facebook · Oct 15, 2019 · bd8fbdb · bd8fbdb
1 parent a8c6a1b
commit bd8fbdb
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 37 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -372,7 +372,7 @@ export function scheduleUpdateOnFiber(
   expirationTime: ExpirationTime,
 ) {
   checkForNestedUpdates();
-  warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber);
+  warnAboutRenderPhaseUpdatesInDEV(fiber);
 
   const root = markUpdateTimeFromFiberToRoot(fiber, expirationTime);
   if (root === null) {
@@ -2663,32 +2663,45 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) {
 
 let didWarnAboutUpdateInRender = false;
 let didWarnAboutUpdateInGetChildContext = false;
-function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) {
-  if (__DEV__) {
-    if (fiber.tag === ClassComponent) {
-      switch (ReactCurrentDebugFiberPhaseInDEV) {
-        case 'getChildContext':
-          if (didWarnAboutUpdateInGetChildContext) {
-            return;
-          }
-          warningWithoutStack(
-            false,
-            'setState(...): Cannot call setState() inside getChildContext()',
-          );
-          didWarnAboutUpdateInGetChildContext = true;
-          break;
-        case 'render':
-          if (didWarnAboutUpdateInRender) {
-            return;
-          }
-          warningWithoutStack(
-            false,
-            'Cannot update during an existing state transition (such as ' +
-              'within `render`). Render methods should be a pure function of ' +
-              'props and state.',
-          );
-          didWarnAboutUpdateInRender = true;
-          break;
+function warnAboutRenderPhaseUpdatesInDEV(fiber) {
+  if (__DEV__ && (executionContext & RenderContext) !== NoContext) {
+    switch (fiber.tag) {
+      case FunctionComponent:
+      case ForwardRef:
+      case SimpleMemoComponent: {
+        warning(
+          false,
+          'Cannot update a component from inside the function body of a ' +
+            'different component.',
+        );
+        break;
+      }
+      case ClassComponent: {
+        switch (ReactCurrentDebugFiberPhaseInDEV) {
+          case 'getChildContext':
+            if (didWarnAboutUpdateInGetChildContext) {
+              return;
+            }
+            warningWithoutStack(
+              false,
+              'setState(...): Cannot call setState() inside getChildContext()',
+            );
+            didWarnAboutUpdateInGetChildContext = true;
+            break;
+          case 'render':
+            if (didWarnAboutUpdateInRender) {
+              return;
+            }
+            warningWithoutStack(
+              false,
+              'Cannot update during an existing state transition (such as ' +
+                'within `render`). Render methods should be a pure function of ' +
+                'props and state.',
+            );
+            didWarnAboutUpdateInRender = true;
+            break;
+        }
+        break;
       }
     }
   }

diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
@@ -1082,7 +1082,10 @@ describe('ReactHooks', () => {
           <Cls />
         </>,
       ),
-    ).toWarnDev(['Context can only be read while React is rendering']);
+    ).toWarnDev([
+      'Context can only be read while React is rendering',
+      'Cannot update a component from inside the function body of a different component.',
+    ]);
   });
 
   it('warns when calling hooks inside useReducer', () => {
@@ -1732,8 +1735,9 @@ describe('ReactHooks', () => {
   });
 
   // Regression test for #14674
-  it('does not swallow original error when updating another component in render phase', () => {
+  it('does not swallow original error when updating another component in render phase', async () => {
     let {useState} = React;
+    spyOnDev(console, 'error');
 
     let _setState;
     function A() {
@@ -1743,22 +1747,29 @@ describe('ReactHooks', () => {
     }
 
     function B() {
-      act(() =>
-        _setState(() => {
-          throw new Error('Hello');
-        }),
-      );
+      _setState(() => {
+        throw new Error('Hello');
+      });
       return null;
     }
 
-    expect(() =>
+    await act(async () => {
       ReactTestRenderer.create(
         <>
           <A />
           <B />
         </>,
-      ),
-    ).toThrow('Hello');
+      );
+      expect(() => Scheduler.unstable_flushAll()).toThrow('Hello');
+    });
+
+    if (__DEV__) {
+      expect(console.error).toHaveBeenCalledTimes(2);
+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'Warning: Cannot update a component from inside the function body ' +
+          'of a different component.%s',
+      );
+    }
   });
 
   // Regression test for https://github.com/facebook/react/issues/15057

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -491,6 +491,50 @@ describe('ReactHooksWithNoopRenderer', () => {
       ]);
       expect(ReactNoop.getChildren()).toEqual([span(22)]);
     });
+
+    it('warns about render phase update on a different component', async () => {
+      let setStep;
+      function Foo() {
+        const [step, _setStep] = useState(0);
+        setStep = _setStep;
+        return <Text text={`Foo [${step}]`} />;
+      }
+
+      function Bar({triggerUpdate}) {
+        if (triggerUpdate) {
+          setStep(1);
+        }
+        return <Text text="Bar" />;
+      }
+
+      const root = ReactNoop.createRoot();
+
+      await ReactNoop.act(async () => {
+        root.render(
+          <>
+            <Foo />
+            <Bar />
+          </>,
+        );
+      });
+      expect(Scheduler).toHaveYielded(['Foo [0]', 'Bar']);
+
+      // Bar will update Foo during its render phase. React should warn.
+      await ReactNoop.act(async () => {
+        root.render(
+          <>
+            <Foo />
+            <Bar triggerUpdate={true} />
+          </>,
+        );
+        expect(() =>
+          expect(Scheduler).toFlushAndYield(['Foo [0]', 'Bar', 'Foo [1]']),
+        ).toWarnDev([
+          'Cannot update a component from inside the function body of a ' +
+            'different component.',
+        ]);
+      });
+    });
   });
 
   describe('useReducer', () => {