From bb01b374dfe4199ef7155d60b294f038a17596d4 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 14 Oct 2021 12:55:51 -0400 Subject: [PATCH] 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. --- .eslintrc.js | 1 + packages/jest-react/src/internalAct.js | 9 ++--- .../src/__tests__/ReactTestUtilsAct-test.js | 34 +++++++++++++++++++ .../react-reconciler/src/ReactFiberAct.new.js | 22 ++++++++---- .../react-reconciler/src/ReactFiberAct.old.js | 22 ++++++++---- packages/react/src/ReactCurrentActQueue.js | 6 ---- scripts/rollup/validate/eslintrc.cjs.js | 3 ++ scripts/rollup/validate/eslintrc.cjs2015.js | 3 ++ scripts/rollup/validate/eslintrc.esm.js | 3 ++ scripts/rollup/validate/eslintrc.fb.js | 3 ++ scripts/rollup/validate/eslintrc.rn.js | 3 ++ scripts/rollup/validate/eslintrc.umd.js | 3 ++ 12 files changed, 90 insertions(+), 22 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index d464e83b453aa..b129af49d3d6a 100644 --- 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 index 486d84c31f64a..9e6bf0e4fd66b 100644 --- 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 | 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 index a47e234534db2..2a9ea2c9cdd52 100644 --- 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(, 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 = process.env.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 index 7c40a3f5d5e01..c83b08d857227 100644 --- a/packages/react-reconciler/src/ReactFiberAct.new.js +++ b/packages/react-reconciler/src/ReactFiberAct.new.js @@ -10,13 +10,23 @@ import type {Fiber} from './ReactFiber.new'; import {warnsIfNotActing} from './ReactFiberHostConfig'; -import ReactSharedInternals from 'shared/ReactSharedInternals'; - -const {ReactCurrentActQueue} = ReactSharedInternals; - export function isActEnvironment(fiber: Fiber) { - const disableActWarning = ReactCurrentActQueue.disableActWarning; + 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 repalced by IS_REACT_ACT_ENVIRONMENT. // $FlowExpectedError - Flow doesn't know about jest const jestIsDefined = typeof jest !== 'undefined'; - return warnsIfNotActing && jestIsDefined && !disableActWarning; + 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 + ); } diff --git a/packages/react-reconciler/src/ReactFiberAct.old.js b/packages/react-reconciler/src/ReactFiberAct.old.js index 9539699be31f8..271251d28a02d 100644 --- a/packages/react-reconciler/src/ReactFiberAct.old.js +++ b/packages/react-reconciler/src/ReactFiberAct.old.js @@ -10,13 +10,23 @@ import type {Fiber} from './ReactFiber.old'; import {warnsIfNotActing} from './ReactFiberHostConfig'; -import ReactSharedInternals from 'shared/ReactSharedInternals'; - -const {ReactCurrentActQueue} = ReactSharedInternals; - export function isActEnvironment(fiber: Fiber) { - const disableActWarning = ReactCurrentActQueue.disableActWarning; + 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 repalced by IS_REACT_ACT_ENVIRONMENT. // $FlowExpectedError - Flow doesn't know about jest const jestIsDefined = typeof jest !== 'undefined'; - return warnsIfNotActing && jestIsDefined && !disableActWarning; + 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 + ); } diff --git a/packages/react/src/ReactCurrentActQueue.js b/packages/react/src/ReactCurrentActQueue.js index 694907a10335e..690f2568c4184 100644 --- 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), - // 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 index 12b8931f06637..0adb5a197dd12 100644 --- 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 index d94b4bc4a02b0..721dd9b026bf1 100644 --- 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 index be4dbf621bf48..bbc94a7f4ee25 100644 --- 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 index 5621d93f0f902..bd5881ca47531 100644 --- 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 index a20cdeb4a37ea..0f2d7f808da81 100644 --- 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, diff --git a/scripts/rollup/validate/eslintrc.umd.js b/scripts/rollup/validate/eslintrc.umd.js index 95ef9550dd6ab..2077eec029484 100644 --- a/scripts/rollup/validate/eslintrc.umd.js +++ b/scripts/rollup/validate/eslintrc.umd.js @@ -47,6 +47,9 @@ module.exports = { // jest jest: true, + + // act + IS_REACT_ACT_ENVIRONMENT: true, }, parserOptions: { ecmaVersion: 5,