Skip to content

Commit 9c34993

Browse files
acdlitezhengjitf
acdlite
authored and
zhengjitf
committed
Expand act warning to cover all APIs that might schedule React work (facebook#22607)
* Move isActEnvironment check to function that warns I'm about to fork the behavior in legacy roots versus concurrent roots even further, so I'm lifting this up so I only have to fork once. * Lift `mode` check, too Similar to previous commit. I only want to check this once. Not for performance reasons, but so the logic is easier to follow. * Expand act warning to include non-hook APIs In a test environment, React warns if an update isn't wrapped with act — but only if the update originates from a hook API, like useState. We did it this way for backwards compatibility with tests that were written before the act API was introduced. Those tests didn't require act, anyway, because in a legacy root, all tasks are synchronous except for `useEffect`. However, in a concurrent root, nearly every task is asynchronous. Even tasks that are synchronous may spawn additional asynchronous work. So all updates need to be wrapped with act, regardless of whether they originate from a hook, a class, a root, or any other type of component. This commit expands the act warning to include any API that triggers an update. It does not currently account for renders that are caused by a Suspense promise resolving; those are modelled slightly differently from updates. I'll fix that in the next step. I also removed the check for whether an update is batched. It shouldn't matter, because even a batched update can spawn asynchronous work, which needs to be flushed by act. This change only affects concurrent roots. The behavior in legacy roots is the same. * Expand act warning to include Suspense resolutions For the same reason we warn when an update is not wrapped with act, we should warn if a Suspense promise resolution is not wrapped with act. Both "pings" and "retries". Legacy root behavior is unchanged.
1 parent c9abb0a commit 9c34993

13 files changed

+578
-240
lines changed

‎packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js

+16-12
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ describe('React hooks DevTools integration', () => {
2020
let scheduleUpdate;
2121
let setSuspenseHandler;
2222

23+
global.IS_REACT_ACT_ENVIRONMENT = true;
24+
2325
beforeEach(() => {
2426
global.__REACT_DEVTOOLS_GLOBAL_HOOK__ = {
2527
inject: injected => {
@@ -64,7 +66,7 @@ describe('React hooks DevTools integration', () => {
6466
expect(stateHook.isStateEditable).toBe(true);
6567

6668
if (__DEV__) {
67-
overrideHookState(fiber, stateHook.id, [], 10);
69+
act(() => overrideHookState(fiber, stateHook.id, [], 10));
6870
expect(renderer.toJSON()).toEqual({
6971
type: 'div',
7072
props: {},
@@ -116,7 +118,7 @@ describe('React hooks DevTools integration', () => {
116118
expect(reducerHook.isStateEditable).toBe(true);
117119

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

166168
if (__DEV__) {
167-
overrideHookState(fiber, stateHook.id, ['count'], 10);
169+
act(() => overrideHookState(fiber, stateHook.id, ['count'], 10));
168170
expect(renderer.toJSON()).toEqual({
169171
type: 'div',
170172
props: {},
171173
children: ['count:', '10'],
172174
});
173-
174175
act(() => setStateFn(state => ({count: state.count + 1})));
175176
expect(renderer.toJSON()).toEqual({
176177
type: 'div',
@@ -233,7 +234,8 @@ describe('React hooks DevTools integration', () => {
233234
}
234235
});
235236

236-
it('should support overriding suspense in concurrent mode', () => {
237+
// @gate __DEV__
238+
it('should support overriding suspense in concurrent mode', async () => {
237239
if (__DEV__) {
238240
// Lock the first render
239241
setSuspenseHandler(() => true);
@@ -243,13 +245,15 @@ describe('React hooks DevTools integration', () => {
243245
return 'Done';
244246
}
245247

246-
const renderer = ReactTestRenderer.create(
247-
<div>
248-
<React.Suspense fallback={'Loading'}>
249-
<MyComponent />
250-
</React.Suspense>
251-
</div>,
252-
{unstable_isConcurrent: true},
248+
const renderer = await act(() =>
249+
ReactTestRenderer.create(
250+
<div>
251+
<React.Suspense fallback={'Loading'}>
252+
<MyComponent />
253+
</React.Suspense>
254+
</div>,
255+
{unstable_isConcurrent: true},
256+
),
253257
);
254258

255259
expect(Scheduler).toFlushAndYield([]);

‎packages/react-devtools-scheduling-profiler/src/import-worker/__tests__/preprocessData-test.internal.js

+7-5
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,6 @@ import {
1717
} from '../../constants';
1818
import REACT_VERSION from 'shared/ReactVersion';
1919

20-
global.IS_REACT_ACT_ENVIRONMENT = true;
21-
2220
describe('getLanesFromTransportDecimalBitmask', () => {
2321
it('should return array of lane numbers from bitmask string', () => {
2422
expect(getLanesFromTransportDecimalBitmask('1')).toEqual([0]);
@@ -210,6 +208,8 @@ describe('preprocessData', () => {
210208
tid = 0;
211209
pid = 0;
212210
startTime = 0;
211+
212+
global.IS_REACT_ACT_ENVIRONMENT = true;
213213
});
214214

215215
afterEach(() => {
@@ -1251,7 +1251,7 @@ describe('preprocessData', () => {
12511251

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

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

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

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

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

1840-
const data = await preprocessData(testMarks);
1842+
const data = await act(() => preprocessData(testMarks));
18411843
expect(data.suspenseEvents).toHaveLength(1);
18421844
expect(data.suspenseEvents[0].warning).toMatchInlineSnapshot(
18431845
`"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."`,
@@ -1895,7 +1897,7 @@ describe('preprocessData', () => {
18951897

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

1898-
const data = await preprocessData(testMarks);
1900+
const data = await act(() => preprocessData(testMarks));
18991901
expect(data.suspenseEvents).toHaveLength(1);
19001902
expect(data.suspenseEvents[0].warning).toBe(null);
19011903
}

‎packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js

+29-9
Original file line numberDiff line numberDiff line change
@@ -32,18 +32,31 @@ describe('ReactTestUtils.act()', () => {
3232
let concurrentRoot = null;
3333
const renderConcurrent = (el, dom) => {
3434
concurrentRoot = ReactDOM.createRoot(dom);
35-
concurrentRoot.render(el);
35+
if (__DEV__) {
36+
act(() => concurrentRoot.render(el));
37+
} else {
38+
concurrentRoot.render(el);
39+
}
3640
};
3741

3842
const unmountConcurrent = _dom => {
39-
if (concurrentRoot !== null) {
40-
concurrentRoot.unmount();
41-
concurrentRoot = null;
43+
if (__DEV__) {
44+
act(() => {
45+
if (concurrentRoot !== null) {
46+
concurrentRoot.unmount();
47+
concurrentRoot = null;
48+
}
49+
});
50+
} else {
51+
if (concurrentRoot !== null) {
52+
concurrentRoot.unmount();
53+
concurrentRoot = null;
54+
}
4255
}
4356
};
4457

4558
const rerenderConcurrent = el => {
46-
concurrentRoot.render(el);
59+
act(() => concurrentRoot.render(el));
4760
};
4861

4962
runActTests(
@@ -98,22 +111,29 @@ describe('ReactTestUtils.act()', () => {
98111
]);
99112
});
100113

114+
// @gate __DEV__
101115
it('does not warn in concurrent mode', () => {
102116
const root = ReactDOM.createRoot(document.createElement('div'));
103-
root.render(<App />);
117+
act(() => root.render(<App />));
104118
Scheduler.unstable_flushAll();
105119
});
106120

107121
it('warns in concurrent mode if root is strict', () => {
122+
// TODO: We don't need this error anymore in concurrent mode because
123+
// effects can only be scheduled as the result of an update, and we now
124+
// enforce all updates must be wrapped with act, not just hook updates.
108125
expect(() => {
109126
const root = ReactDOM.createRoot(document.createElement('div'), {
110127
unstable_strictMode: true,
111128
});
112129
root.render(<App />);
113-
Scheduler.unstable_flushAll();
114-
}).toErrorDev([
130+
}).toErrorDev(
131+
'An update to Root inside a test was not wrapped in act(...)',
132+
{withoutStack: true},
133+
);
134+
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
115135
'An update to App ran an effect, but was not wrapped in act(...)',
116-
]);
136+
);
117137
});
118138
});
119139
});

‎packages/react-reconciler/src/ReactFiberAct.new.js

+29-25
Original file line numberDiff line numberDiff line change
@@ -12,43 +12,47 @@ import type {Fiber} from './ReactFiber.new';
1212
import ReactSharedInternals from 'shared/ReactSharedInternals';
1313

1414
import {warnsIfNotActing} from './ReactFiberHostConfig';
15-
import {ConcurrentMode} from './ReactTypeOfMode';
1615

1716
const {ReactCurrentActQueue} = ReactSharedInternals;
1817

19-
export function isActEnvironment(fiber: Fiber) {
18+
export function isLegacyActEnvironment(fiber: Fiber) {
19+
if (__DEV__) {
20+
// Legacy mode. We preserve the behavior of React 17's act. It assumes an
21+
// act environment whenever `jest` is defined, but you can still turn off
22+
// spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly
23+
// to false.
24+
25+
const isReactActEnvironmentGlobal =
26+
// $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
27+
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
28+
? IS_REACT_ACT_ENVIRONMENT
29+
: undefined;
30+
31+
// $FlowExpectedError - Flow doesn't know about jest
32+
const jestIsDefined = typeof jest !== 'undefined';
33+
return (
34+
warnsIfNotActing && jestIsDefined && isReactActEnvironmentGlobal !== false
35+
);
36+
}
37+
return false;
38+
}
39+
40+
export function isConcurrentActEnvironment() {
2041
if (__DEV__) {
2142
const isReactActEnvironmentGlobal =
2243
// $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
2344
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
2445
? IS_REACT_ACT_ENVIRONMENT
2546
: undefined;
2647

27-
if (fiber.mode & ConcurrentMode) {
28-
if (
29-
!isReactActEnvironmentGlobal &&
30-
ReactCurrentActQueue.current !== null
31-
) {
32-
// TODO: Include link to relevant documentation page.
33-
console.error(
34-
'The current testing environment is not configured to support ' +
35-
'act(...)',
36-
);
37-
}
38-
return isReactActEnvironmentGlobal;
39-
} else {
40-
// Legacy mode. We preserve the behavior of React 17's act. It assumes an
41-
// act environment whenever `jest` is defined, but you can still turn off
42-
// spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly
43-
// to false.
44-
// $FlowExpectedError - Flow doesn't know about jest
45-
const jestIsDefined = typeof jest !== 'undefined';
46-
return (
47-
warnsIfNotActing &&
48-
jestIsDefined &&
49-
isReactActEnvironmentGlobal !== false
48+
if (!isReactActEnvironmentGlobal && ReactCurrentActQueue.current !== null) {
49+
// TODO: Include link to relevant documentation page.
50+
console.error(
51+
'The current testing environment is not configured to support ' +
52+
'act(...)',
5053
);
5154
}
55+
return isReactActEnvironmentGlobal;
5256
}
5357
return false;
5458
}

‎packages/react-reconciler/src/ReactFiberAct.old.js

+29-25
Original file line numberDiff line numberDiff line change
@@ -12,43 +12,47 @@ import type {Fiber} from './ReactFiber.old';
1212
import ReactSharedInternals from 'shared/ReactSharedInternals';
1313

1414
import {warnsIfNotActing} from './ReactFiberHostConfig';
15-
import {ConcurrentMode} from './ReactTypeOfMode';
1615

1716
const {ReactCurrentActQueue} = ReactSharedInternals;
1817

19-
export function isActEnvironment(fiber: Fiber) {
18+
export function isLegacyActEnvironment(fiber: Fiber) {
19+
if (__DEV__) {
20+
// Legacy mode. We preserve the behavior of React 17's act. It assumes an
21+
// act environment whenever `jest` is defined, but you can still turn off
22+
// spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly
23+
// to false.
24+
25+
const isReactActEnvironmentGlobal =
26+
// $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
27+
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
28+
? IS_REACT_ACT_ENVIRONMENT
29+
: undefined;
30+
31+
// $FlowExpectedError - Flow doesn't know about jest
32+
const jestIsDefined = typeof jest !== 'undefined';
33+
return (
34+
warnsIfNotActing && jestIsDefined && isReactActEnvironmentGlobal !== false
35+
);
36+
}
37+
return false;
38+
}
39+
40+
export function isConcurrentActEnvironment() {
2041
if (__DEV__) {
2142
const isReactActEnvironmentGlobal =
2243
// $FlowExpectedError – Flow doesn't know about IS_REACT_ACT_ENVIRONMENT global
2344
typeof IS_REACT_ACT_ENVIRONMENT !== 'undefined'
2445
? IS_REACT_ACT_ENVIRONMENT
2546
: undefined;
2647

27-
if (fiber.mode & ConcurrentMode) {
28-
if (
29-
!isReactActEnvironmentGlobal &&
30-
ReactCurrentActQueue.current !== null
31-
) {
32-
// TODO: Include link to relevant documentation page.
33-
console.error(
34-
'The current testing environment is not configured to support ' +
35-
'act(...)',
36-
);
37-
}
38-
return isReactActEnvironmentGlobal;
39-
} else {
40-
// Legacy mode. We preserve the behavior of React 17's act. It assumes an
41-
// act environment whenever `jest` is defined, but you can still turn off
42-
// spurious warnings by setting IS_REACT_ACT_ENVIRONMENT explicitly
43-
// to false.
44-
// $FlowExpectedError - Flow doesn't know about jest
45-
const jestIsDefined = typeof jest !== 'undefined';
46-
return (
47-
warnsIfNotActing &&
48-
jestIsDefined &&
49-
isReactActEnvironmentGlobal !== false
48+
if (!isReactActEnvironmentGlobal && ReactCurrentActQueue.current !== null) {
49+
// TODO: Include link to relevant documentation page.
50+
console.error(
51+
'The current testing environment is not configured to support ' +
52+
'act(...)',
5053
);
5154
}
55+
return isReactActEnvironmentGlobal;
5256
}
5357
return false;
5458
}

0 commit comments

Comments
 (0)