From af0e07fdc2861aed746394b2eee944db8f7ab606 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Tue, 9 Mar 2021 19:01:11 -0700 Subject: [PATCH 1/4] Unify sync priority and input discrete --- .../src/__tests__/ReactDOMFiberAsync-test.js | 41 +++++-------------- .../ReactDOMNativeEventHeuristic-test.js | 11 ++--- .../__tests__/ChangeEventPlugin-test.js | 5 +-- .../__tests__/SimpleEventPlugin-test.js | 37 ++++++----------- .../src/ReactFiberWorkLoop.new.js | 11 ++--- .../src/ReactFiberWorkLoop.old.js | 11 ++--- 6 files changed, 36 insertions(+), 80 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index ee609d7c2e0c5..7fde6e5ddeb6b 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -386,63 +386,44 @@ describe('ReactDOMFiberAsync', () => { }); // @gate experimental - it('ignores discrete events on a pending removed element', () => { + it('ignores discrete events on a pending removed element', async () => { const disableButtonRef = React.createRef(); const submitButtonRef = React.createRef(); - let formSubmitted = false; - function Form() { const [active, setActive] = React.useState(true); function disableForm() { setActive(false); } - function submitForm() { - formSubmitted = true; // This should not get invoked - } + return (
- {active ? ( - - ) : null} + {active ? : null}
); } const root = ReactDOM.unstable_createRoot(container); - root.render(
); - // Flush - Scheduler.unstable_flushAll(); + await act(async () => { + root.render(); + }); const disableButton = disableButtonRef.current; expect(disableButton.tagName).toBe('BUTTON'); + const submitButton = submitButtonRef.current; + expect(submitButton.tagName).toBe('BUTTON'); + // Dispatch a click event on the Disable-button. const firstEvent = document.createEvent('Event'); firstEvent.initEvent('click', true, true); disableButton.dispatchEvent(firstEvent); - // There should now be a pending update to disable the form. - - // This should not have flushed yet since it's in concurrent mode. - const submitButton = submitButtonRef.current; - expect(submitButton.tagName).toBe('BUTTON'); - - // In the meantime, we can dispatch a new client event on the submit button. - const secondEvent = document.createEvent('Event'); - secondEvent.initEvent('click', true, true); - // This should force the pending update to flush which disables the submit button before the event is invoked. - submitButton.dispatchEvent(secondEvent); - - // Therefore the form should never have been submitted. - expect(formSubmitted).toBe(false); - - expect(submitButtonRef.current).toBe(null); + // The click event is flushed synchronously, even in concurrent mode. + expect(submitButton.current).toBe(undefined); }); // @gate experimental diff --git a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js index 7c87698b1a35b..dd682d2def59f 100644 --- a/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMNativeEventHeuristic-test.js @@ -67,9 +67,9 @@ describe('ReactDOMNativeEventHeuristic-test', () => { } const root = ReactDOM.unstable_createRoot(container); - root.render(); - // Flush - Scheduler.unstable_flushAll(); + await act(() => { + root.render(); + }); const disableButton = disableButtonRef.current; expect(disableButton.tagName).toBe('BUTTON'); @@ -81,11 +81,6 @@ describe('ReactDOMNativeEventHeuristic-test', () => { dispatchAndSetCurrentEvent(disableButton, firstEvent), ).toErrorDev(['An update to Form inside a test was not wrapped in act']); - // There should now be a pending update to disable the form. - // This should not have flushed yet since it's in concurrent mode. - const submitButton = submitButtonRef.current; - expect(submitButton.tagName).toBe('BUTTON'); - // Discrete events should be flushed in a microtask. // Verify that the second button was removed. await null; diff --git a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js index 26630b68b11fe..9b7fed1468470 100644 --- a/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js +++ b/packages/react-dom/src/events/plugins/__tests__/ChangeEventPlugin-test.js @@ -685,7 +685,7 @@ describe('ChangeEventPlugin', () => { }); // @gate experimental - it('is async for non-input events', async () => { + it('is sync for non-input events', async () => { const root = ReactDOM.unstable_createRoot(container); let input; @@ -724,9 +724,6 @@ describe('ChangeEventPlugin', () => { input.dispatchEvent( new Event('click', {bubbles: true, cancelable: true}), ); - // Nothing should have changed - expect(Scheduler).toHaveYielded([]); - expect(input.value).toBe('initial'); // Flush microtask queue. await null; diff --git a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js index 9af14ccd86809..95ea1cc598a5e 100644 --- a/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js +++ b/packages/react-dom/src/events/plugins/__tests__/SimpleEventPlugin-test.js @@ -240,7 +240,7 @@ describe('SimpleEventPlugin', function() { }); // @gate experimental - it('flushes pending interactive work before extracting event handler', () => { + it('flushes pending interactive work before exiting event handler', () => { container = document.createElement('div'); const root = ReactDOM.unstable_createRoot(container); document.body.appendChild(container); @@ -292,17 +292,14 @@ describe('SimpleEventPlugin', function() { expect(Scheduler).toHaveYielded([ // The handler fired 'Side-effect', - // but the component did not re-render yet, because it's async + // The component re-rendered synchronously, even in concurrent mode. + 'render button: disabled', ]); // Click the button again click(); expect(Scheduler).toHaveYielded([ - // Before handling this second click event, the previous interactive - // update is flushed - 'render button: disabled', - // The event handler was removed from the button, so there's no second - // side-effect + // The event handler was removed from the button, so there's no effect. ]); // The handler should not fire again no matter how many times we @@ -359,8 +356,8 @@ describe('SimpleEventPlugin', function() { // Click the button a single time click(); - // The counter should not have updated yet because it's async - expect(button.textContent).toEqual('Count: 0'); + // The counter should update synchronously, even in concurrent mode. + expect(button.textContent).toEqual('Count: 1'); // Click the button many more times await TestUtils.act(async () => { @@ -442,15 +439,10 @@ describe('SimpleEventPlugin', function() { button.dispatchEvent(event); } - // Click the button a single time - click(); - // Nothing should flush on the first click. - expect(Scheduler).toHaveYielded([]); - // Click again. This will force the previous discrete update to flush. But - // only the high-pri count will increase. + // Click the button a single time. + // This will flush at the end of the event, even in concurrent mode. click(); expect(Scheduler).toHaveYielded(['High-pri count: 1, Low-pri count: 0']); - expect(button.textContent).toEqual('High-pri count: 1, Low-pri count: 0'); // Click the button many more times click(); @@ -460,7 +452,7 @@ describe('SimpleEventPlugin', function() { click(); click(); - // Flush the remaining work. + // Each update should synchronously flush, even in concurrent mode. expect(Scheduler).toHaveYielded([ 'High-pri count: 2, Low-pri count: 0', 'High-pri count: 3, Low-pri count: 0', @@ -470,16 +462,13 @@ describe('SimpleEventPlugin', function() { 'High-pri count: 7, Low-pri count: 0', ]); - // Flush the microtask queue - await null; - - // At the end, both counters should equal the total number of clicks - expect(Scheduler).toHaveYielded(['High-pri count: 8, Low-pri count: 0']); + // Now flush the scheduler to apply the transition updates. + // At the end, both counters should equal the total number of clicks. expect(Scheduler).toFlushAndYield([ - 'High-pri count: 8, Low-pri count: 8', + 'High-pri count: 7, Low-pri count: 7', ]); - expect(button.textContent).toEqual('High-pri count: 8, Low-pri count: 8'); + expect(button.textContent).toEqual('High-pri count: 7, Low-pri count: 7'); }); }); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 5b6d174316434..168962a20fb4c 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -727,7 +727,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // Schedule a new callback. let newCallbackNode; - if (newCallbackPriority === SyncLanePriority) { + if ( + newCallbackPriority === SyncLanePriority || + newCallbackPriority === InputDiscreteLanePriority + ) { // Special case: Sync React callbacks are scheduled on a special // internal queue scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); @@ -737,12 +740,6 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { ImmediateSchedulerPriority, performSyncWorkOnRoot.bind(null, root), ); - } else if ( - supportsMicrotasks && - newCallbackPriority === InputDiscreteLanePriority - ) { - scheduleMicrotask(performSyncWorkOnRoot.bind(null, root)); - newCallbackNode = null; } else { const schedulerPriorityLevel = lanePriorityToSchedulerPriority( newCallbackPriority, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index a41d723dfe1ab..6d558c676774a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -727,7 +727,10 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // Schedule a new callback. let newCallbackNode; - if (newCallbackPriority === SyncLanePriority) { + if ( + newCallbackPriority === SyncLanePriority || + newCallbackPriority === InputDiscreteLanePriority + ) { // Special case: Sync React callbacks are scheduled on a special // internal queue scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); @@ -737,12 +740,6 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { ImmediateSchedulerPriority, performSyncWorkOnRoot.bind(null, root), ); - } else if ( - supportsMicrotasks && - newCallbackPriority === InputDiscreteLanePriority - ) { - scheduleMicrotask(performSyncWorkOnRoot.bind(null, root)); - newCallbackNode = null; } else { const schedulerPriorityLevel = lanePriorityToSchedulerPriority( newCallbackPriority, From d9272206b93e935c456d18b0b2cf8274400adf00 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Tue, 9 Mar 2021 19:09:04 -0700 Subject: [PATCH 2/4] Fix lint --- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 1 - packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 1 - 2 files changed, 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 168962a20fb4c..482562d4f234f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -90,7 +90,6 @@ import { clearContainer, getCurrentEventPriority, supportsMicrotasks, - scheduleMicrotask, } from './ReactFiberHostConfig'; import { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 6d558c676774a..363cb22c2f3fc 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -90,7 +90,6 @@ import { clearContainer, getCurrentEventPriority, supportsMicrotasks, - scheduleMicrotask, } from './ReactFiberHostConfig'; import { From b12fb3334ae610d44626573be305091b0230a06a Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Wed, 10 Mar 2021 15:21:58 -0700 Subject: [PATCH 3/4] Use update lane instead --- packages/react-reconciler/src/ReactFiberLane.new.js | 2 +- packages/react-reconciler/src/ReactFiberLane.old.js | 2 +- packages/react-reconciler/src/ReactFiberWorkLoop.new.js | 5 +---- packages/react-reconciler/src/ReactFiberWorkLoop.old.js | 5 +---- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index ef74b355ebfad..4a0e2a151bcc7 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -566,7 +566,7 @@ export function findUpdateLane(lanePriority: LanePriority): Lane { case SyncBatchedLanePriority: return SyncBatchedLane; case InputDiscreteLanePriority: - return InputDiscreteLane; + return SyncLane; case InputContinuousLanePriority: return InputContinuousLane; case DefaultLanePriority: diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index 4e2ce8942f08a..28a5e2e542218 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -566,7 +566,7 @@ export function findUpdateLane(lanePriority: LanePriority): Lane { case SyncBatchedLanePriority: return SyncBatchedLane; case InputDiscreteLanePriority: - return InputDiscreteLane; + return SyncLane; case InputContinuousLanePriority: return InputContinuousLane; case DefaultLanePriority: diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 482562d4f234f..8f83cf88fea9b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -726,10 +726,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // Schedule a new callback. let newCallbackNode; - if ( - newCallbackPriority === SyncLanePriority || - newCallbackPriority === InputDiscreteLanePriority - ) { + if (newCallbackPriority === SyncLanePriority) { // Special case: Sync React callbacks are scheduled on a special // internal queue scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 363cb22c2f3fc..4b59de6b0d5d6 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -726,10 +726,7 @@ function ensureRootIsScheduled(root: FiberRoot, currentTime: number) { // Schedule a new callback. let newCallbackNode; - if ( - newCallbackPriority === SyncLanePriority || - newCallbackPriority === InputDiscreteLanePriority - ) { + if (newCallbackPriority === SyncLanePriority) { // Special case: Sync React callbacks are scheduled on a special // internal queue scheduleSyncCallback(performSyncWorkOnRoot.bind(null, root)); From ffba3d8fc196b8daa073ee2fbff80fbeb669866e Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Wed, 10 Mar 2021 15:28:48 -0700 Subject: [PATCH 4/4] Update sync lane labels --- .../src/__tests__/SchedulingProfilerLabels-test.internal.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js index d06371940b470..6474be855b38f 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js @@ -140,9 +140,7 @@ describe('SchedulingProfiler labels', () => { targetRef.current.click(); }); expect(clearedMarks).toContain( - `--schedule-state-update-${formatLanes( - ReactFiberLane.InputDiscreteLane, - )}-App`, + `--schedule-state-update-${formatLanes(ReactFiberLane.SyncLane)}-App`, ); });