warn if passive effects get queued outside of an act() call

While the code itself isn't much (it adds the warning to mountEffect() and updateEffect() in ReactFiberHooks), it does change a lot of our tests. We follow a bad-ish pattern here, which is doing asserts inside act() scopes, but it makes sense for *us* because we're testing intermediate states, and we're manually flush/yield what we need in these tests.

This commit has one last failing test. Working on it.

packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js

@@ -146,7 +146,10 @@ describe('ReactHooksInspectionIntegration', () => {
         </div>
       );
     }
-    let renderer = ReactTestRenderer.create(<Foo prop="prop" />);
+    let renderer;
+    act(() => {
+      renderer = ReactTestRenderer.create(<Foo prop="prop" />);
+    });
 
     let childFiber = renderer.root.findByType(Foo)._currentFiber();
 

packages/react-dom/src/__tests__/ReactDOMHooks-test.js

@@ -11,6 +11,7 @@
 
 let React;
 let ReactDOM;
+let act;
 let Scheduler;
 
 describe('ReactDOMHooks', () => {
@@ -21,6 +22,7 @@ describe('ReactDOMHooks', () => {
 
     React = require('react');
     ReactDOM = require('react-dom');
+    act = require('react-dom/test-utils').act;
     Scheduler = require('scheduler');
 
     container = document.createElement('div');
@@ -53,23 +55,33 @@ describe('ReactDOMHooks', () => {
       return 3 * n;
     }
 
-    ReactDOM.render(<Example1 n={1} />, container);
-    expect(container.textContent).toBe('1');
-    expect(container2.textContent).toBe('');
-    expect(container3.textContent).toBe('');
-    Scheduler.flushAll();
-    expect(container.textContent).toBe('1');
-    expect(container2.textContent).toBe('2');
-    expect(container3.textContent).toBe('3');
-
-    ReactDOM.render(<Example1 n={2} />, container);
-    expect(container.textContent).toBe('2');
-    expect(container2.textContent).toBe('2'); // Not flushed yet
-    expect(container3.textContent).toBe('3'); // Not flushed yet
-    Scheduler.flushAll();
-    expect(container.textContent).toBe('2');
-    expect(container2.textContent).toBe('4');
-    expect(container3.textContent).toBe('6');
+    // we explicitly catch the missing act() warnings
+    // to simulate this tricky repro
+    // todo - is this ok?
+    expect(() => {
+      ReactDOM.render(<Example1 n={1} />, container);
+      expect(container.textContent).toBe('1');
+      expect(container2.textContent).toBe('');
+      expect(container3.textContent).toBe('');
+      Scheduler.flushAll();
+      expect(container.textContent).toBe('1');
+      expect(container2.textContent).toBe('2');
+      expect(container3.textContent).toBe('3');
+
+      ReactDOM.render(<Example1 n={2} />, container);
+      expect(container.textContent).toBe('2');
+      expect(container2.textContent).toBe('2'); // Not flushed yet
+      expect(container3.textContent).toBe('3'); // Not flushed yet
+      Scheduler.flushAll();
+      expect(container.textContent).toBe('2');
+      expect(container2.textContent).toBe('4');
+      expect(container3.textContent).toBe('6');
+    }).toWarnDev([
+      'Your test just caused an effect from Example1',
+      'Your test just caused an effect from Example2',
+      'Your test just caused an effect from Example1',
+      'Your test just caused an effect from Example2',
+    ]);
   });
 
   it('should not bail out when an update is scheduled from within an event handler', () => {

packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js

@@ -17,6 +17,7 @@ let React;
 let ReactFeatureFlags;
 let ReactDOM;
 let ReactDOMServer;
+let act;
 let useState;
 let useReducer;
 let useEffect;
@@ -41,6 +42,7 @@ function initModules() {
   React = require('react');
   ReactDOM = require('react-dom');
   ReactDOMServer = require('react-dom/server');
+  act = require('react-dom/test-utils').act;
   useState = React.useState;
   useReducer = React.useReducer;
   useEffect = React.useEffect;
@@ -546,10 +548,12 @@ describe('ReactDOMServerHooks', () => {
         });
         return <Text text={'Count: ' + props.count} />;
       }
-      const domNode = await render(<Counter count={0} />);
-      expect(clearYields()).toEqual(['Count: 0']);
-      expect(domNode.tagName).toEqual('SPAN');
-      expect(domNode.textContent).toEqual('Count: 0');
+      await act(async () => {
+        const domNode = await render(<Counter count={0} />);
+        expect(clearYields()).toEqual(['Count: 0']);
+        expect(domNode.tagName).toEqual('SPAN');
+        expect(domNode.textContent).toEqual('Count: 0');
+      });
     });
   });
 

packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js

@@ -12,6 +12,7 @@
 let PropTypes;
 let React;
 let ReactDOM;
+let act;
 let ReactFeatureFlags;
 let Scheduler;
 
@@ -45,6 +46,7 @@ describe('ReactErrorBoundaries', () => {
     ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
     ReactDOM = require('react-dom');
     React = require('react');
+    act = require('react-dom/test-utils').act;
     Scheduler = require('scheduler');
 
     log = [];
@@ -1835,25 +1837,26 @@ describe('ReactErrorBoundaries', () => {
 
   it('catches errors in useEffect', () => {
     const container = document.createElement('div');
-    ReactDOM.render(
-      <ErrorBoundary>
-        <BrokenUseEffect>Initial value</BrokenUseEffect>
-      </ErrorBoundary>,
-      container,
-    );
-    expect(log).toEqual([
-      'ErrorBoundary constructor',
-      'ErrorBoundary componentWillMount',
-      'ErrorBoundary render success',
-      'BrokenUseEffect render',
-      'ErrorBoundary componentDidMount',
-    ]);
-
-    expect(container.firstChild.textContent).toBe('Initial value');
-    log.length = 0;
-
-    // Flush passive effects and handle the error
-    Scheduler.flushAll();
+    act(() => {
+      ReactDOM.render(
+        <ErrorBoundary>
+          <BrokenUseEffect>Initial value</BrokenUseEffect>
+        </ErrorBoundary>,
+        container,
+      );
+      expect(log).toEqual([
+        'ErrorBoundary constructor',
+        'ErrorBoundary componentWillMount',
+        'ErrorBoundary render success',
+        'BrokenUseEffect render',
+        'ErrorBoundary componentDidMount',
+      ]);
+
+      expect(container.firstChild.textContent).toBe('Initial value');
+      log.length = 0;
+    });
+
+    // verify flushed passive effects and handle the error
     expect(log).toEqual([
       'BrokenUseEffect useEffect [!]',
       // Handle the error

packages/react-dom/src/__tests__/ReactUpdates-test.js

@@ -12,6 +12,7 @@
 let React;
 let ReactDOM;
 let ReactTestUtils;
+let act;
 let Scheduler;
 
 describe('ReactUpdates', () => {
@@ -20,6 +21,7 @@ describe('ReactUpdates', () => {
     React = require('react');
     ReactDOM = require('react-dom');
     ReactTestUtils = require('react-dom/test-utils');
+    act = ReactTestUtils.act;
     Scheduler = require('scheduler');
   });
 
@@ -1322,30 +1324,31 @@ describe('ReactUpdates', () => {
     }
 
     const root = ReactDOM.unstable_createRoot(container);
-    root.render(<Foo />);
-    if (__DEV__) {
-      expect(Scheduler).toFlushAndYieldThrough([
-        'Foo',
-        'Foo',
-        'Baz',
-        'Foo#effect',
-      ]);
-    } else {
-      expect(Scheduler).toFlushAndYieldThrough(['Foo', 'Baz', 'Foo#effect']);
-    }
-
-    const hiddenDiv = container.firstChild.firstChild;
-    expect(hiddenDiv.hidden).toBe(true);
-    expect(hiddenDiv.innerHTML).toBe('');
-
-    // Run offscreen update
-    if (__DEV__) {
-      expect(Scheduler).toFlushAndYield(['Bar', 'Bar']);
-    } else {
-      expect(Scheduler).toFlushAndYield(['Bar']);
-    }
-    expect(hiddenDiv.hidden).toBe(true);
-    expect(hiddenDiv.innerHTML).toBe('<p>bar 0</p>');
+    let hiddenDiv;
+    act(() => {
+      root.render(<Foo />);
+      if (__DEV__) {
+        expect(Scheduler).toFlushAndYieldThrough([
+          'Foo',
+          'Foo',
+          'Baz',
+          'Foo#effect',
+        ]);
+      } else {
+        expect(Scheduler).toFlushAndYieldThrough(['Foo', 'Baz', 'Foo#effect']);
+      }
+      hiddenDiv = container.firstChild.firstChild;
+      expect(hiddenDiv.hidden).toBe(true);
+      expect(hiddenDiv.innerHTML).toBe('');
+      // Run offscreen update
+      if (__DEV__) {
+        expect(Scheduler).toFlushAndYield(['Bar', 'Bar']);
+      } else {
+        expect(Scheduler).toFlushAndYield(['Bar']);
+      }
+      expect(hiddenDiv.hidden).toBe(true);
+      expect(hiddenDiv.innerHTML).toBe('<p>bar 0</p>');
+    });
 
     ReactDOM.flushSync(() => {
       setCounter(1);
@@ -1618,8 +1621,11 @@ describe('ReactUpdates', () => {
       let stack = null;
       let originalConsoleError = console.error;
       console.error = (e, s) => {
-        error = e;
-        stack = s;
+        // skip the missing act() error
+        if (e.slice(0, 40) !== 'Warning: Your test just caused an effect') {
+          error = e;
+          stack = s;
+        }
       };
       try {
         const container = document.createElement('div');
@@ -1651,7 +1657,9 @@ describe('ReactUpdates', () => {
       }
 
       const container = document.createElement('div');
-      ReactDOM.render(<Terminating />, container);
+      act(() => {
+        ReactDOM.render(<Terminating />, container);
+      });
 
       // Verify we can flush them asynchronously without warning
       for (let i = 0; i < LIMIT * 2; i++) {
@@ -1660,16 +1668,16 @@ describe('ReactUpdates', () => {
       expect(container.textContent).toBe('50');
 
       // Verify restarting from 0 doesn't cross the limit
-      expect(() => {
+      act(() => {
         _setStep(0);
-      }).toWarnDev(
-        'An update to Terminating inside a test was not wrapped in act',
-      );
-      expect(container.textContent).toBe('0');
-      for (let i = 0; i < LIMIT * 2; i++) {
+        // flush once to update the dom
         Scheduler.unstable_flushNumberOfYields(1);
-      }
-      expect(container.textContent).toBe('50');
+        expect(container.textContent).toBe('0');
+        for (let i = 0; i < LIMIT * 2; i++) {
+          Scheduler.unstable_flushNumberOfYields(1);
+        }
+        expect(container.textContent).toBe('50');
+      });
     });
 
     it('can have many updates inside useEffect without triggering a warning', () => {
@@ -1685,8 +1693,11 @@ describe('ReactUpdates', () => {
       }
 
       const container = document.createElement('div');
-      ReactDOM.render(<Terminating />, container);
-      expect(Scheduler).toFlushAndYield(['Done']);
+      act(() => {
+        ReactDOM.render(<Terminating />, container);
+      });
+
+      expect(Scheduler).toHaveYielded(['Done']);
       expect(container.textContent).toBe('1000');
     });
   }

packages/react-reconciler/src/ReactFiberHooks.js

@@ -34,6 +34,7 @@ import {
   computeExpirationForFiber,
   flushPassiveEffects,
   requestCurrentTime,
+  warnIfNotCurrentlyActingEffectsInDEV,
   warnIfNotCurrentlyActingUpdatesInDev,
   warnIfNotScopedWithMatchingAct,
   markRenderEventTimeAndConfig,
@@ -892,6 +893,14 @@ function mountEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
+  if (__DEV__) {
+    // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
+    if ('undefined' !== typeof jest) {
+      warnIfNotCurrentlyActingEffectsInDEV(
+        ((currentlyRenderingFiber: any): Fiber),
+      );
+    }
+  }
   return mountEffectImpl(
     UpdateEffect | PassiveEffect,
     UnmountPassive | MountPassive,
@@ -904,6 +913,14 @@ function updateEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
+  if (__DEV__) {
+    // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
+    if ('undefined' !== typeof jest) {
+      warnIfNotCurrentlyActingEffectsInDEV(
+        ((currentlyRenderingFiber: any): Fiber),
+      );
+    }
+  }
   return updateEffectImpl(
     UpdateEffect | PassiveEffect,
     UnmountPassive | MountPassive,

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -2429,6 +2429,29 @@ export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
   }
 }
 
+export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
+  if (__DEV__) {
+    if (ReactCurrentActingRendererSigil.current !== ReactActingRendererSigil) {
+      warningWithoutStack(
+        false,
+        'Your test just caused an effect from %s, but was not wrapped in act(...).\n\n' +
+          'When testing, code that causes React state updates should be ' +
+          'wrapped into act(...):\n\n' +
+          'act(() => {\n' +
+          '  /* fire events that update state */\n' +
+          '});\n' +
+          '/* assert on the output */\n\n' +
+          "This ensures that you're testing the behavior the user would see " +
+          'in the browser.' +
+          ' Learn more at https://fb.me/react-wrap-tests-with-act' +
+          '%s',
+        getComponentName(fiber.type),
+        getStackByFiberInDevAndProd(fiber),
+      );
+    }
+  }
+}
+
 function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
   if (__DEV__) {
     if (