Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace global jest heuristic with IS_REACT_ACT_ENVIRONMENT #22562

Merged
merged 2 commits into from
Oct 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ describe('InspectedElement', () => {
let ErrorBoundary;
let errorBoundaryInstance;

global.IS_REACT_ACT_ENVIRONMENT = true;

beforeEach(() => {
utils = require('./utils');
utils.beforeEachProfiling();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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']);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These warnings no longer fire because our tests don't enable the act environment flag — we mock the Scheduler package instead, or (as in this test) manually flush the microtask queue.

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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand All @@ -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 () => {
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ let container;

jest.useRealTimers();

global.IS_REACT_ACT_ENVIRONMENT = true;

function sleep(period) {
return new Promise(resolve => {
setTimeout(() => {
Expand Down
43 changes: 31 additions & 12 deletions packages/react-reconciler/src/ReactFiberAct.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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__) {
Expand All @@ -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;
}
43 changes: 31 additions & 12 deletions packages/react-reconciler/src/ReactFiberAct.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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__) {
Expand All @@ -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;
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
130 changes: 130 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactActWarnings-test.js
Original file line number Diff line number Diff line change
@@ -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 <Text text={state} />;
}

const root = ReactNoop.createRoot();
root.render(<App />);
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 <Text text={state} />;
}

const root = ReactNoop.createRoot();
root.render(<App />);
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');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ describe('SchedulingProfiler labels', () => {
let featureDetectionMarkName = null;
let marks;

global.IS_REACT_ACT_ENVIRONMENT = true;

function polyfillJSDomUserTiming() {
featureDetectionMarkName = null;

Expand Down