@samitouri / QOS-React-2 / commits / 89acfa639b

Fix native event batching in concurrent mode (#21010)

* Fix native event batching in concurrent mode * Wrap DevTools test updates with act These tests expect the `scheduleUpdate` DevTools hook to trigger a synchronous re-render with legacy semantics, but flushing in a microtask is fine. Wrapping the updates with `act` fixes it. * Testing nits * Nit: Check executionContext === NoContext first In the common case it will be false and the binary expression will short circuit. Co-authored-by: Andrew Clark <git@andrewclark.io>

Ricky committed Mar 16, 2021 at 13:15 UTC 89acfa639bcecebdb18276ba2c0f4d5beee2de19

4 files changed +70 -16

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

+14 -14

@@ -201,32 +201,32 @@ describe('React hooks DevTools integration', () => {

201 if (__DEV__) {

202 // First render was locked

203 expect(renderer.toJSON().children).toEqual(['Loading']);

204 - scheduleUpdate(fiber); // Re-render

204 + act(() => scheduleUpdate(fiber)); // Re-render

205 expect(renderer.toJSON().children).toEqual(['Loading']);

206

207 // Release the lock

208 setSuspenseHandler(() => false);

209 - scheduleUpdate(fiber); // Re-render

209 + act(() => scheduleUpdate(fiber)); // Re-render

210 expect(renderer.toJSON().children).toEqual(['Done']);

211 - scheduleUpdate(fiber); // Re-render

211 + act(() => scheduleUpdate(fiber)); // Re-render

212 expect(renderer.toJSON().children).toEqual(['Done']);

213

214 // Lock again

215 setSuspenseHandler(() => true);

216 - scheduleUpdate(fiber); // Re-render

216 + act(() => scheduleUpdate(fiber)); // Re-render

217 expect(renderer.toJSON().children).toEqual(['Loading']);

218

219 // Release the lock again

220 setSuspenseHandler(() => false);

221 - scheduleUpdate(fiber); // Re-render

221 + act(() => scheduleUpdate(fiber)); // Re-render

222 expect(renderer.toJSON().children).toEqual(['Done']);

223

224 // Ensure it checks specific fibers.

225 setSuspenseHandler(f => f === fiber || f === fiber.alternate);

226 - scheduleUpdate(fiber); // Re-render

226 + act(() => scheduleUpdate(fiber)); // Re-render

227 expect(renderer.toJSON().children).toEqual(['Loading']);

228 setSuspenseHandler(f => f !== fiber && f !== fiber.alternate);

229 - scheduleUpdate(fiber); // Re-render

229 + act(() => scheduleUpdate(fiber)); // Re-render

230 expect(renderer.toJSON().children).toEqual(['Done']);

231 } else {

232 expect(renderer.toJSON().children).toEqual(['Done']);

@@ -259,33 +259,33 @@ describe('React hooks DevTools integration', () => {

259 if (__DEV__) {

260 // First render was locked

261 expect(renderer.toJSON().children).toEqual(['Loading']);

262 - scheduleUpdate(fiber); // Re-render

262 + act(() => scheduleUpdate(fiber)); // Re-render

263 expect(renderer.toJSON().children).toEqual(['Loading']);

264

265 // Release the lock

266 setSuspenseHandler(() => false);

267 - scheduleUpdate(fiber); // Re-render

267 + act(() => scheduleUpdate(fiber)); // Re-render

268 Scheduler.unstable_flushAll();

269 expect(renderer.toJSON().children).toEqual(['Done']);

270 - scheduleUpdate(fiber); // Re-render

270 + act(() => scheduleUpdate(fiber)); // Re-render

271 expect(renderer.toJSON().children).toEqual(['Done']);

272

273 // Lock again

274 setSuspenseHandler(() => true);

275 - scheduleUpdate(fiber); // Re-render

275 + act(() => scheduleUpdate(fiber)); // Re-render

276 expect(renderer.toJSON().children).toEqual(['Loading']);

277

278 // Release the lock again

279 setSuspenseHandler(() => false);

280 - scheduleUpdate(fiber); // Re-render

280 + act(() => scheduleUpdate(fiber)); // Re-render

281 expect(renderer.toJSON().children).toEqual(['Done']);

282

283 // Ensure it checks specific fibers.

284 setSuspenseHandler(f => f === fiber || f === fiber.alternate);

285 - scheduleUpdate(fiber); // Re-render

285 + act(() => scheduleUpdate(fiber)); // Re-render

286 expect(renderer.toJSON().children).toEqual(['Loading']);

287 setSuspenseHandler(f => f !== fiber && f !== fiber.alternate);

288 - scheduleUpdate(fiber); // Re-render

288 + act(() => scheduleUpdate(fiber)); // Re-render

289 expect(renderer.toJSON().children).toEqual(['Done']);

290 } else {

291 expect(renderer.toJSON().children).toEqual(['Done']);

packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js

+48

@@ -281,4 +281,52 @@ describe('ReactDOMNativeEventHeuristic-test', () => {

281 expect(container.textContent).toEqual('hovered');

282 });

283 });

284 +

285 + // @gate experimental

286 + it('should batch inside native events', async () => {

287 + const root = ReactDOM.unstable_createRoot(container);

288 +

289 + const target = React.createRef(null);

290 + function Foo() {

291 + const [count, setCount] = React.useState(0);

292 + const countRef = React.useRef(-1);

293 +

294 + React.useLayoutEffect(() => {

295 + countRef.current = count;

296 + target.current.onclick = () => {

297 + setCount(countRef.current + 1);

298 + // Now update again. If these updates are batched, then this should be

299 + // a no-op, because we didn't re-render yet and `countRef` hasn't

300 + // been mutated.

301 + setCount(countRef.current + 1);

302 + };

303 + });

304 + return <div ref={target}>Count: {count}</div>;

305 + }

306 +

307 + await act(async () => {

308 + root.render(<Foo />);

309 + });

310 + expect(container.textContent).toEqual('Count: 0');

311 +

312 + // Ignore act warning. We can't use act because it forces batched updates.

313 + spyOnDev(console, 'error');

314 +

315 + const pressEvent = document.createEvent('Event');

316 + pressEvent.initEvent('click', true, true);

317 + dispatchAndSetCurrentEvent(target.current, pressEvent);

318 + // If this is 2, that means the `setCount` calls were not batched.

319 + expect(container.textContent).toEqual('Count: 1');

320 +

321 + // Assert that the `act` warnings were the only ones that fired.

322 + if (__DEV__) {

323 + expect(console.error).toHaveBeenCalledTimes(2);

324 + expect(console.error.calls.argsFor(0)[0]).toContain(

325 + 'was not wrapped in act',

326 + );

327 + expect(console.error.calls.argsFor(1)[0]).toContain(

328 + 'was not wrapped in act',

329 + );

330 + }

331 + });

332 });

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

+4 -1

@@ -547,7 +547,10 @@ export function scheduleUpdateOnFiber(

547 } else {

548 ensureRootIsScheduled(root, eventTime);

549 schedulePendingInteractions(root, lane);

550 - if (executionContext === NoContext) {

550 + if (

551 + executionContext === NoContext &&

552 + (fiber.mode & ConcurrentMode) === NoMode

553 + ) {

554 // Flush the synchronous work now, unless we're already working or inside

555 // a batch. This is intentionally inside scheduleUpdateOnFiber instead of

556 // scheduleCallbackForFiber to preserve the ability to schedule a callback

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