The library for web and native user interfaces.
act: Move didScheduleLegacyUpdate to ensureRootIsScheduled (#26552)
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before #26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after #26512 landed. Will add a regression test before merging.
Andrew Clark committed
Apr 10, 2023 at 14:40 UTC
fec97ecbc4bc2e0e1407160289a8f5fac5241cbc
4 files changed
+80
-15
packages/react-reconciler/src/ReactFiberRootScheduler.js
+9
@@ -119,6 +119,15 @@ export function ensureRootIsScheduled(root: FiberRoot): void {
119
// unblock additional features we have planned.
120
scheduleTaskForRootDuringMicrotask(root, now());
121
}
122
+
123
+ if (
124
+ __DEV__ &&
125
+ ReactCurrentActQueue.isBatchingLegacy &&
126
+ root.tag === LegacyRoot
127
+ ) {
128
+ // Special `act` case: Record whenever a legacy update is scheduled.
129
+ ReactCurrentActQueue.didScheduleLegacyUpdate = true;
130
+ }
131
}
132
133
export function flushSyncWorkOnAllRoots() {
packages/react-reconciler/src/ReactFiberWorkLoop.js
-1
@@ -828,7 +828,6 @@ export function scheduleUpdateOnFiber(
828
) {
829
if (__DEV__ && ReactCurrentActQueue.isBatchingLegacy) {
830
// Treat `act` as if it's inside `batchedUpdates`, even in legacy mode.
831
- ReactCurrentActQueue.didScheduleLegacyUpdate = true;
831
} else {
832
// Flush the synchronous work now, unless we're already working or inside
833
// a batch. This is intentionally inside scheduleUpdateOnFiber instead of
packages/react-reconciler/src/__tests__/ReactIsomorphicAct-test.js
+68
-11
@@ -17,10 +17,13 @@ let Suspense;
17
let DiscreteEventPriority;
18
let startTransition;
19
let waitForMicrotasks;
20
+let Scheduler;
21
+let assertLog;
22
23
describe('isomorphic act()', () => {
24
beforeEach(() => {
25
React = require('react');
26
+ Scheduler = require('scheduler');
27
28
ReactNoop = require('react-noop-renderer');
29
DiscreteEventPriority =
@@ -31,6 +34,7 @@ describe('isomorphic act()', () => {
34
startTransition = React.startTransition;
35
36
waitForMicrotasks = require('internal-test-utils').waitForMicrotasks;
37
+ assertLog = require('internal-test-utils').assertLog;
38
});
39
40
beforeEach(() => {
@@ -41,6 +45,11 @@ describe('isomorphic act()', () => {
45
jest.restoreAllMocks();
46
});
47
48
+ function Text({text}) {
49
+ Scheduler.log(text);
50
+ return text;
51
+ }
52
+
53
// @gate __DEV__
54
test('bypasses queueMicrotask', async () => {
55
const root = ReactNoop.createRoot();
@@ -132,19 +141,67 @@ describe('isomorphic act()', () => {
141
const root = ReactNoop.createLegacyRoot();
142
143
await act(async () => {
135
- // These updates are batched. This replicates the behavior of the original
136
- // `act` implementation, for compatibility.
137
- root.render('A');
138
- root.render('B');
139
- // Nothing has rendered yet.
140
- expect(root).toMatchRenderedOutput(null);
144
+ queueMicrotask(() => {
145
+ Scheduler.log('Current tree in microtask: ' + root.getChildrenAsJSX());
146
+ root.render(<Text text="C" />);
147
+ });
148
+ root.render(<Text text="A" />);
149
+ root.render(<Text text="B" />);
150
+
151
await null;
142
- // Updates are flushed after the first await.
143
- expect(root).toMatchRenderedOutput('B');
152
+ assertLog([
153
+ // A and B should render in a single batch _before_ the microtask queue
154
+ // has run. This replicates the behavior of the original `act`
155
+ // implementation, for compatibility.
156
+ 'B',
157
+ 'Current tree in microtask: B',
158
+
159
+ // C isn't scheduled until a microtask, so it's rendered separately.
160
+ 'C',
161
+ ]);
162
+
163
+ // Subsequent updates should also render in separate batches.
164
+ root.render(<Text text="D" />);
165
+ root.render(<Text text="E" />);
166
+ assertLog(['D', 'E']);
167
+ });
168
+ });
169
145
- // Subsequent updates in the same scope aren't batched.
146
- root.render('C');
147
- expect(root).toMatchRenderedOutput('C');
170
+ // @gate __DEV__
171
+ test('in legacy mode, in an async scope, updates are batched until the first `await` (regression test: batchedUpdates)', async () => {
172
+ const root = ReactNoop.createLegacyRoot();
173
+
174
+ await act(async () => {
175
+ queueMicrotask(() => {
176
+ Scheduler.log('Current tree in microtask: ' + root.getChildrenAsJSX());
177
+ root.render(<Text text="C" />);
178
+ });
179
+
180
+ // This is a regression test. The presence of `batchedUpdates` would cause
181
+ // these updates to not flush until a microtask. The correct behavior is
182
+ // that they flush before the microtask queue, regardless of whether
183
+ // they are wrapped with `batchedUpdates`.
184
+ ReactNoop.batchedUpdates(() => {
185
+ root.render(<Text text="A" />);
186
+ root.render(<Text text="B" />);
187
+ });
188
+
189
+ await null;
190
+ assertLog([
191
+ // A and B should render in a single batch _before_ the microtask queue
192
+ // has run. This replicates the behavior of the original `act`
193
+ // implementation, for compatibility.
194
+ 'B',
195
+ 'Current tree in microtask: B',
196
+
197
+ // C isn't scheduled until a microtask, so it's rendered separately.
198
+ 'C',
199
+ ]);
200
+
201
+ // Subsequent updates should also render in separate batches.
202
+ root.render(<Text text="D" />);
203
+ root.render(<Text text="E" />);
204
+ assertLog(['D', 'E']);
205
});
206
});
207
scripts/jest/matchers/reactTestMatchers.js
+3
-3
@@ -20,13 +20,13 @@ function captureAssertion(fn) {
20
return {pass: true};
21
}
22
23
-function assertYieldsWereCleared(Scheduler) {
23
+function assertYieldsWereCleared(Scheduler, caller) {
24
const actualYields = Scheduler.unstable_clearLog();
25
if (actualYields.length !== 0) {
26
const error = Error(
27
'The event log is not empty. Call assertLog(...) first.'
28
);
29
- Error.captureStackTrace(error, assertYieldsWereCleared);
29
+ Error.captureStackTrace(error, caller);
30
throw error;
31
}
32
}
@@ -34,7 +34,7 @@ function assertYieldsWereCleared(Scheduler) {
34
function toMatchRenderedOutput(ReactNoop, expectedJSX) {
35
if (typeof ReactNoop.getChildrenAsJSX === 'function') {
36
const Scheduler = ReactNoop._Scheduler;
37
- assertYieldsWereCleared(Scheduler);
37
+ assertYieldsWereCleared(Scheduler, toMatchRenderedOutput);
38
return captureAssertion(() => {
39
expect(ReactNoop.getChildrenAsJSX()).toEqual(expectedJSX);
40
});