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.

Sign up for GitHub

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

Expand act warning to cover all APIs that might schedule React work #22607

Merged
merged 4 commits into from
Oct 21, 2021
Merged

Expand act warning to cover all APIs that might schedule React work #22607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ace6501
Move isActEnvironment check to function that warns
acdlite Oct 20, 2021
72a8460
Lift `mode` check, too
acdlite Oct 20, 2021
846c1b1
Expand act warning to include non-hook APIs
acdlite Oct 20, 2021
1961c99
Expand act warning to include Suspense resolutions
acdlite Oct 20, 2021
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
28 changes: 16 additions & 12 deletions packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ describe('React hooks DevTools integration', () => {
let scheduleUpdate;
let setSuspenseHandler;

global.IS_REACT_ACT_ENVIRONMENT = true;

beforeEach(() => {
global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = {
inject: injected => {
Expand Down Expand Up @@ -64,7 +66,7 @@ describe('React hooks DevTools integration', () => {
expect(stateHook.isStateEditable).toBe(true);

if (__DEV__) {
overrideHookState(fiber, stateHook.id, [], 10);
act(() => overrideHookState(fiber, stateHook.id, [], 10));
expect(renderer.toJSON()).toEqual({
type: 'div',
props: {},
Expand Down Expand Up @@ -116,7 +118,7 @@ describe('React hooks DevTools integration', () => {
expect(reducerHook.isStateEditable).toBe(true);

if (__DEV__) {
overrideHookState(fiber, reducerHook.id, ['foo'], 'def');
act(() => overrideHookState(fiber, reducerHook.id, ['foo'], 'def'));
expect(renderer.toJSON()).toEqual({
type: 'div',
props: {},
Expand Down Expand Up @@ -164,13 +166,12 @@ describe('React hooks DevTools integration', () => {
expect(stateHook.isStateEditable).toBe(true);

if (__DEV__) {
overrideHookState(fiber, stateHook.id, ['count'], 10);
act(() => overrideHookState(fiber, stateHook.id, ['count'], 10));
expect(renderer.toJSON()).toEqual({
type: 'div',
props: {},
children: ['count:', '10'],
});

act(() => setStateFn(state => ({count: state.count + 1})));
expect(renderer.toJSON()).toEqual({
type: 'div',
Expand Down Expand Up @@ -233,7 +234,8 @@ describe('React hooks DevTools integration', () => {
}
});

it('should support overriding suspense in concurrent mode', () => {
// @gate __DEV__
it('should support overriding suspense in concurrent mode', async () => {
if (__DEV__) {
// Lock the first render
setSuspenseHandler(() => true);
Expand All @@ -243,13 +245,15 @@ describe('React hooks DevTools integration', () => {
return 'Done';
}

const renderer = ReactTestRenderer.create(
<div>
<React.Suspense fallback={'Loading'}>
<MyComponent />
</React.Suspense>
</div>,
{unstable_isConcurrent: true},
const renderer = await act(() =>
ReactTestRenderer.create(
<div>
<React.Suspense fallback={'Loading'}>
<MyComponent />
</React.Suspense>
</div>,
{unstable_isConcurrent: true},
),
);

expect(Scheduler).toFlushAndYield([]);
Expand Down
12 changes: 7 additions & 5 deletions ...-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ 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 Expand Up @@ -210,6 +208,8 @@ describe('preprocessData', () => {
tid = 0;
pid = 0;
startTime = 0;

global.IS_REACT_ACT_ENVIRONMENT = true;
});

afterEach(() => {
Expand Down Expand Up @@ -1251,7 +1251,7 @@ describe('preprocessData', () => {

testMarks.push(...createUserTimingData(clearedMarks));

const data = await preprocessData(testMarks);
const data = await act(() => preprocessData(testMarks));
expect(data.suspenseEvents).toHaveLength(1);
expect(data.suspenseEvents[0].promiseName).toBe('Testing displayName');
}
Expand Down Expand Up @@ -1367,6 +1367,8 @@ describe('preprocessData', () => {

const root = ReactDOM.createRoot(document.createElement('div'));

// Temporarily turn off the act environment, since we're intentionally using Scheduler instead.
global.IS_REACT_ACT_ENVIRONMENT = false;
React.startTransition(() => {
// Start rendering an async update (but don't finish).
root.render(
Expand Down Expand Up @@ -1837,7 +1839,7 @@ describe('preprocessData', () => {

testMarks.push(...createUserTimingData(clearedMarks));

const data = await preprocessData(testMarks);
const data = await act(() => preprocessData(testMarks));
expect(data.suspenseEvents).toHaveLength(1);
expect(data.suspenseEvents[0].warning).toMatchInlineSnapshot(
`"A component suspended during an update which caused a fallback to be shown. Consider using the Transition API to avoid hiding components after they've been mounted."`,
Expand Down Expand Up @@ -1895,7 +1897,7 @@ describe('preprocessData', () => {

testMarks.push(...createUserTimingData(clearedMarks));

const data = await preprocessData(testMarks);
const data = await act(() => preprocessData(testMarks));
expect(data.suspenseEvents).toHaveLength(1);
expect(data.suspenseEvents[0].warning).toBe(null);
}
Expand Down
38 changes: 29 additions & 9 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,18 +32,31 @@ describe('ReactTestUtils.act()', () => {
let concurrentRoot = null;
const renderConcurrent = (el, dom) => {
concurrentRoot = ReactDOM.createRoot(dom);
concurrentRoot.render(el);
if (__DEV__) {
act(() => concurrentRoot.render(el));
} else {
concurrentRoot.render(el);
}
};

const unmountConcurrent = _dom => {
if (concurrentRoot !== null) {
concurrentRoot.unmount();
concurrentRoot = null;
if (__DEV__) {
act(() => {
if (concurrentRoot !== null) {
concurrentRoot.unmount();
concurrentRoot = null;
}
});
} else {
if (concurrentRoot !== null) {
concurrentRoot.unmount();
concurrentRoot = null;
}
}
};

const rerenderConcurrent = el => {
concurrentRoot.render(el);
act(() => concurrentRoot.render(el));
};

runActTests(
Expand Down Expand Up @@ -98,22 +111,29 @@ describe('ReactTestUtils.act()', () => {
]);
});

// @gate __DEV__
it('does not warn in concurrent mode', () => {
const root = ReactDOM.createRoot(document.createElement('div'));
root.render(<App />);
act(() => root.render(<App />));
Scheduler.unstable_flushAll();
});

it('warns in concurrent mode if root is strict', () => {
// TODO: We don't need this error anymore in concurrent mode because
// effects can only be scheduled as the result of an update, and we now
// enforce all updates must be wrapped with act, not just hook updates.
expect(() => {
const root = ReactDOM.createRoot(document.createElement('div'), {
unstable_strictMode: true,
});
root.render(<App />);
Scheduler.unstable_flushAll();
}).toErrorDev([
}).toErrorDev(
'An update to Root inside a test was not wrapped in act(...)',
{withoutStack: true},
);
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
'An update to App ran an effect, but was not wrapped in act(...)',
]);
);
});
});
});
Expand Down
54 changes: 29 additions & 25 deletions packages/react-reconciler/src/ReactFiberAct.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,43 +12,47 @@ 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) {
export function isLegacyActEnvironment(fiber: Fiber) {
if (__DEV__) {
// 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.

const isReactActEnvironmentGlobal =
// $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
? IS_REACT_ACT_ENVIRONMENT
: undefined;

// $FlowExpectedError - Flow doesn't know about jest
const jestIsDefined = typeof jest !== 'undefined';
return (
warnsIfNotActing && jestIsDefined && isReactActEnvironmentGlobal !== false
);
}
return false;
}

export function isConcurrentActEnvironment() {
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;

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
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;
}
return false;
}
54 changes: 29 additions & 25 deletions packages/react-reconciler/src/ReactFiberAct.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,43 +12,47 @@ 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) {
export function isLegacyActEnvironment(fiber: Fiber) {
if (__DEV__) {
// 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.

const isReactActEnvironmentGlobal =
// $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
? IS_REACT_ACT_ENVIRONMENT
: undefined;

// $FlowExpectedError - Flow doesn't know about jest
const jestIsDefined = typeof jest !== 'undefined';
return (
warnsIfNotActing && jestIsDefined && isReactActEnvironmentGlobal !== false
);
}
return false;
}

export function isConcurrentActEnvironment() {
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;

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
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;
}
return false;
}
Loading