Unify InputDiscreteLane with SyncLane (facebook#20968)

* Unify sync priority and input discrete * Fix lint * Use update lane instead * Update sync lane labels
koto · Jun 15, 2021 · 81880dc · 81880dc
1 parent 82337d5
commit 81880dc
Show file tree

Hide file tree

Showing 9 changed files with 31 additions and 85 deletions.
diff --git 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 (
           <div>
             <button onClick={disableForm} ref={disableButtonRef}>
               Disable
             </button>
-            {active ? (
-              <button onClick={submitForm} ref={submitButtonRef}>
-                Submit
-              </button>
-            ) : null}
+            {active ? <button ref={submitButtonRef}>Submit</button> : null}
           </div>
         );
       }
 
       const root = ReactDOM.unstable_createRoot(container);
-      root.render(<Form />);
-      // Flush
-      Scheduler.unstable_flushAll();
+      await act(async () => {
+        root.render(<Form />);
+      });
 
       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
@@ -67,9 +67,9 @@ describe('ReactDOMNativeEventHeuristic-test', () => {
     }
 
     const root = ReactDOM.unstable_createRoot(container);
-    root.render(<Form />);
-    // Flush
-    Scheduler.unstable_flushAll();
+    await act(() => {
+      root.render(<Form />);
+    });
 
     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
@@ -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
@@ -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/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
@@ -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
@@ -90,7 +90,6 @@ import {
   clearContainer,
   getCurrentEventPriority,
   supportsMicrotasks,
-  scheduleMicrotask,
 } from './ReactFiberHostConfig';
 
 import {
@@ -737,12 +736,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
@@ -90,7 +90,6 @@ import {
   clearContainer,
   getCurrentEventPriority,
   supportsMicrotasks,
-  scheduleMicrotask,
 } from './ReactFiberHostConfig';
 
 import {
@@ -737,12 +736,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/__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`,
     );
   });