Skip to content

Commit 1961c99

Browse files
acdliteacdlite
committed
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 846c1b1 commit 1961c99

File tree

5 files changed

+243
-9
lines changed

5 files changed

+243
-9
lines changed

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -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
}
@@ -1839,7 +1839,7 @@ describe('preprocessData', () => {
18391839

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

1842-
const data = await preprocessData(testMarks);
1842+
const data = await act(() => preprocessData(testMarks));
18431843
expect(data.suspenseEvents).toHaveLength(1);
18441844
expect(data.suspenseEvents[0].warning).toMatchInlineSnapshot(
18451845
`"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."`,
@@ -1897,7 +1897,7 @@ describe('preprocessData', () => {
18971897

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

1900-
const data = await preprocessData(testMarks);
1900+
const data = await act(() => preprocessData(testMarks));
19011901
expect(data.suspenseEvents).toHaveLength(1);
19021902
expect(data.suspenseEvents[0].warning).toBe(null);
19031903
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2408,6 +2408,8 @@ export function pingSuspendedRoot(
24082408
const eventTime = requestEventTime();
24092409
markRootPinged(root, pingedLanes, eventTime);
24102410

2411+
warnIfSuspenseResolutionNotWrappedWithActDEV(root);
2412+
24112413
if (
24122414
workInProgressRoot === root &&
24132415
isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes)
@@ -2942,3 +2944,27 @@ function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void {
29422944
}
29432945
}
29442946
}
2947+
2948+
function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void {
2949+
if (__DEV__) {
2950+
if (
2951+
root.tag !== LegacyRoot &&
2952+
isConcurrentActEnvironment() &&
2953+
ReactCurrentActQueue.current === null
2954+
) {
2955+
console.error(
2956+
'A suspended resource finished loading inside a test, but the event ' +
2957+
'was not wrapped in act(...).\n\n' +
2958+
'When testing, code that resolves suspended data should be wrapped ' +
2959+
'into act(...):\n\n' +
2960+
'act(() => {\n' +
2961+
' /* finish loading suspended data */\n' +
2962+
'});\n' +
2963+
'/* assert on the output */\n\n' +
2964+
"This ensures that you're testing the behavior the user would see " +
2965+
'in the browser.' +
2966+
' Learn more at https://reactjs.org/link/wrap-tests-with-act',
2967+
);
2968+
}
2969+
}
2970+
}

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2408,6 +2408,8 @@ export function pingSuspendedRoot(
24082408
const eventTime = requestEventTime();
24092409
markRootPinged(root, pingedLanes, eventTime);
24102410

2411+
warnIfSuspenseResolutionNotWrappedWithActDEV(root);
2412+
24112413
if (
24122414
workInProgressRoot === root &&
24132415
isSubsetOfLanes(workInProgressRootRenderLanes, pingedLanes)
@@ -2942,3 +2944,27 @@ function warnIfUpdatesNotWrappedWithActDEV(fiber: Fiber): void {
29422944
}
29432945
}
29442946
}
2947+
2948+
function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void {
2949+
if (__DEV__) {
2950+
if (
2951+
root.tag !== LegacyRoot &&
2952+
isConcurrentActEnvironment() &&
2953+
ReactCurrentActQueue.current === null
2954+
) {
2955+
console.error(
2956+
'A suspended resource finished loading inside a test, but the event ' +
2957+
'was not wrapped in act(...).\n\n' +
2958+
'When testing, code that resolves suspended data should be wrapped ' +
2959+
'into act(...):\n\n' +
2960+
'act(() => {\n' +
2961+
' /* finish loading suspended data */\n' +
2962+
'});\n' +
2963+
'/* assert on the output */\n\n' +
2964+
"This ensures that you're testing the behavior the user would see " +
2965+
'in the browser.' +
2966+
' Learn more at https://reactjs.org/link/wrap-tests-with-act',
2967+
);
2968+
}
2969+
}
2970+
}

‎packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,20 @@ describe('DebugTracing', () => {
140140

141141
// @gate experimental && build === 'development' && enableDebugTracing
142142
it('should log concurrent render with suspense', async () => {
143-
const fakeSuspensePromise = Promise.resolve(true);
143+
let isResolved = false;
144+
let resolveFakeSuspensePromise;
145+
const fakeSuspensePromise = new Promise(resolve => {
146+
resolveFakeSuspensePromise = () => {
147+
resolve();
148+
isResolved = true;
149+
};
150+
});
151+
144152
function Example() {
145-
throw fakeSuspensePromise;
153+
if (!isResolved) {
154+
throw fakeSuspensePromise;
155+
}
156+
return null;
146157
}
147158

148159
ReactTestRenderer.act(() =>
@@ -164,7 +175,7 @@ describe('DebugTracing', () => {
164175

165176
logs.splice(0);
166177

167-
await fakeSuspensePromise;
178+
await ReactTestRenderer.act(async () => await resolveFakeSuspensePromise());
168179
expect(logs).toEqual(['log: ⚛️ Example resolved']);
169180
});
170181

‎packages/react-reconciler/src/__tests__/ReactActWarnings-test.js

Lines changed: 174 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,10 @@ let Scheduler;
1212
let ReactNoop;
1313
let useState;
1414
let act;
15+
let Suspense;
16+
let startTransition;
17+
let getCacheForType;
18+
let caches;
1519

1620
// These tests are mostly concerned with concurrent roots. The legacy root
1721
// behavior is covered by other older test suites and is unchanged from
@@ -24,11 +28,110 @@ describe('act warnings', () => {
2428
ReactNoop = require('react-noop-renderer');
2529
act = React.unstable_act;
2630
useState = React.useState;
31+
Suspense = React.Suspense;
32+
startTransition = React.startTransition;
33+
getCacheForType = React.unstable_getCacheForType;
34+
caches = [];
2735
});
2836

29-
function Text(props) {
30-
Scheduler.unstable_yieldValue(props.text);
31-
return props.text;
37+
function createTextCache() {
38+
const data = new Map();
39+
const version = caches.length + 1;
40+
const cache = {
41+
version,
42+
data,
43+
resolve(text) {
44+
const record = data.get(text);
45+
if (record === undefined) {
46+
const newRecord = {
47+
status: 'resolved',
48+
value: text,
49+
};
50+
data.set(text, newRecord);
51+
} else if (record.status === 'pending') {
52+
const thenable = record.value;
53+
record.status = 'resolved';
54+
record.value = text;
55+
thenable.pings.forEach(t => t());
56+
}
57+
},
58+
reject(text, error) {
59+
const record = data.get(text);
60+
if (record === undefined) {
61+
const newRecord = {
62+
status: 'rejected',
63+
value: error,
64+
};
65+
data.set(text, newRecord);
66+
} else if (record.status === 'pending') {
67+
const thenable = record.value;
68+
record.status = 'rejected';
69+
record.value = error;
70+
thenable.pings.forEach(t => t());
71+
}
72+
},
73+
};
74+
caches.push(cache);
75+
return cache;
76+
}
77+
78+
function readText(text) {
79+
const textCache = getCacheForType(createTextCache);
80+
const record = textCache.data.get(text);
81+
if (record !== undefined) {
82+
switch (record.status) {
83+
case 'pending':
84+
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
85+
throw record.value;
86+
case 'rejected':
87+
Scheduler.unstable_yieldValue(`Error! [${text}]`);
88+
throw record.value;
89+
case 'resolved':
90+
return textCache.version;
91+
}
92+
} else {
93+
Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
94+
95+
const thenable = {
96+
pings: [],
97+
then(resolve) {
98+
if (newRecord.status === 'pending') {
99+
thenable.pings.push(resolve);
100+
} else {
101+
Promise.resolve().then(() => resolve(newRecord.value));
102+
}
103+
},
104+
};
105+
106+
const newRecord = {
107+
status: 'pending',
108+
value: thenable,
109+
};
110+
textCache.data.set(text, newRecord);
111+
112+
throw thenable;
113+
}
114+
}
115+
116+
function Text({text}) {
117+
Scheduler.unstable_yieldValue(text);
118+
return text;
119+
}
120+
121+
function AsyncText({text}) {
122+
readText(text);
123+
Scheduler.unstable_yieldValue(text);
124+
return text;
125+
}
126+
127+
function resolveText(text) {
128+
if (caches.length === 0) {
129+
throw Error('Cache does not exist.');
130+
} else {
131+
// Resolve the most recently created cache. An older cache can by
132+
// resolved with `caches[index].resolve(text)`.
133+
caches[caches.length - 1].resolve(text);
134+
}
32135
}
33136

34137
function withActEnvironment(value, scope) {
@@ -187,4 +290,72 @@ describe('act warnings', () => {
187290
expect(root).toMatchRenderedOutput('1');
188291
});
189292
});
293+
294+
// @gate __DEV__
295+
// @gate enableCache
296+
test('warns if Suspense retry is not wrapped', () => {
297+
function App() {
298+
return (
299+
<Suspense fallback={<Text text="Loading..." />}>
300+
<AsyncText text="Async" />
301+
</Suspense>
302+
);
303+
}
304+
305+
withActEnvironment(true, () => {
306+
const root = ReactNoop.createRoot();
307+
act(() => {
308+
root.render(<App />);
309+
});
310+
expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
311+
expect(root).toMatchRenderedOutput('Loading...');
312+
313+
// This is a retry, not a ping, because we already showed a fallback.
314+
expect(() =>
315+
resolveText('Async'),
316+
).toErrorDev(
317+
'A suspended resource finished loading inside a test, but the event ' +
318+
'was not wrapped in act(...)',
319+
{withoutStack: true},
320+
);
321+
});
322+
});
323+
324+
// @gate __DEV__
325+
// @gate enableCache
326+
test('warns if Suspense ping is not wrapped', () => {
327+
function App({showMore}) {
328+
return (
329+
<Suspense fallback={<Text text="Loading..." />}>
330+
{showMore ? <AsyncText text="Async" /> : <Text text="(empty)" />}
331+
</Suspense>
332+
);
333+
}
334+
335+
withActEnvironment(true, () => {
336+
const root = ReactNoop.createRoot();
337+
act(() => {
338+
root.render(<App showMore={false} />);
339+
});
340+
expect(Scheduler).toHaveYielded(['(empty)']);
341+
expect(root).toMatchRenderedOutput('(empty)');
342+
343+
act(() => {
344+
startTransition(() => {
345+
root.render(<App showMore={true} />);
346+
});
347+
});
348+
expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
349+
expect(root).toMatchRenderedOutput('(empty)');
350+
351+
// This is a ping, not a retry, because no fallback is showing.
352+
expect(() =>
353+
resolveText('Async'),
354+
).toErrorDev(
355+
'A suspended resource finished loading inside a test, but the event ' +
356+
'was not wrapped in act(...)',
357+
{withoutStack: true},
358+
);
359+
});
360+
});
190361
});

0 commit comments

Comments
 (0)