@samitouri / QOS-React-2 / commits / c635807875

Support `use` in `act` testing API (#25523)

`use` can avoid suspending on already resolved data by yielding to microtasks. In a real, browser environment, we do this by scheduling a platform task (i.e. postTask). In a test environment, tasks are scheduled on a special internal queue so that they can be flushed by the `act` testing API. So we need to add support for this in `act`. This behavior only works if you `await` the thenable returned by the `act` call. We currently do not require that users do this. So I added a warning, but it only fires if `use` was called. The old Suspense pattern will not trigger a warning. This is to avoid breaking existing tests that use Suspense. The implementation of `act` has gotten extremely complicated because of the subtle changes in behavior over the years, and our commitment to maintaining backwards compatibility. We really should consider being more restrictive in a future major release. The changes are a bit confusing so I did my best to add inline comments explaining how it works. ## Test plan I ran this against Facebook's internal Jest test suite to confirm nothing broke

Andrew Clark committed Oct 20, 2022 at 22:08 UTC c635807875630e7057777e898372eed43e3b0a24
5 files changed +380 -118
packages/react-reconciler/src/ReactFiberWakeable.new.js
+7
@@ -15,6 +15,9 @@ import type {
15 RejectedThenable,
16 } from 'shared/ReactTypes';
17
18 +import ReactSharedInternals from 'shared/ReactSharedInternals';
19 +const {ReactCurrentActQueue} = ReactSharedInternals;
20 +
21 let suspendedThenable: Thenable<mixed> | null = null;
22 let adHocSuspendCount: number = 0;
23
@@ -124,6 +127,10 @@ export function trackUsedThenable<T>(thenable: Thenable<T>, index: number) {
127 }
128 usedThenables[index] = thenable;
129 lastUsedThenable = thenable;
130 +
131 + if (__DEV__ && ReactCurrentActQueue.current !== null) {
132 + ReactCurrentActQueue.didUsePromise = true;
133 + }
134 }
135
136 export function getPreviouslyUsedThenableAtIndex<T>(
packages/react-reconciler/src/ReactFiberWakeable.old.js
+7
@@ -15,6 +15,9 @@ import type {
15 RejectedThenable,
16 } from 'shared/ReactTypes';
17
18 +import ReactSharedInternals from 'shared/ReactSharedInternals';
19 +const {ReactCurrentActQueue} = ReactSharedInternals;
20 +
21 let suspendedThenable: Thenable<mixed> | null = null;
22 let adHocSuspendCount: number = 0;
23
@@ -124,6 +127,10 @@ export function trackUsedThenable<T>(thenable: Thenable<T>, index: number) {
127 }
128 usedThenables[index] = thenable;
129 lastUsedThenable = thenable;
130 +
131 + if (__DEV__ && ReactCurrentActQueue.current !== null) {
132 + ReactCurrentActQueue.didUsePromise = true;
133 + }
134 }
135
136 export function getPreviouslyUsedThenableAtIndex<T>(
packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js
+157
@@ -12,15 +12,22 @@
12 let React;
13 let ReactNoop;
14 let act;
15 +let use;
16 +let Suspense;
17 let DiscreteEventPriority;
18 +let startTransition;
19
20 describe('isomorphic act()', () => {
21 beforeEach(() => {
22 React = require('react');
23 +
24 ReactNoop = require('react-noop-renderer');
25 DiscreteEventPriority = require('react-reconciler/constants')
26 .DiscreteEventPriority;
27 act = React.unstable_act;
28 + use = React.experimental_use;
29 + Suspense = React.Suspense;
30 + startTransition = React.startTransition;
31 });
32
33 beforeEach(() => {
@@ -133,4 +140,154 @@ describe('isomorphic act()', () => {
140 expect(root).toMatchRenderedOutput('C');
141 });
142 });
143 +
144 + // @gate __DEV__
145 + // @gate enableUseHook
146 + test('unwraps promises by yielding to microtasks (async act scope)', async () => {
147 + const promise = Promise.resolve('Async');
148 +
149 + function Fallback() {
150 + throw new Error('Fallback should never be rendered');
151 + }
152 +
153 + function App() {
154 + return use(promise);
155 + }
156 +
157 + const root = ReactNoop.createRoot();
158 + await act(async () => {
159 + startTransition(() => {
160 + root.render(
161 + <Suspense fallback={<Fallback />}>
162 + <App />
163 + </Suspense>,
164 + );
165 + });
166 + });
167 + expect(root).toMatchRenderedOutput('Async');
168 + });
169 +
170 + // @gate __DEV__
171 + // @gate enableUseHook
172 + test('unwraps promises by yielding to microtasks (non-async act scope)', async () => {
173 + const promise = Promise.resolve('Async');
174 +
175 + function Fallback() {
176 + throw new Error('Fallback should never be rendered');
177 + }
178 +
179 + function App() {
180 + return use(promise);
181 + }
182 +
183 + const root = ReactNoop.createRoot();
184 +
185 + // Note that the scope function is not an async function
186 + await act(() => {
187 + startTransition(() => {
188 + root.render(
189 + <Suspense fallback={<Fallback />}>
190 + <App />
191 + </Suspense>,
192 + );
193 + });
194 + });
195 + expect(root).toMatchRenderedOutput('Async');
196 + });
197 +
198 + // @gate __DEV__
199 + // @gate enableUseHook
200 + test('warns if a promise is used in a non-awaited `act` scope', async () => {
201 + const promise = new Promise(() => {});
202 +
203 + function Fallback() {
204 + throw new Error('Fallback should never be rendered');
205 + }
206 +
207 + function App() {
208 + return use(promise);
209 + }
210 +
211 + spyOnDev(console, 'error');
212 + const root = ReactNoop.createRoot();
213 + act(() => {
214 + startTransition(() => {
215 + root.render(
216 + <Suspense fallback={<Fallback />}>
217 + <App />
218 + </Suspense>,
219 + );
220 + });
221 + });
222 +
223 + // `act` warns after a few microtasks, instead of a macrotask, so that it's
224 + // more likely to be attributed to the correct test case.
225 + //
226 + // The exact number of microtasks is an implementation detail; just needs
227 + // to happen when the microtask queue is flushed.
228 + await null;
229 + await null;
230 + await null;
231 +
232 + expect(console.error.calls.count()).toBe(1);
233 + expect(console.error.calls.argsFor(0)[0]).toContain(
234 + 'Warning: A component suspended inside an `act` scope, but the `act` ' +
235 + 'call was not awaited. When testing React components that ' +
236 + 'depend on asynchronous data, you must await the result:\n\n' +
237 + 'await act(() => ...)',
238 + );
239 + });
240 +
241 + // @gate __DEV__
242 + test('does not warn when suspending via legacy `throw` API in non-awaited `act` scope', async () => {
243 + let didResolve = false;
244 + let resolvePromise;
245 + const promise = new Promise(r => {
246 + resolvePromise = () => {
247 + didResolve = true;
248 + r();
249 + };
250 + });
251 +
252 + function Fallback() {
253 + return 'Loading...';
254 + }
255 +
256 + function App() {
257 + if (!didResolve) {
258 + throw promise;
259 + }
260 + return 'Async';
261 + }
262 +
263 + spyOnDev(console, 'error');
264 + const root = ReactNoop.createRoot();
265 + act(() => {
266 + startTransition(() => {
267 + root.render(
268 + <Suspense fallback={<Fallback />}>
269 + <App />
270 + </Suspense>,
271 + );
272 + });
273 + });
274 + expect(root).toMatchRenderedOutput('Loading...');
275 +
276 + // `act` warns after a few microtasks, instead of a macrotask, so that it's
277 + // more likely to be attributed to the correct test case.
278 + //
279 + // The exact number of microtasks is an implementation detail; just needs
280 + // to happen when the microtask queue is flushed.
281 + await null;
282 + await null;
283 + await null;
284 +
285 + expect(console.error.calls.count()).toBe(0);
286 +
287 + // Finish loading the data
288 + await act(async () => {
289 + resolvePromise();
290 + });
291 + expect(root).toMatchRenderedOutput('Async');
292 + });
293 });
packages/react/src/ReactAct.js
+203 -117
@@ -8,53 +8,72 @@
8 */
9
10 import type {Thenable} from 'shared/ReactTypes';
11 +import type {RendererTask} from './ReactCurrentActQueue';
12 import ReactCurrentActQueue from './ReactCurrentActQueue';
12 -import enqueueTask from 'shared/enqueueTask';
13 +import queueMacrotask from 'shared/enqueueTask';
14
15 +// `act` calls can be nested, so we track the depth. This represents the
16 +// number of `act` scopes on the stack.
17 let actScopeDepth = 0;
18 +
19 +// We only warn the first time you neglect to await an async `act` scope.
20 let didWarnNoAwaitAct = false;
21
22 export function act<T>(callback: () => T | Thenable<T>): Thenable<T> {
23 if (__DEV__) {
19 - // `act` calls can be nested, so we track the depth. This represents the
20 - // number of `act` scopes on the stack.
24 + // When ReactCurrentActQueue.current is not null, it signals to React that
25 + // we're currently inside an `act` scope. React will push all its tasks to
26 + // this queue instead of scheduling them with platform APIs.
27 + //
28 + // We set this to an empty array when we first enter an `act` scope, and
29 + // only unset it once we've left the outermost `act` scope — remember that
30 + // `act` calls can be nested.
31 + //
32 + // If we're already inside an `act` scope, reuse the existing queue.
33 + const prevIsBatchingLegacy = ReactCurrentActQueue.isBatchingLegacy;
34 + const prevActQueue = ReactCurrentActQueue.current;
35 const prevActScopeDepth = actScopeDepth;
36 actScopeDepth++;
37 + const queue = (ReactCurrentActQueue.current =
38 + prevActQueue !== null ? prevActQueue : []);
39 + // Used to reproduce behavior of `batchedUpdates` in legacy mode. Only
40 + // set to `true` while the given callback is executed, not for updates
41 + // triggered during an async event, because this is how the legacy
42 + // implementation of `act` behaved.
43 + ReactCurrentActQueue.isBatchingLegacy = true;
44
24 - if (ReactCurrentActQueue.current === null) {
25 - // This is the outermost `act` scope. Initialize the queue. The reconciler
26 - // will detect the queue and use it instead of Scheduler.
27 - ReactCurrentActQueue.current = [];
28 - }
29 -
30 - const prevIsBatchingLegacy = ReactCurrentActQueue.isBatchingLegacy;
45 let result;
46 + // This tracks whether the `act` call is awaited. In certain cases, not
47 + // awaiting it is a mistake, so we will detect that and warn.
48 + let didAwaitActCall = false;
49 try {
33 - // Used to reproduce behavior of `batchedUpdates` in legacy mode. Only
34 - // set to `true` while the given callback is executed, not for updates
35 - // triggered during an async event, because this is how the legacy
36 - // implementation of `act` behaved.
37 - ReactCurrentActQueue.isBatchingLegacy = true;
50 + // Reset this to `false` right before entering the React work loop. The
51 + // only place we ever read this fields is just below, right after running
52 + // the callback. So we don't need to reset after the callback runs.
53 + ReactCurrentActQueue.didScheduleLegacyUpdate = false;
54 result = callback();
55 + const didScheduleLegacyUpdate =
56 + ReactCurrentActQueue.didScheduleLegacyUpdate;
57
58 // Replicate behavior of original `act` implementation in legacy mode,
59 // which flushed updates immediately after the scope function exits, even
60 // if it's an async function.
43 - if (
44 - !prevIsBatchingLegacy &&
45 - ReactCurrentActQueue.didScheduleLegacyUpdate
46 - ) {
47 - const queue = ReactCurrentActQueue.current;
48 - if (queue !== null) {
49 - ReactCurrentActQueue.didScheduleLegacyUpdate = false;
50 - flushActQueue(queue);
51 - }
61 + if (!prevIsBatchingLegacy && didScheduleLegacyUpdate) {
62 + flushActQueue(queue);
63 }
64 + // `isBatchingLegacy` gets reset using the regular stack, not the async
65 + // one used to track `act` scopes. Why, you may be wondering? Because
66 + // that's how it worked before version 18. Yes, it's confusing! We should
67 + // delete legacy mode!!
68 + ReactCurrentActQueue.isBatchingLegacy = prevIsBatchingLegacy;
69 } catch (error) {
54 - popActScope(prevActScopeDepth);
55 - throw error;
56 - } finally {
70 + // `isBatchingLegacy` gets reset using the regular stack, not the async
71 + // one used to track `act` scopes. Why, you may be wondering? Because
72 + // that's how it worked before version 18. Yes, it's confusing! We should
73 + // delete legacy mode!!
74 ReactCurrentActQueue.isBatchingLegacy = prevIsBatchingLegacy;
75 + popActScope(prevActQueue, prevActScopeDepth);
76 + throw error;
77 }
78
79 if (
@@ -63,99 +82,130 @@ export function act<T>(callback: () => T | Thenable<T>): Thenable<T> {
82 // $FlowFixMe[method-unbinding]
83 typeof result.then === 'function'
84 ) {
66 - const thenableResult: Thenable<T> = (result: any);
67 - // The callback is an async function (i.e. returned a promise). Wait
68 - // for it to resolve before exiting the current scope.
69 - let wasAwaited = false;
70 - const thenable: Thenable<T> = {
85 + // A promise/thenable was returned from the callback. Wait for it to
86 + // resolve before flushing the queue.
87 + //
88 + // If `act` were implemented as an async function, this whole block could
89 + // be a single `await` call. That's really the only difference between
90 + // this branch and the next one.
91 + const thenable = ((result: any): Thenable<T>);
92 +
93 + // Warn if the an `act` call with an async scope is not awaited. In a
94 + // future release, consider making this an error.
95 + queueSeveralMicrotasks(() => {
96 + if (!didAwaitActCall && !didWarnNoAwaitAct) {
97 + didWarnNoAwaitAct = true;
98 + console.error(
99 + 'You called act(async () => ...) without await. ' +
100 + 'This could lead to unexpected testing behaviour, ' +
101 + 'interleaving multiple act calls and mixing their ' +
102 + 'scopes. ' +
103 + 'You should - await act(async () => ...);',
104 + );
105 + }
106 + });
107 +
108 + return {
109 then(resolve, reject) {
72 - wasAwaited = true;
73 - thenableResult.then(
110 + didAwaitActCall = true;
111 + thenable.then(
112 returnValue => {
75 - popActScope(prevActScopeDepth);
76 - if (actScopeDepth === 0) {
77 - // We've exited the outermost act scope. Recursively flush the
78 - // queue until there's no remaining work.
79 - recursivelyFlushAsyncActWork(returnValue, resolve, reject);
113 + popActScope(prevActQueue, prevActScopeDepth);
114 + if (prevActScopeDepth === 0) {
115 + // We're exiting the outermost `act` scope. Flush the queue.
116 + try {
117 + flushActQueue(queue);
118 + queueMacrotask(() =>
119 + // Recursively flush tasks scheduled by a microtask.
120 + recursivelyFlushAsyncActWork(returnValue, resolve, reject),
121 + );
122 + } catch (error) {
123 + // `thenable` might not be a real promise, and `flushActQueue`
124 + // might throw, so we need to wrap `flushActQueue` in a
125 + // try/catch.
126 + reject(error);
127 + }
128 } else {
129 resolve(returnValue);
130 }
131 },
132 error => {
85 - // The callback threw an error.
86 - popActScope(prevActScopeDepth);
133 + popActScope(prevActQueue, prevActScopeDepth);
134 reject(error);
135 },
136 );
137 },
138 };
92 -
93 - if (__DEV__) {
94 - if (!didWarnNoAwaitAct && typeof Promise !== 'undefined') {
95 - // eslint-disable-next-line no-undef
96 - Promise.resolve()
97 - .then(() => {})
98 - .then(() => {
99 - if (!wasAwaited) {
100 - didWarnNoAwaitAct = true;
101 - console.error(
102 - 'You called act(async () => ...) without await. ' +
103 - 'This could lead to unexpected testing behaviour, ' +
104 - 'interleaving multiple act calls and mixing their ' +
105 - 'scopes. ' +
106 - 'You should - await act(async () => ...);',
107 - );
108 - }
109 - });
110 - }
111 - }
112 - return thenable;
139 } else {
140 const returnValue: T = (result: any);
115 - // The callback is not an async function. Exit the current scope
116 - // immediately, without awaiting.
117 - popActScope(prevActScopeDepth);
118 - if (actScopeDepth === 0) {
119 - // Exiting the outermost act scope. Flush the queue.
120 - const queue = ReactCurrentActQueue.current;
121 - if (queue !== null) {
122 - flushActQueue(queue);
123 - ReactCurrentActQueue.current = null;
124 - }
125 - // Return a thenable. If the user awaits it, we'll flush again in
126 - // case additional work was scheduled by a microtask.
127 - const thenable: Thenable<T> = {
128 - then(resolve, reject) {
129 - // Confirm we haven't re-entered another `act` scope, in case
130 - // the user does something weird like await the thenable
131 - // multiple times.
132 - if (ReactCurrentActQueue.current === null) {
133 - // Recursively flush the queue until there's no remaining work.
134 - ReactCurrentActQueue.current = [];
135 - recursivelyFlushAsyncActWork(returnValue, resolve, reject);
136 - } else {
137 - resolve(returnValue);
141 + // The callback is not an async function. Exit the current
142 + // scope immediately.
143 + popActScope(prevActQueue, prevActScopeDepth);
144 + if (prevActScopeDepth === 0) {
145 + // We're exiting the outermost `act` scope. Flush the queue.
146 + flushActQueue(queue);
147 +
148 + // If the queue is not empty, it implies that we intentionally yielded
149 + // to the main thread, because something suspended. We will continue
150 + // in an asynchronous task.
151 + //
152 + // Warn if something suspends but the `act` call is not awaited.
153 + // In a future release, consider making this an error.
154 + if (queue.length !== 0) {
155 + queueSeveralMicrotasks(() => {
156 + if (!didAwaitActCall && !didWarnNoAwaitAct) {
157 + didWarnNoAwaitAct = true;
158 + console.error(
159 + 'A component suspended inside an `act` scope, but the ' +
160 + '`act` call was not awaited. When testing React ' +
161 + 'components that depend on asynchronous data, you must ' +
162 + 'await the result:\n\n' +
163 + 'await act(() => ...)',
164 + );
165 }
139 - },
140 - };
141 - return thenable;
142 - } else {
143 - // Since we're inside a nested `act` scope, the returned thenable
144 - // immediately resolves. The outer scope will flush the queue.
145 - const thenable: Thenable<T> = {
146 - then(resolve, reject) {
147 - resolve(returnValue);
148 - },
149 - };
150 - return thenable;
166 + });
167 + }
168 +
169 + // Like many things in this module, this is next part is confusing.
170 + //
171 + // We do not currently require every `act` call that is passed a
172 + // callback to be awaited, through arguably we should. Since this
173 + // callback was synchronous, we need to exit the current scope before
174 + // returning.
175 + //
176 + // However, if thenable we're about to return *is* awaited, we'll
177 + // immediately restore the current scope. So it shouldn't observable.
178 + //
179 + // This doesn't affect the case where the scope callback is async,
180 + // because we always require those calls to be awaited.
181 + //
182 + // TODO: In a future version, consider always requiring all `act` calls
183 + // to be awaited, regardless of whether the callback is sync or async.
184 + ReactCurrentActQueue.current = null;
185 }
186 + return {
187 + then(resolve, reject) {
188 + didAwaitActCall = true;
189 + if (prevActScopeDepth === 0) {
190 + // If the `act` call is awaited, restore the queue we were
191 + // using before (see long comment above) so we can flush it.
192 + ReactCurrentActQueue.current = queue;
193 + queueMacrotask(() =>
194 + // Recursively flush tasks scheduled by a microtask.
195 + recursivelyFlushAsyncActWork(returnValue, resolve, reject),
196 + );
197 + } else {
198 + resolve(returnValue);
199 + }
200 + },
201 + };
202 }
203 } else {
204 throw new Error('act(...) is not supported in production builds of React.');
205 }
206 }
207
158 -function popActScope(prevActScopeDepth) {
208 +function popActScope(prevActQueue, prevActScopeDepth) {
209 if (__DEV__) {
210 if (prevActScopeDepth !== actScopeDepth - 1) {
211 console.error(
@@ -173,22 +223,27 @@ function recursivelyFlushAsyncActWork<T>(
223 reject: mixed => mixed,
224 ) {
225 if (__DEV__) {
226 + // Check if any tasks were scheduled asynchronously.
227 const queue = ReactCurrentActQueue.current;
228 if (queue !== null) {
178 - try {
179 - flushActQueue(queue);
180 - enqueueTask(() => {
181 - if (queue.length === 0) {
182 - // No additional work was scheduled. Finish.
183 - ReactCurrentActQueue.current = null;
184 - resolve(returnValue);
185 - } else {
186 - // Keep flushing work until there's none left.
187 - recursivelyFlushAsyncActWork(returnValue, resolve, reject);
188 - }
189 - });
190 - } catch (error) {
191 - reject(error);
229 + if (queue.length !== 0) {
230 + // Async tasks were scheduled, mostly likely in a microtask.
231 + // Keep flushing until there are no more.
232 + try {
233 + flushActQueue(queue);
234 + // The work we just performed may have schedule additional async
235 + // tasks. Wait a macrotask and check again.
236 + queueMacrotask(() =>
237 + recursivelyFlushAsyncActWork(returnValue, resolve, reject),
238 + );
239 + } catch (error) {
240 + // Leave remaining tasks on the queue if something throws.
241 + reject(error);
242 + }
243 + } else {
244 + // The queue is empty. We can finish.
245 + ReactCurrentActQueue.current = null;
246 + resolve(returnValue);
247 }
248 } else {
249 resolve(returnValue);
@@ -205,16 +260,30 @@ function flushActQueue(queue) {
260 let i = 0;
261 try {
262 for (; i < queue.length; i++) {
208 - let callback = queue[i];
263 + let callback: RendererTask = queue[i];
264 do {
210 - // $FlowFixMe[incompatible-type] found when upgrading Flow
211 - callback = callback(true);
212 - } while (callback !== null);
265 + ReactCurrentActQueue.didUsePromise = false;
266 + const continuation = callback(false);
267 + if (continuation !== null) {
268 + if (ReactCurrentActQueue.didUsePromise) {
269 + // The component just suspended. Yield to the main thread in
270 + // case the promise is already resolved. If so, it will ping in
271 + // a microtask and we can resume without unwinding the stack.
272 + queue[i] = callback;
273 + queue.splice(0, i);
274 + return;
275 + }
276 + callback = continuation;
277 + } else {
278 + break;
279 + }
280 + } while (true);
281 }
282 + // We flushed the entire queue.
283 queue.length = 0;
284 } catch (error) {
285 // If something throws, leave the remaining callbacks on the queue.
217 - queue = queue.slice(i + 1);
286 + queue.splice(0, i + 1);
287 throw error;
288 } finally {
289 isFlushing = false;
@@ -222,3 +291,20 @@ function flushActQueue(queue) {
291 }
292 }
293 }
294 +
295 +// Some of our warnings attempt to detect if the `act` call is awaited by
296 +// checking in an asynchronous task. Wait a few microtasks before checking. The
297 +// only reason one isn't sufficient is we want to accommodate the case where an
298 +// `act` call is returned from an async function without first being awaited,
299 +// since that's a somewhat common pattern. If you do this too many times in a
300 +// nested sequence, you might get a warning, but you can always fix by awaiting
301 +// the call.
302 +//
303 +// A macrotask would also work (and is the fallback) but depending on the test
304 +// environment it may cause the warning to fire too late.
305 +const queueSeveralMicrotasks =
306 + typeof queueMicrotask === 'function'
307 + ? callback => {
308 + queueMicrotask(() => queueMicrotask(callback));
309 + }
310 + : queueMacrotask;
packages/react/src/ReactCurrentActQueue.js
+6 -1
@@ -7,7 +7,7 @@
7 * @flow
8 */
9
10 -type RendererTask = boolean => RendererTask | null;
10 +export type RendererTask = boolean => RendererTask | null;
11
12 const ReactCurrentActQueue = {
13 current: (null: null | Array<RendererTask>),
@@ -15,6 +15,11 @@ const ReactCurrentActQueue = {
15 // Used to reproduce behavior of `batchedUpdates` in legacy mode.
16 isBatchingLegacy: false,
17 didScheduleLegacyUpdate: false,
18 +
19 + // Tracks whether something called `use` during the current batch of work.
20 + // Determines whether we should yield to microtasks to unwrap already resolved
21 + // promises without suspending.
22 + didUsePromise: false,
23 };
24
25 export default ReactCurrentActQueue;