Skip to content

Commit e8cdce4

Browse files
authored
Don't flush sync at end of discreteUpdates (#21327)
All it should do is change the priority. The updates will be flushed by the microtask.
1 parent a155860 commit e8cdce4

File tree

6 files changed

+41
-132
lines changed

6 files changed

+41
-132
lines changed

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

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ describe('ReactDOMFiberAsync', () => {
427427
});
428428

429429
// @gate experimental
430-
it('ignores discrete events on a pending removed event listener', () => {
430+
it('ignores discrete events on a pending removed event listener', async () => {
431431
const disableButtonRef = React.createRef();
432432
const submitButtonRef = React.createRef();
433433

@@ -459,17 +459,19 @@ describe('ReactDOMFiberAsync', () => {
459459
}
460460

461461
const root = ReactDOM.unstable_createRoot(container);
462-
root.render(<Form />);
463-
// Flush
464-
Scheduler.unstable_flushAll();
462+
await act(async () => {
463+
root.render(<Form />);
464+
});
465465

466466
const disableButton = disableButtonRef.current;
467467
expect(disableButton.tagName).toBe('BUTTON');
468468

469469
// Dispatch a click event on the Disable-button.
470470
const firstEvent = document.createEvent('Event');
471471
firstEvent.initEvent('click', true, true);
472-
disableButton.dispatchEvent(firstEvent);
472+
await act(async () => {
473+
disableButton.dispatchEvent(firstEvent);
474+
});
473475

474476
// There should now be a pending update to disable the form.
475477

@@ -481,14 +483,16 @@ describe('ReactDOMFiberAsync', () => {
481483
const secondEvent = document.createEvent('Event');
482484
secondEvent.initEvent('click', true, true);
483485
// This should force the pending update to flush which disables the submit button before the event is invoked.
484-
submitButton.dispatchEvent(secondEvent);
486+
await act(async () => {
487+
submitButton.dispatchEvent(secondEvent);
488+
});
485489

486490
// Therefore the form should never have been submitted.
487491
expect(formSubmitted).toBe(false);
488492
});
489493

490494
// @gate experimental
491-
it('uses the newest discrete events on a pending changed event listener', () => {
495+
it('uses the newest discrete events on a pending changed event listener', async () => {
492496
const enableButtonRef = React.createRef();
493497
const submitButtonRef = React.createRef();
494498

@@ -515,17 +519,19 @@ describe('ReactDOMFiberAsync', () => {
515519
}
516520

517521
const root = ReactDOM.unstable_createRoot(container);
518-
root.render(<Form />);
519-
// Flush
520-
Scheduler.unstable_flushAll();
522+
await act(async () => {
523+
root.render(<Form />);
524+
});
521525

522526
const enableButton = enableButtonRef.current;
523527
expect(enableButton.tagName).toBe('BUTTON');
524528

525529
// Dispatch a click event on the Enable-button.
526530
const firstEvent = document.createEvent('Event');
527531
firstEvent.initEvent('click', true, true);
528-
enableButton.dispatchEvent(firstEvent);
532+
await act(async () => {
533+
enableButton.dispatchEvent(firstEvent);
534+
});
529535

530536
// There should now be a pending update to enable the form.
531537

@@ -537,7 +543,9 @@ describe('ReactDOMFiberAsync', () => {
537543
const secondEvent = document.createEvent('Event');
538544
secondEvent.initEvent('click', true, true);
539545
// This should force the pending update to flush which enables the submit button before the event is invoked.
540-
submitButton.dispatchEvent(secondEvent);
546+
await act(async () => {
547+
submitButton.dispatchEvent(secondEvent);
548+
});
541549

542550
// Therefore the form should have been submitted.
543551
expect(formSubmitted).toBe(true);

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,9 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
357357
const pressEvent = document.createEvent('Event');
358358
pressEvent.initEvent('click', true, true);
359359
dispatchAndSetCurrentEvent(target.current, pressEvent);
360+
// Intentionally not using `act` so we can observe in between the press
361+
// event and the microtask, without batching.
362+
await null;
360363
// If this is 2, that means the `setCount` calls were not batched.
361364
expect(container.textContent).toEqual('Count: 1');
362365

@@ -409,11 +412,7 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
409412
dispatchAndSetCurrentEvent(target, pressEvent);
410413

411414
expect(Scheduler).toHaveYielded(['Count: 0 [after batchedUpdates]']);
412-
// TODO: There's a `flushDiscreteUpdates` call at the end of the event
413-
// delegation listener that gets called even if no React event handlers are
414-
// fired. Once that is removed, this will be 0, not 1.
415-
// expect(container.textContent).toEqual('Count: 0');
416-
expect(container.textContent).toEqual('Count: 1');
415+
expect(container.textContent).toEqual('Count: 0');
417416

418417
// Intentionally not using `act` so we can observe in between the click
419418
// event and the microtask, without batching.

packages/react-dom/src/events/ReactDOMUpdateBatching.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ function finishEventHandler() {
3939
// If a controlled event was fired, we may need to restore the state of
4040
// the DOM node back to the controlled value. This is necessary when React
4141
// bails out of the update without touching the DOM.
42+
// TODO: Restore state in the microtask, after the discrete updates flush,
43+
// instead of early flushing them here.
4244
flushDiscreteUpdatesImpl();
4345
restoreStateIfNeeded();
4446
}

packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js

Lines changed: 15 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ describe('SimpleEventPlugin', function() {
1313
let React;
1414
let ReactDOM;
1515
let Scheduler;
16-
let TestUtils;
16+
let act;
1717

1818
let onClick;
1919
let container;
@@ -40,7 +40,6 @@ describe('SimpleEventPlugin', function() {
4040
React = require('react');
4141
ReactDOM = require('react-dom');
4242
Scheduler = require('scheduler');
43-
TestUtils = require('react-dom/test-utils');
4443

4544
onClick = jest.fn();
4645
});
@@ -237,10 +236,12 @@ describe('SimpleEventPlugin', function() {
237236
React = require('react');
238237
ReactDOM = require('react-dom');
239238
Scheduler = require('scheduler');
239+
240+
act = require('react-dom/test-utils').unstable_concurrentAct;
240241
});
241242

242243
// @gate experimental
243-
it('flushes pending interactive work before exiting event handler', () => {
244+
it('flushes pending interactive work before exiting event handler', async () => {
244245
container = document.createElement('div');
245246
const root = ReactDOM.unstable_createRoot(container);
246247
document.body.appendChild(container);
@@ -288,7 +289,7 @@ describe('SimpleEventPlugin', function() {
288289
}
289290

290291
// Click the button to trigger the side-effect
291-
click();
292+
await act(async () => click());
292293
expect(Scheduler).toHaveYielded([
293294
// The handler fired
294295
'Side-effect',
@@ -312,6 +313,9 @@ describe('SimpleEventPlugin', function() {
312313
expect(Scheduler).toFlushAndYield([]);
313314
});
314315

316+
// NOTE: This test was written for the old behavior of discrete updates,
317+
// where they would be async, but flushed early if another discrete update
318+
// was dispatched.
315319
// @gate experimental
316320
it('end result of many interactive updates is deterministic', async () => {
317321
container = document.createElement('div');
@@ -355,121 +359,23 @@ describe('SimpleEventPlugin', function() {
355359
}
356360

357361
// Click the button a single time
358-
click();
362+
await act(async () => click());
359363
// The counter should update synchronously, even in concurrent mode.
360364
expect(button.textContent).toEqual('Count: 1');
361365

362366
// Click the button many more times
363-
await TestUtils.act(async () => {
364-
click();
365-
click();
366-
click();
367-
click();
368-
click();
369-
click();
370-
});
367+
await act(async () => click());
368+
await act(async () => click());
369+
await act(async () => click());
370+
await act(async () => click());
371+
await act(async () => click());
372+
await act(async () => click());
371373

372374
// Flush the remaining work
373375
Scheduler.unstable_flushAll();
374376
// The counter should equal the total number of clicks
375377
expect(button.textContent).toEqual('Count: 7');
376378
});
377-
378-
// @gate experimental
379-
it('flushes discrete updates in order', async () => {
380-
container = document.createElement('div');
381-
document.body.appendChild(container);
382-
383-
let button;
384-
class Button extends React.Component {
385-
state = {lowPriCount: 0};
386-
render() {
387-
const text = `High-pri count: ${this.props.highPriCount}, Low-pri count: ${this.state.lowPriCount}`;
388-
Scheduler.unstable_yieldValue(text);
389-
return (
390-
<button
391-
ref={el => (button = el)}
392-
onClick={() => {
393-
React.unstable_startTransition(() => {
394-
this.setState(state => ({
395-
lowPriCount: state.lowPriCount + 1,
396-
}));
397-
});
398-
}}>
399-
{text}
400-
</button>
401-
);
402-
}
403-
}
404-
405-
class Wrapper extends React.Component {
406-
state = {highPriCount: 0};
407-
render() {
408-
return (
409-
<div
410-
onClick={
411-
// Intentionally not using the updater form here, to test
412-
// that updates are serially processed.
413-
() => {
414-
this.setState({highPriCount: this.state.highPriCount + 1});
415-
}
416-
}>
417-
<Button highPriCount={this.state.highPriCount} />
418-
</div>
419-
);
420-
}
421-
}
422-
423-
// Initial mount
424-
const root = ReactDOM.unstable_createRoot(container);
425-
root.render(<Wrapper />);
426-
expect(Scheduler).toFlushAndYield([
427-
'High-pri count: 0, Low-pri count: 0',
428-
]);
429-
expect(button.textContent).toEqual('High-pri count: 0, Low-pri count: 0');
430-
431-
function click() {
432-
const event = new MouseEvent('click', {
433-
bubbles: true,
434-
cancelable: true,
435-
});
436-
Object.defineProperty(event, 'timeStamp', {
437-
value: 0,
438-
});
439-
button.dispatchEvent(event);
440-
}
441-
442-
// Click the button a single time.
443-
// This will flush at the end of the event, even in concurrent mode.
444-
click();
445-
expect(Scheduler).toHaveYielded(['High-pri count: 1, Low-pri count: 0']);
446-
447-
// Click the button many more times
448-
click();
449-
click();
450-
click();
451-
click();
452-
click();
453-
click();
454-
455-
// Each update should synchronously flush, even in concurrent mode.
456-
expect(Scheduler).toHaveYielded([
457-
'High-pri count: 2, Low-pri count: 0',
458-
'High-pri count: 3, Low-pri count: 0',
459-
'High-pri count: 4, Low-pri count: 0',
460-
'High-pri count: 5, Low-pri count: 0',
461-
'High-pri count: 6, Low-pri count: 0',
462-
'High-pri count: 7, Low-pri count: 0',
463-
]);
464-
465-
// Now flush the scheduler to apply the transition updates.
466-
// At the end, both counters should equal the total number of clicks.
467-
expect(Scheduler).toFlushAndYield([
468-
'High-pri count: 7, Low-pri count: 7',
469-
]);
470-
471-
expect(button.textContent).toEqual('High-pri count: 7, Low-pri count: 7');
472-
});
473379
});
474380

475381
describe('iOS bubbling click fix', function() {

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1160,9 +1160,6 @@ export function discreteUpdates<A, B, C, D, R>(
11601160
ReactCurrentBatchConfig.transition = prevTransition;
11611161
if (executionContext === NoContext) {
11621162
resetRenderTimer();
1163-
// TODO: This should only flush legacy sync updates. Not discrete updates
1164-
// in Concurrent Mode. Discrete updates will flush in a microtask.
1165-
flushSyncCallbacks();
11661163
}
11671164
}
11681165
}

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1160,9 +1160,6 @@ export function discreteUpdates<A, B, C, D, R>(
11601160
ReactCurrentBatchConfig.transition = prevTransition;
11611161
if (executionContext === NoContext) {
11621162
resetRenderTimer();
1163-
// TODO: This should only flush legacy sync updates. Not discrete updates
1164-
// in Concurrent Mode. Discrete updates will flush in a microtask.
1165-
flushSyncCallbacks();
11661163
}
11671164
}
11681165
}

0 commit comments

Comments
 (0)