Support disabling spurious act warnings with a global environment flag (

facebook#22561) * Extract `act` environment check into function `act` checks the environment to determine whether to fire a warning. We're changing how this check works in React 18. As a first step, this refactors the logic into a single function. No behavior changes yet. * Use IS_REACT_ACT_ENVIRONMENT to disable warnings If `IS_REACT_ACT_ENVIRONMENT` is set to `false`, we will suppress any `act` warnings. Otherwise, the behavior of `act` is the same as in React 17: if `jest` is defined, it warns. In concurrent mode, the plan is to remove the `jest` check and only warn if `IS_REACT_ACT_ENVIRONMENT` is true. I have not implemented that part yet.
KamranAsif · Nov 4, 2021 · b81e99b · b81e99b
1 parent d69561a
commit b81e99b
Show file tree

Hide file tree

Showing 16 changed files with 142 additions and 60 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -279,5 +279,6 @@ module.exports = {
     __VARIANT__: true,
     gate: true,
     trustedTypes: true,
+    IS_REACT_ACT_ENVIRONMENT: true,
   },
 };
diff --git a/packages/jest-react/src/internalAct.js b/packages/jest-react/src/internalAct.js
@@ -18,9 +18,7 @@ import type {Thenable} from 'shared/ReactTypes';
 
 import * as Scheduler from 'scheduler/unstable_mock';
 
-import ReactSharedInternals from 'shared/ReactSharedInternals';
 import enqueueTask from 'shared/enqueueTask';
-const {ReactCurrentActQueue} = ReactSharedInternals;
 
 let actingUpdatesScopeDepth = 0;
 
@@ -37,15 +35,18 @@ export function act(scope: () => Thenable<mixed> | void) {
     );
   }
 
+  const previousIsActEnvironment = global.IS_REACT_ACT_ENVIRONMENT;
   const previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
   actingUpdatesScopeDepth++;
   if (__DEV__ && actingUpdatesScopeDepth === 1) {
-    ReactCurrentActQueue.disableActWarning = true;
+    // Because this is not the "real" `act`, we set this to `false` so React
+    // knows not to fire `act` warnings.
+    global.IS_REACT_ACT_ENVIRONMENT = false;
   }
 
   const unwind = () => {
     if (__DEV__ && actingUpdatesScopeDepth === 1) {
-      ReactCurrentActQueue.disableActWarning = false;
+      global.IS_REACT_ACT_ENVIRONMENT = previousIsActEnvironment;
     }
     actingUpdatesScopeDepth--;
 

diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
@@ -273,6 +273,40 @@ function runActTests(label, render, unmount, rerender) {
         ]);
       });
 
+      // @gate __DEV__
+      it('does not warn if IS_REACT_ACT_ENVIRONMENT is set to false', () => {
+        let setState;
+        function App() {
+          const [state, _setState] = React.useState(0);
+          setState = _setState;
+          return state;
+        }
+
+        act(() => {
+          render(<App />, container);
+        });
+
+        // First show that it does warn
+        expect(() => setState(1)).toErrorDev(
+          'An update to App inside a test was not wrapped in act(...)',
+        );
+
+        // Now do the same thing again, but disable with the environment flag
+        const prevIsActEnvironment = global.IS_REACT_ACT_ENVIRONMENT;
+        global.IS_REACT_ACT_ENVIRONMENT = false;
+        try {
+          setState(2);
+        } finally {
+          global.IS_REACT_ACT_ENVIRONMENT = prevIsActEnvironment;
+        }
+
+        // When the flag is restored to its previous value, it should start
+        // warning again. This shows that React reads the flag each time.
+        expect(() => setState(3)).toErrorDev(
+          'An update to App inside a test was not wrapped in act(...)',
+        );
+      });
+
       describe('fake timers', () => {
         beforeEach(() => {
           jest.useFakeTimers();

diff --git a/packages/react-reconciler/src/ReactFiberAct.new.js b/packages/react-reconciler/src/ReactFiberAct.new.js
@@ -0,0 +1,35 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+
+import type {Fiber} from './ReactFiber.new';
+import {warnsIfNotActing} from './ReactFiberHostConfig';
+
+export function isActEnvironment(fiber: Fiber) {
+  if (__DEV__) {
+    const isReactActEnvironmentGlobal =
+      // $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
+      typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
+        ? IS_REACT_ACT_ENVIRONMENT
+        : undefined;
+
+    // TODO: Only check `jest` in legacy mode. In concurrent mode, this
+    // heuristic is replaced by IS_REACT_ACT_ENVIRONMENT.
+    // $FlowExpectedError - Flow doesn't know about jest
+    const jestIsDefined = typeof jest !== 'undefined';
+    return (
+      warnsIfNotActing &&
+      jestIsDefined &&
+      // Legacy mode assumes an act environment whenever `jest` is defined, but
+      // you can still turn off spurious warnings by setting
+      // IS_REACT_ACT_ENVIRONMENT explicitly to false.
+      isReactActEnvironmentGlobal !== false
+    );
+  }
+  return false;
+}
diff --git a/packages/react-reconciler/src/ReactFiberAct.old.js b/packages/react-reconciler/src/ReactFiberAct.old.js
@@ -0,0 +1,35 @@
+/**
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+
+import type {Fiber} from './ReactFiber.old';
+import {warnsIfNotActing} from './ReactFiberHostConfig';
+
+export function isActEnvironment(fiber: Fiber) {
+  if (__DEV__) {
+    const isReactActEnvironmentGlobal =
+      // $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
+      typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
+        ? IS_REACT_ACT_ENVIRONMENT
+        : undefined;
+
+    // TODO: Only check `jest` in legacy mode. In concurrent mode, this
+    // heuristic is replaced by IS_REACT_ACT_ENVIRONMENT.
+    // $FlowExpectedError - Flow doesn't know about jest
+    const jestIsDefined = typeof jest !== 'undefined';
+    return (
+      warnsIfNotActing &&
+      jestIsDefined &&
+      // Legacy mode assumes an act environment whenever `jest` is defined, but
+      // you can still turn off spurious warnings by setting
+      // IS_REACT_ACT_ENVIRONMENT explicitly to false.
+      isReactActEnvironmentGlobal !== false
+    );
+  }
+  return false;
+}
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -118,6 +118,7 @@ import {
 } from './ReactUpdateQueue.new';
 import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new';
 import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';
+import {isActEnvironment} from './ReactFiberAct.new';
 
 const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
 
@@ -1678,8 +1679,7 @@ function mountEffect(
   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) {
+    if (isActEnvironment(currentlyRenderingFiber)) {
       warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
     }
   }
@@ -1709,8 +1709,7 @@ function updateEffect(
   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) {
+    if (isActEnvironment(currentlyRenderingFiber)) {
       warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
     }
   }
@@ -2193,8 +2192,7 @@ function dispatchReducerAction<S, A>(
     enqueueUpdate(fiber, queue, update, lane);
 
     if (__DEV__) {
-      // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
-      if ('undefined' !== typeof jest) {
+      if (isActEnvironment(fiber)) {
         warnIfNotCurrentlyActingUpdatesInDev(fiber);
       }
     }
@@ -2279,8 +2277,7 @@ function dispatchSetState<S, A>(
       }
     }
     if (__DEV__) {
-      // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
-      if ('undefined' !== typeof jest) {
+      if (isActEnvironment(fiber)) {
         warnIfNotCurrentlyActingUpdatesInDev(fiber);
       }
     }

diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -118,6 +118,7 @@ import {
 } from './ReactUpdateQueue.old';
 import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.old';
 import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';
+import {isActEnvironment} from './ReactFiberAct.old';
 
 const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
 
@@ -1678,8 +1679,7 @@ function mountEffect(
   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) {
+    if (isActEnvironment(currentlyRenderingFiber)) {
       warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
     }
   }
@@ -1709,8 +1709,7 @@ function updateEffect(
   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) {
+    if (isActEnvironment(currentlyRenderingFiber)) {
       warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
     }
   }
@@ -2193,8 +2192,7 @@ function dispatchReducerAction<S, A>(
     enqueueUpdate(fiber, queue, update, lane);
 
     if (__DEV__) {
-      // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
-      if ('undefined' !== typeof jest) {
+      if (isActEnvironment(fiber)) {
         warnIfNotCurrentlyActingUpdatesInDev(fiber);
       }
     }
@@ -2279,8 +2277,7 @@ function dispatchSetState<S, A>(
       }
     }
     if (__DEV__) {
-      // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
-      if ('undefined' !== typeof jest) {
+      if (isActEnvironment(fiber)) {
         warnIfNotCurrentlyActingUpdatesInDev(fiber);
       }
     }

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -84,7 +84,6 @@ import {
   scheduleTimeout,
   cancelTimeout,
   noTimeout,
-  warnsIfNotActing,
   afterActiveInstanceBlur,
   clearContainer,
   getCurrentEventPriority,
@@ -2816,15 +2815,8 @@ function shouldForceFlushFallbacksInDEV() {
 export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
   if (__DEV__) {
     if (
-      warnsIfNotActing === true &&
       (fiber.mode & StrictLegacyMode) !== NoMode &&
-      ReactCurrentActQueue.current === null &&
-      // Our internal tests use a custom implementation of `act` that works by
-      // mocking the Scheduler package. Disable the `act` warning.
-      // TODO: Maybe the warning should be disabled by default, and then turned
-      // on at the testing frameworks layer? Instead of what we do now, which
-      // is check if a `jest` global is defined.
-      ReactCurrentActQueue.disableActWarning === false
+      ReactCurrentActQueue.current === null
     ) {
       console.error(
         'An update to %s ran an effect, but was not wrapped in act(...).\n\n' +
@@ -2846,15 +2838,8 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
 function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
   if (__DEV__) {
     if (
-      warnsIfNotActing === true &&
       executionContext === NoContext &&
-      ReactCurrentActQueue.current === null &&
-      // Our internal tests use a custom implementation of `act` that works by
-      // mocking the Scheduler package. Disable the `act` warning.
-      // TODO: Maybe the warning should be disabled by default, and then turned
-      // on at the testing frameworks layer? Instead of what we do now, which
-      // is check if a `jest` global is defined.
-      ReactCurrentActQueue.disableActWarning === false
+      ReactCurrentActQueue.current === null
     ) {
       const previousFiber = ReactCurrentFiberCurrent;
       try {

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -84,7 +84,6 @@ import {
   scheduleTimeout,
   cancelTimeout,
   noTimeout,
-  warnsIfNotActing,
   afterActiveInstanceBlur,
   clearContainer,
   getCurrentEventPriority,
@@ -2816,15 +2815,8 @@ function shouldForceFlushFallbacksInDEV() {
 export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
   if (__DEV__) {
     if (
-      warnsIfNotActing === true &&
       (fiber.mode & StrictLegacyMode) !== NoMode &&
-      ReactCurrentActQueue.current === null &&
-      // Our internal tests use a custom implementation of `act` that works by
-      // mocking the Scheduler package. Disable the `act` warning.
-      // TODO: Maybe the warning should be disabled by default, and then turned
-      // on at the testing frameworks layer? Instead of what we do now, which
-      // is check if a `jest` global is defined.
-      ReactCurrentActQueue.disableActWarning === false
+      ReactCurrentActQueue.current === null
     ) {
       console.error(
         'An update to %s ran an effect, but was not wrapped in act(...).\n\n' +
@@ -2846,15 +2838,8 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void {
 function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
   if (__DEV__) {
     if (
-      warnsIfNotActing === true &&
       executionContext === NoContext &&
-      ReactCurrentActQueue.current === null &&
-      // Our internal tests use a custom implementation of `act` that works by
-      // mocking the Scheduler package. Disable the `act` warning.
-      // TODO: Maybe the warning should be disabled by default, and then turned
-      // on at the testing frameworks layer? Instead of what we do now, which
-      // is check if a `jest` global is defined.
-      ReactCurrentActQueue.disableActWarning === false
+      ReactCurrentActQueue.current === null
     ) {
       const previousFiber = ReactCurrentFiberCurrent;
       try {

diff --git a/packages/react/src/ReactCurrentActQueue.js b/packages/react/src/ReactCurrentActQueue.js
@@ -11,12 +11,6 @@ type RendererTask = boolean => RendererTask | null;
 
 const ReactCurrentActQueue = {
   current: (null: null | Array<RendererTask>),
-  // Our internal tests use a custom implementation of `act` that works by
-  // mocking the Scheduler package. Use this field to disable the `act` warning.
-  // TODO: Maybe the warning should be disabled by default, and then turned
-  // on at the testing frameworks layer? Instead of what we do now, which
-  // is check if a `jest` global is defined.
-  disableActWarning: (false: boolean),
 
   // Used to reproduce behavior of `batchedUpdates` in legacy mode.
   isBatchingLegacy: false,

diff --git a/scripts/rollup/validate/eslintrc.cjs.js b/scripts/rollup/validate/eslintrc.cjs.js
@@ -42,6 +42,9 @@ module.exports = {
     // jest
     expect: true,
     jest: true,
+
+    // act
+    IS_REACT_ACT_ENVIRONMENT: true,
   },
   parserOptions: {
     ecmaVersion: 5,

diff --git a/scripts/rollup/validate/eslintrc.cjs2015.js b/scripts/rollup/validate/eslintrc.cjs2015.js
@@ -42,6 +42,9 @@ module.exports = {
     // jest
     expect: true,
     jest: true,
+
+    // act
+    IS_REACT_ACT_ENVIRONMENT: true,
   },
   parserOptions: {
     ecmaVersion: 2015,

diff --git a/scripts/rollup/validate/eslintrc.esm.js b/scripts/rollup/validate/eslintrc.esm.js
@@ -42,6 +42,9 @@ module.exports = {
     // jest
     expect: true,
     jest: true,
+
+    // act
+    IS_REACT_ACT_ENVIRONMENT: true,
   },
   parserOptions: {
     ecmaVersion: 2017,

diff --git a/scripts/rollup/validate/eslintrc.fb.js b/scripts/rollup/validate/eslintrc.fb.js
@@ -38,6 +38,9 @@ module.exports = {
 
     // jest
     jest: true,
+
+    // act
+    IS_REACT_ACT_ENVIRONMENT: true,
   },
   parserOptions: {
     ecmaVersion: 5,

diff --git a/scripts/rollup/validate/eslintrc.rn.js b/scripts/rollup/validate/eslintrc.rn.js
@@ -34,6 +34,9 @@ module.exports = {
 
     // jest
     jest: true,
+
+    // act
+    IS_REACT_ACT_ENVIRONMENT: true,
   },
   parserOptions: {
     ecmaVersion: 5,