From 163e81c1f88a47749e092f2d5b31b7c919f07de8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 18 Oct 2021 11:27:26 -0400 Subject: [PATCH] Support disabling spurious act warnings with a global environment flag (#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. --- .eslintrc.js | 1 + packages/jest-react/src/internalAct.js | 9 ++--- .../src/__tests__/ReactTestUtilsAct-test.js | 34 ++++++++++++++++++ .../react-reconciler/src/ReactFiberAct.new.js | 35 +++++++++++++++++++ .../react-reconciler/src/ReactFiberAct.old.js | 35 +++++++++++++++++++ .../src/ReactFiberHooks.new.js | 13 +++---- .../src/ReactFiberHooks.old.js | 13 +++---- .../src/ReactFiberWorkLoop.new.js | 19 ++-------- .../src/ReactFiberWorkLoop.old.js | 19 ++-------- 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 ++ 16 files changed, 142 insertions(+), 60 deletions(-) create mode 100644 packages/react-reconciler/src/ReactFiberAct.new.js create mode 100644 packages/react-reconciler/src/ReactFiberAct.old.js 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..f5e33d789891c 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 = 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 new file mode 100644 index 0000000000000..4338a7d5cd459 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..e477024a858f8 --- /dev/null +++ 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 index b66b62c9ade73..afaebd20a5264 100644 --- 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 | 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 | 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( 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( } } 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 index af21b5f6f9da3..03df5b1444e62 100644 --- 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 | 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 | 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( 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( } } 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 index 884f8cc22aa5d..89286bd573858 100644 --- 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 index db7a3de837b40..d5cb23483d2d0 100644 --- 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 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,