Re-add discrete flushing timeStamp heuristic (behind flag) (#19540)

Author: trueadm

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

@@ -1031,6 +1031,17 @@ describe('ReactDOMFiber', () => {
     const handlerA = () => ops.push('A');
     const handlerB = () => ops.push('B');
 
+    function click() {
+      const event = new MouseEvent('click', {
+        bubbles: true,
+        cancelable: true,
+      });
+      Object.defineProperty(event, 'timeStamp', {
+        value: 0,
+      });
+      node.dispatchEvent(event);
+    }
+
     class Example extends React.Component {
       state = {flip: false, count: 0};
       flip() {
@@ -1064,23 +1075,23 @@ describe('ReactDOMFiber', () => {
     const node = container.firstChild;
     expect(node.tagName).toEqual('DIV');
 
-    node.click();
+    click();
 
     expect(ops).toEqual(['A']);
     ops = [];
 
     // Render with the other event handler.
     inst.flip();
 
-    node.click();
+    click();
 
     expect(ops).toEqual(['B']);
     ops = [];
 
     // Rerender without changing any props.
     inst.tick();
 
-    node.click();
+    click();
 
     expect(ops).toEqual(['B']);
     ops = [];
@@ -1100,7 +1111,7 @@ describe('ReactDOMFiber', () => {
     ops = [];
 
     // Any click that happens after commit, should invoke A.
-    node.click();
+    click();
     expect(ops).toEqual(['A']);
   });
 

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

@@ -130,7 +130,7 @@ function dispatchDiscreteEvent(
     // flushed for this event and we don't need to do it again.
     (eventSystemFlags & IS_LEGACY_FB_SUPPORT_MODE) === 0
   ) {
-    flushDiscreteUpdatesIfNeeded();
+    flushDiscreteUpdatesIfNeeded(nativeEvent.timeStamp);
   }
   discreteUpdates(
     dispatchEvent,

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

@@ -9,6 +9,7 @@ import {
   needsStateRestore,
   restoreStateIfNeeded,
 } from './ReactDOMControlledComponent';
+import {enableDiscreteEventFlushingChange} from 'shared/ReactFeatureFlags';
 
 // Used as a way to call batchedUpdates when we don't have a reference to
 // the renderer. Such as when we're dispatching events or if third party
@@ -87,9 +88,32 @@ export function discreteUpdates(fn, a, b, c, d) {
   }
 }
 
-export function flushDiscreteUpdatesIfNeeded() {
-  if (!isInsideEventHandler) {
-    flushDiscreteUpdatesImpl();
+let lastFlushedEventTimeStamp = 0;
+export function flushDiscreteUpdatesIfNeeded(timeStamp: number) {
+  if (enableDiscreteEventFlushingChange) {
+    // event.timeStamp isn't overly reliable due to inconsistencies in
+    // how different browsers have historically provided the time stamp.
+    // Some browsers provide high-resolution time stamps for all events,
+    // some provide low-resolution time stamps for all events. FF < 52
+    // even mixes both time stamps together. Some browsers even report
+    // negative time stamps or time stamps that are 0 (iOS9) in some cases.
+    // Given we are only comparing two time stamps with equality (!==),
+    // we are safe from the resolution differences. If the time stamp is 0
+    // we bail-out of preventing the flush, which can affect semantics,
+    // such as if an earlier flush removes or adds event listeners that
+    // are fired in the subsequent flush. However, this is the same
+    // behaviour as we had before this change, so the risks are low.
+    if (
+      !isInsideEventHandler &&
+      (timeStamp === 0 || lastFlushedEventTimeStamp !== timeStamp)
+    ) {
+      lastFlushedEventTimeStamp = timeStamp;
+      flushDiscreteUpdatesImpl();
+    }
+  } else {
+    if (!isInsideEventHandler) {
+      flushDiscreteUpdatesImpl();
+    }
   }
 }
 

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

@@ -275,9 +275,14 @@ describe('SimpleEventPlugin', function() {
       expect(Scheduler).toFlushAndYield(['render button: enabled']);
 
       function click() {
-        button.dispatchEvent(
-          new MouseEvent('click', {bubbles: true, cancelable: true}),
-        );
+        const event = new MouseEvent('click', {
+          bubbles: true,
+          cancelable: true,
+        });
+        Object.defineProperty(event, 'timeStamp', {
+          value: 0,
+        });
+        button.dispatchEvent(event);
       }
 
       // Click the button to trigger the side-effect
@@ -340,9 +345,14 @@ describe('SimpleEventPlugin', function() {
       expect(button.textContent).toEqual('Count: 0');
 
       function click() {
-        button.dispatchEvent(
-          new MouseEvent('click', {bubbles: true, cancelable: true}),
-        );
+        const event = new MouseEvent('click', {
+          bubbles: true,
+          cancelable: true,
+        });
+        Object.defineProperty(event, 'timeStamp', {
+          value: 0,
+        });
+        button.dispatchEvent(event);
       }
 
       // Click the button a single time
@@ -421,9 +431,14 @@ describe('SimpleEventPlugin', function() {
       expect(button.textContent).toEqual('High-pri count: 0, Low-pri count: 0');
 
       function click() {
-        button.dispatchEvent(
-          new MouseEvent('click', {bubbles: true, cancelable: true}),
-        );
+        const event = new MouseEvent('click', {
+          bubbles: true,
+          cancelable: true,
+        });
+        Object.defineProperty(event, 'timeStamp', {
+          value: 0,
+        });
+        button.dispatchEvent(event);
       }
 
       // Click the button a single time

packages/shared/ReactFeatureFlags.js

@@ -125,3 +125,5 @@ export const deferRenderPhaseUpdateToNextBatch = true;
 
 // Replacement for runWithPriority in React internals.
 export const decoupleUpdatePriorityFromScheduler = false;
+
+export const enableDiscreteEventFlushingChange = false;

packages/shared/forks/ReactFeatureFlags.native-fb.js

@@ -48,6 +48,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = false;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.native-oss.js

@@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = false;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.test-renderer.js

@@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = false;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.test-renderer.native.js

@@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = false;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.test-renderer.www.js

@@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = false;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.testing.js

@@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = false;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.testing.www.js

@@ -47,6 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 export const decoupleUpdatePriorityFromScheduler = false;
+export const enableDiscreteEventFlushingChange = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars

packages/shared/forks/ReactFeatureFlags.www.js

@@ -75,6 +75,8 @@ export const disableTextareaChildren = __EXPERIMENTAL__;
 
 export const warnUnstableRenderSubtreeIntoContainer = false;
 
+export const enableDiscreteEventFlushingChange = true;
+
 // Enable forked reconciler. Piggy-backing on the "variant" global so that we
 // don't have to add another test dimension. The build system will compile this
 // to the correct value.