From 29a0fd3b3b42451788632df76c877a43f5533e70 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 14 Oct 2021 16:12:53 -0400 Subject: [PATCH] Remove `jest` global check in concurrent roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../ReactDOMNativeEventHeuristic-test.js | 26 +------ .../src/__tests__/ReactTestUtilsAct-test.js | 2 + .../react-reconciler/src/ReactFiberAct.new.js | 28 ++++--- .../react-reconciler/src/ReactFiberAct.old.js | 28 ++++--- .../src/__tests__/ReactActWarnings-test.js | 76 +++++++++++++++++++ 5 files changed, 113 insertions(+), 47 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactActWarnings-test.js 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..893e5cb5189a6 100644 --- a/packages/react-reconciler/src/ReactFiberAct.new.js +++ b/packages/react-reconciler/src/ReactFiberAct.new.js @@ -9,6 +9,7 @@ import type {Fiber} from './ReactFiber.new'; import {warnsIfNotActing} from './ReactFiberHostConfig'; +import {ConcurrentMode} from './ReactTypeOfMode'; export function isActEnvironment(fiber: Fiber) { if (__DEV__) { @@ -18,18 +19,21 @@ 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) { + 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..fb404627b5462 100644 --- a/packages/react-reconciler/src/ReactFiberAct.old.js +++ b/packages/react-reconciler/src/ReactFiberAct.old.js @@ -9,6 +9,7 @@ import type {Fiber} from './ReactFiber.old'; import {warnsIfNotActing} from './ReactFiberHostConfig'; +import {ConcurrentMode} from './ReactTypeOfMode'; export function isActEnvironment(fiber: Fiber) { if (__DEV__) { @@ -18,18 +19,21 @@ 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) { + 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__/ReactActWarnings-test.js b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js new file mode 100644 index 0000000000000..77cfff3e63f0f --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactActWarnings-test.js @@ -0,0 +1,76 @@ +/** + * 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; + +// 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(() => { + React = require('react'); + Scheduler = require('scheduler'); + ReactNoop = require('react-noop-renderer'); + 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'); + }); + }); +});