From 996da67b2991f480b6219471e38aeaa7280bd00c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 18 Oct 2021 11:59:18 -0400 Subject: [PATCH] Replace global `jest` heuristic with `IS_REACT_ACT_ENVIRONMENT` (#22562) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Remove `jest` global check in concurrent roots In concurrent mode, instead of checking `jest`, we check the new `IS_REACT_ACT_ENVIRONMENT` global. The default behavior is `false`. Legacy mode behavior is unchanged. React's own internal test suite use a custom version of `act` that works by mocking the Scheduler — rather than the "real" act used publicly. So we don't enable the flag in our repo. * Warn if `act` is called in wrong environment Adds a warning if `act` is called but `IS_REACT_ACT_ENVIRONMENT` is not enabled. The goal is to prompt users to correctly configure their testing environment, so that if they forget to use `act` in a different test, we can detect and warn about. It's expected that the environment flag will be configured by the testing framework. For example, a Jest plugin. We will link to the relevant documentation page, once it exists. The warning only fires in concurrent mode. Legacy roots will keep the existing behavior. --- .../__tests__/preprocessData-test.internal.js | 2 + .../src/__tests__/inspectedElement-test.js | 2 + .../ReactDOMNativeEventHeuristic-test.js | 26 +--- .../src/__tests__/ReactTestUtilsAct-test.js | 2 + .../react-reconciler/src/ReactFiberAct.new.js | 43 ++++-- .../react-reconciler/src/ReactFiberAct.old.js | 43 ++++-- .../__tests__/DebugTracing-test.internal.js | 2 + .../src/__tests__/ReactActWarnings-test.js | 130 ++++++++++++++++++ .../SchedulingProfilerLabels-test.internal.js | 2 + 9 files changed, 205 insertions(+), 47 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactActWarnings-test.js diff --git a/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js b/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js index e24611e9daea5..e458f8060f84b 100644 --- a/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js +++ b/packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js @@ -17,6 +17,8 @@ import { } from '../../constants'; import REACT_VERSION from 'shared/ReactVersion'; +global.IS_REACT_ACT_ENVIRONMENT = true; + describe('getLanesFromTransportDecimalBitmask', () => { it('should return array of lane numbers from bitmask string', () => { expect(getLanesFromTransportDecimalBitmask('1')).toEqual([0]); diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 68d014de3ad47..be5a852396538 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -38,6 +38,8 @@ describe('InspectedElement', () => { let ErrorBoundary; let errorBoundaryInstance; + global.IS_REACT_ACT_ENVIRONMENT = true; + beforeEach(() => { utils = require('./utils'); utils.beforeEachProfiling(); diff --git a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js index 702f213d7a2ed..94c5fe35290bd 100644 --- a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js @@ -76,9 +76,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => { // Dispatch a click event on the Disable-button. const firstEvent = document.createEvent('Event'); firstEvent.initEvent('click', true, true); - expect(() => - dispatchAndSetCurrentEvent(disableButton, firstEvent), - ).toErrorDev(['An update to Form inside a test was not wrapped in act']); + dispatchAndSetCurrentEvent(disableButton, firstEvent); // Discrete events should be flushed in a microtask. // Verify that the second button was removed. @@ -134,9 +132,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => { // Dispatch a click event on the Disable-button. const firstEvent = document.createEvent('Event'); firstEvent.initEvent('click', true, true); - expect(() => { - dispatchAndSetCurrentEvent(disableButton, firstEvent); - }).toErrorDev(['An update to Form inside a test was not wrapped in act']); + dispatchAndSetCurrentEvent(disableButton, firstEvent); // There should now be a pending update to disable the form. // This should not have flushed yet since it's in concurrent mode. @@ -196,9 +192,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => { // Dispatch a click event on the Enable-button. const firstEvent = document.createEvent('Event'); firstEvent.initEvent('click', true, true); - expect(() => { - dispatchAndSetCurrentEvent(enableButton, firstEvent); - }).toErrorDev(['An update to Form inside a test was not wrapped in act']); + dispatchAndSetCurrentEvent(enableButton, firstEvent); // There should now be a pending update to enable the form. // This should not have flushed yet since it's in concurrent mode. @@ -344,9 +338,6 @@ describe('ReactDOMNativeEventHeuristic-test', () => { }); expect(container.textContent).toEqual('Count: 0'); - // Ignore act warning. We can't use act because it forces batched updates. - spyOnDev(console, 'error'); - const pressEvent = document.createEvent('Event'); pressEvent.initEvent('click', true, true); dispatchAndSetCurrentEvent(target.current, pressEvent); @@ -355,17 +346,6 @@ describe('ReactDOMNativeEventHeuristic-test', () => { await null; // If this is 2, that means the `setCount` calls were not batched. expect(container.textContent).toEqual('Count: 1'); - - // Assert that the `act` warnings were the only ones that fired. - if (__DEV__) { - expect(console.error).toHaveBeenCalledTimes(2); - expect(console.error.calls.argsFor(0)[0]).toContain( - 'was not wrapped in act', - ); - expect(console.error.calls.argsFor(1)[0]).toContain( - 'was not wrapped in act', - ); - } }); it('should not flush discrete events at the end of outermost batchedUpdates', async () => { diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index f5e33d789891c..d0992dce53d41 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -16,6 +16,8 @@ let container; jest.useRealTimers(); +global.IS_REACT_ACT_ENVIRONMENT = true; + function sleep(period) { return new Promise(resolve => { setTimeout(() => { diff --git a/packages/react-reconciler/src/ReactFiberAct.new.js b/packages/react-reconciler/src/ReactFiberAct.new.js index 4338a7d5cd459..18055e7c738c7 100644 --- a/packages/react-reconciler/src/ReactFiberAct.new.js +++ b/packages/react-reconciler/src/ReactFiberAct.new.js @@ -8,7 +8,13 @@ */ import type {Fiber} from './ReactFiber.new'; + +import ReactSharedInternals from 'shared/ReactSharedInternals'; + import {warnsIfNotActing} from './ReactFiberHostConfig'; +import {ConcurrentMode} from './ReactTypeOfMode'; + +const {ReactCurrentActQueue} = ReactSharedInternals; export function isActEnvironment(fiber: Fiber) { if (__DEV__) { @@ -18,18 +24,31 @@ export function isActEnvironment(fiber: Fiber) { ? 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 - ); + if (fiber.mode & ConcurrentMode) { + if ( + !isReactActEnvironmentGlobal && + ReactCurrentActQueue.current !== null + ) { + // TODO: Include link to relevant documentation page. + console.error( + 'The current testing environment is not configured to support ' + + 'act(...)', + ); + } + return isReactActEnvironmentGlobal; + } else { + // Legacy mode. We preserve the behavior of React 17's act. It 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. + // $FlowExpectedError - Flow doesn't know about jest + const jestIsDefined = typeof jest !== 'undefined'; + return ( + warnsIfNotActing && + jestIsDefined && + isReactActEnvironmentGlobal !== false + ); + } } return false; } diff --git a/packages/react-reconciler/src/ReactFiberAct.old.js b/packages/react-reconciler/src/ReactFiberAct.old.js index e477024a858f8..ddae518dcb631 100644 --- a/packages/react-reconciler/src/ReactFiberAct.old.js +++ b/packages/react-reconciler/src/ReactFiberAct.old.js @@ -8,7 +8,13 @@ */ import type {Fiber} from './ReactFiber.old'; + +import ReactSharedInternals from 'shared/ReactSharedInternals'; + import {warnsIfNotActing} from './ReactFiberHostConfig'; +import {ConcurrentMode} from './ReactTypeOfMode'; + +const {ReactCurrentActQueue} = ReactSharedInternals; export function isActEnvironment(fiber: Fiber) { if (__DEV__) { @@ -18,18 +24,31 @@ export function isActEnvironment(fiber: Fiber) { ? 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 - ); + if (fiber.mode & ConcurrentMode) { + if ( + !isReactActEnvironmentGlobal && + ReactCurrentActQueue.current !== null + ) { + // TODO: Include link to relevant documentation page. + console.error( + 'The current testing environment is not configured to support ' + + 'act(...)', + ); + } + return isReactActEnvironmentGlobal; + } else { + // Legacy mode. We preserve the behavior of React 17's act. It 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. + // $FlowExpectedError - Flow doesn't know about jest + const jestIsDefined = typeof jest !== 'undefined'; + return ( + warnsIfNotActing && + jestIsDefined && + isReactActEnvironmentGlobal !== false + ); + } } return false; } diff --git a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js index 94431b435054e..ee93b3f992994 100644 --- a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js +++ b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js @@ -19,6 +19,8 @@ describe('DebugTracing', () => { const DEFAULT_LANE_STRING = '0b0000000000000000000000000010000'; const RETRY_LANE_STRING = '0b0000000010000000000000000000000'; + global.IS_REACT_ACT_ENVIRONMENT = true; + beforeEach(() => { jest.resetModules(); diff --git a/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js new file mode 100644 index 0000000000000..058e01b1fe060 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js @@ -0,0 +1,130 @@ +/** + * 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. + * + * @jest-environment node + */ + +let React; +let Scheduler; +let ReactNoop; +let useState; +let act; + +// These tests are mostly concerned with concurrent roots. The legacy root +// behavior is covered by other older test suites and is unchanged from +// React 17. +describe('act warnings', () => { + beforeEach(() => { + jest.resetModules(); + React = require('react'); + Scheduler = require('scheduler'); + ReactNoop = require('react-noop-renderer'); + act = React.unstable_act; + useState = React.useState; + }); + + function Text(props) { + Scheduler.unstable_yieldValue(props.text); + return props.text; + } + + function withActEnvironment(value, scope) { + const prevValue = global.IS_REACT_ACT_ENVIRONMENT; + global.IS_REACT_ACT_ENVIRONMENT = value; + try { + return scope(); + } finally { + global.IS_REACT_ACT_ENVIRONMENT = prevValue; + } + } + + test('warns about unwrapped updates only if environment flag is enabled', () => { + let setState; + function App() { + const [state, _setState] = useState(0); + setState = _setState; + return ; + } + + const root = ReactNoop.createRoot(); + root.render(); + expect(Scheduler).toFlushAndYield([0]); + expect(root).toMatchRenderedOutput('0'); + + // Default behavior. Flag is undefined. No warning. + expect(global.IS_REACT_ACT_ENVIRONMENT).toBe(undefined); + setState(1); + expect(Scheduler).toFlushAndYield([1]); + expect(root).toMatchRenderedOutput('1'); + + // Flag is true. Warn. + withActEnvironment(true, () => { + expect(() => setState(2)).toErrorDev( + 'An update to App inside a test was not wrapped in act', + ); + expect(Scheduler).toFlushAndYield([2]); + expect(root).toMatchRenderedOutput('2'); + }); + + // Flag is false. No warning. + withActEnvironment(false, () => { + setState(3); + expect(Scheduler).toFlushAndYield([3]); + expect(root).toMatchRenderedOutput('3'); + }); + }); + + // @gate __DEV__ + test('act warns if the environment flag is not enabled', () => { + let setState; + function App() { + const [state, _setState] = useState(0); + setState = _setState; + return ; + } + + const root = ReactNoop.createRoot(); + root.render(); + expect(Scheduler).toFlushAndYield([0]); + expect(root).toMatchRenderedOutput('0'); + + // Default behavior. Flag is undefined. Warn. + expect(global.IS_REACT_ACT_ENVIRONMENT).toBe(undefined); + expect(() => { + act(() => { + setState(1); + }); + }).toErrorDev( + 'The current testing environment is not configured to support act(...)', + {withoutStack: true}, + ); + expect(Scheduler).toHaveYielded([1]); + expect(root).toMatchRenderedOutput('1'); + + // Flag is true. Don't warn. + withActEnvironment(true, () => { + act(() => { + setState(2); + }); + expect(Scheduler).toHaveYielded([2]); + expect(root).toMatchRenderedOutput('2'); + }); + + // Flag is false. Warn. + withActEnvironment(false, () => { + expect(() => { + act(() => { + setState(1); + }); + }).toErrorDev( + 'The current testing environment is not configured to support act(...)', + {withoutStack: true}, + ); + expect(Scheduler).toHaveYielded([1]); + expect(root).toMatchRenderedOutput('1'); + }); + }); +}); diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js index 09fdb95b01e6c..13ca3988b2a71 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js @@ -21,6 +21,8 @@ describe('SchedulingProfiler labels', () => { let featureDetectionMarkName = null; let marks; + global.IS_REACT_ACT_ENVIRONMENT = true; + function polyfillJSDomUserTiming() { featureDetectionMarkName = null;