[Flare] Update interactiveUpdates flushing heuristics (#15687)

Author: trueadm

packages/events/ReactGenericBatching.js

@@ -17,15 +17,33 @@ import {
 // scheduled work and instead do synchronous work.
 
 // Defaults
-let _batchedUpdatesImpl = function(fn, bookkeeping) {
+let batchedUpdatesImpl = function(fn, bookkeeping) {
   return fn(bookkeeping);
 };
-let _interactiveUpdatesImpl = function(fn, a, b, c) {
+let discreteUpdatesImpl = function(fn, a, b, c) {
   return fn(a, b, c);
 };
-let _flushInteractiveUpdatesImpl = function() {};
+let flushDiscreteUpdatesImpl = function() {};
+let batchedEventUpdatesImpl = batchedUpdatesImpl;
 
 let isBatching = false;
+
+function batchedUpdatesFinally() {
+  // Here we wait until all updates have propagated, which is important
+  // when using controlled components within layers:
+  // https://github.com/facebook/react/issues/1698
+  // Then we restore state of any controlled component.
+  isBatching = false;
+  const controlledComponentsHavePendingUpdates = needsStateRestore();
+  if (controlledComponentsHavePendingUpdates) {
+    // If a controlled event was fired, we may need to restore the state of
+    // the DOM node back to the controlled value. This is necessary when React
+    // bails out of the update without touching the DOM.
+    flushDiscreteUpdatesImpl();
+    restoreStateIfNeeded();
+  }
+}
+
 export function batchedUpdates(fn, bookkeeping) {
   if (isBatching) {
     // If we are currently inside another batch, we need to wait until it
@@ -34,38 +52,42 @@ export function batchedUpdates(fn, bookkeeping) {
   }
   isBatching = true;
   try {
-    return _batchedUpdatesImpl(fn, bookkeeping);
+    return batchedUpdatesImpl(fn, bookkeeping);
+  } finally {
+    batchedUpdatesFinally();
+  }
+}
+
+export function batchedEventUpdates(fn, bookkeeping) {
+  if (isBatching) {
+    // If we are currently inside another batch, we need to wait until it
+    // fully completes before restoring state.
+    return fn(bookkeeping);
+  }
+  isBatching = true;
+  try {
+    return batchedEventUpdatesImpl(fn, bookkeeping);
   } finally {
-    // Here we wait until all updates have propagated, which is important
-    // when using controlled components within layers:
-    // https://github.com/facebook/react/issues/1698
-    // Then we restore state of any controlled component.
-    isBatching = false;
-    const controlledComponentsHavePendingUpdates = needsStateRestore();
-    if (controlledComponentsHavePendingUpdates) {
-      // If a controlled event was fired, we may need to restore the state of
-      // the DOM node back to the controlled value. This is necessary when React
-      // bails out of the update without touching the DOM.
-      _flushInteractiveUpdatesImpl();
-      restoreStateIfNeeded();
-    }
+    batchedUpdatesFinally();
   }
 }
 
-export function interactiveUpdates(fn, a, b, c) {
-  return _interactiveUpdatesImpl(fn, a, b, c);
+export function discreteUpdates(fn, a, b, c) {
+  return discreteUpdatesImpl(fn, a, b, c);
 }
 
-export function flushInteractiveUpdates() {
-  return _flushInteractiveUpdatesImpl();
+export function flushDiscreteUpdates() {
+  return flushDiscreteUpdatesImpl();
 }
 
 export function setBatchingImplementation(
-  batchedUpdatesImpl,
-  interactiveUpdatesImpl,
-  flushInteractiveUpdatesImpl,
+  _batchedUpdatesImpl,
+  _discreteUpdatesImpl,
+  _flushDiscreteUpdatesImpl,
+  _batchedEventUpdatesImpl,
 ) {
-  _batchedUpdatesImpl = batchedUpdatesImpl;
-  _interactiveUpdatesImpl = interactiveUpdatesImpl;
-  _flushInteractiveUpdatesImpl = flushInteractiveUpdatesImpl;
+  batchedUpdatesImpl = _batchedUpdatesImpl;
+  discreteUpdatesImpl = _discreteUpdatesImpl;
+  flushDiscreteUpdatesImpl = _flushDiscreteUpdatesImpl;
+  batchedEventUpdatesImpl = _batchedEventUpdatesImpl;
 }

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

@@ -1026,9 +1026,15 @@ describe('ReactDOMFiber', () => {
 
   it('should not update event handlers until commit', () => {
     let ops = [];
+    let eventErrors = [];
     const handlerA = () => ops.push('A');
     const handlerB = () => ops.push('B');
 
+    spyOnProd(console, 'error');
+    window.addEventListener('error', e => {
+      eventErrors.push(e.message);
+    });
+
     class Example extends React.Component {
       state = {flip: false, count: 0};
       flip() {
@@ -1090,12 +1096,16 @@ describe('ReactDOMFiber', () => {
 
     // Because the new click handler has not yet committed, we should still
     // invoke B.
-    expect(ops).toEqual(['B']);
+    expect(ops).toEqual([]);
     ops = [];
 
     // Any click that happens after commit, should invoke A.
     node.click();
     expect(ops).toEqual(['A']);
+    expect(eventErrors[0]).toEqual(
+      'unstable_flushDiscreteUpdates: Cannot flush ' +
+        'updates when React is already rendering.',
+    );
   });
 
   it('should not crash encountering low-priority tree', () => {

packages/react-dom/src/client/ReactDOM.js

@@ -26,10 +26,11 @@ import {
   flushRoot,
   createContainer,
   updateContainer,
+  batchedEventUpdates,
   batchedUpdates,
   unbatchedUpdates,
-  interactiveUpdates,
-  flushInteractiveUpdates,
+  discreteUpdates,
+  flushDiscreteUpdates,
   flushSync,
   flushControlled,
   injectIntoDevTools,
@@ -481,8 +482,9 @@ function shouldHydrateDueToLegacyHeuristic(container) {
 
 setBatchingImplementation(
   batchedUpdates,
-  interactiveUpdates,
-  flushInteractiveUpdates,
+  discreteUpdates,
+  flushDiscreteUpdates,
+  batchedEventUpdates,
 );
 
 let warnedAboutHydrateAPI = false;
@@ -783,7 +785,14 @@ const ReactDOM: Object = {
 
   unstable_batchedUpdates: batchedUpdates,
 
-  unstable_interactiveUpdates: interactiveUpdates,
+  // TODO remove this legacy method, unstable_discreteUpdates replaces it
+  unstable_interactiveUpdates: (fn, a, b, c) => {
+    flushDiscreteUpdates();
+    return discreteUpdates(fn, a, b, c);
+  },
+
+  unstable_discreteUpdates: discreteUpdates,
+  unstable_flushDiscreteUpdates: flushDiscreteUpdates,
 
   flushSync: flushSync,
 

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

@@ -25,7 +25,11 @@ import type {
   ReactResponderEvent,
 } from 'shared/ReactTypes';
 import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
-import {batchedUpdates, interactiveUpdates} from 'events/ReactGenericBatching';
+import {
+  batchedEventUpdates,
+  discreteUpdates,
+  flushDiscreteUpdates,
+} from 'events/ReactGenericBatching';
 import type {Fiber} from 'react-reconciler/src/ReactFiber';
 import warning from 'shared/warning';
 import {enableEventAPI} from 'shared/ReactFeatureFlags';
@@ -601,11 +605,14 @@ export function processEventQueue(): void {
     return;
   }
   if (discrete) {
-    interactiveUpdates(() => {
-      batchedUpdates(processEvents, events);
+    if (shouldflushDiscreteUpdates(currentTimeStamp)) {
+      flushDiscreteUpdates();
+    }
+    discreteUpdates(() => {
+      batchedEventUpdates(processEvents, events);
     });
   } else {
-    batchedUpdates(processEvents, events);
+    batchedEventUpdates(processEvents, events);
   }
 }
 
@@ -1004,3 +1011,25 @@ export function generateListeningKey(
   const passiveKey = passive ? '_passive' : '_active';
   return `${topLevelType}${passiveKey}`;
 }
+
+let lastDiscreteEventTimeStamp = 0;
+
+export function shouldflushDiscreteUpdates(timeStamp: number): boolean {
+  // 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-resoltion 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 (timeStamp === 0 || lastDiscreteEventTimeStamp !== timeStamp) {
+    lastDiscreteEventTimeStamp = timeStamp;
+    return true;
+  }
+  return false;
+}

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

@@ -11,9 +11,16 @@ import type {AnyNativeEvent} from 'events/PluginModuleType';
 import type {Fiber} from 'react-reconciler/src/ReactFiber';
 import type {DOMTopLevelEventType} from 'events/TopLevelEventTypes';
 
-import {batchedUpdates, interactiveUpdates} from 'events/ReactGenericBatching';
+import {
+  batchedEventUpdates,
+  discreteUpdates,
+  flushDiscreteUpdates,
+} from 'events/ReactGenericBatching';
 import {runExtractedPluginEventsInBatch} from 'events/EventPluginHub';
-import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem';
+import {
+  dispatchEventForResponderEventSystem,
+  shouldflushDiscreteUpdates,
+} from '../events/DOMEventResponderSystem';
 import {isFiberMounted} from 'react-reconciler/reflection';
 import {HostRoot} from 'shared/ReactWorkTags';
 import {
@@ -188,7 +195,7 @@ export function trapEventForResponderEventSystem(
     } else {
       eventFlags |= IS_ACTIVE;
     }
-    // Check if interactive and wrap in interactiveUpdates
+    // Check if interactive and wrap in discreteUpdates
     const listener = dispatchEvent.bind(null, topLevelType, eventFlags);
     if (passiveBrowserEventsSupported) {
       addEventCaptureListenerWithPassiveFlag(
@@ -212,7 +219,7 @@ function trapEventForPluginEventSystem(
     ? dispatchInteractiveEvent
     : dispatchEvent;
   const rawEventName = getRawEventName(topLevelType);
-  // Check if interactive and wrap in interactiveUpdates
+  // Check if interactive and wrap in discreteUpdates
   const listener = dispatch.bind(null, topLevelType, PLUGIN_EVENT_SYSTEM);
   if (capture) {
     addEventCaptureListener(element, rawEventName, listener);
@@ -222,12 +229,10 @@ function trapEventForPluginEventSystem(
 }
 
 function dispatchInteractiveEvent(topLevelType, eventSystemFlags, nativeEvent) {
-  interactiveUpdates(
-    dispatchEvent,
-    topLevelType,
-    eventSystemFlags,
-    nativeEvent,
-  );
+  if (!enableEventAPI || shouldflushDiscreteUpdates(nativeEvent.timeStamp)) {
+    flushDiscreteUpdates();
+  }
+  discreteUpdates(dispatchEvent, topLevelType, eventSystemFlags, nativeEvent);
 }
 
 function dispatchEventForPluginEventSystem(
@@ -245,7 +250,7 @@ function dispatchEventForPluginEventSystem(
   try {
     // Event queue being processed in the same cycle allows
     // `preventDefault`.
-    batchedUpdates(handleTopLevel, bookKeeping);
+    batchedEventUpdates(handleTopLevel, bookKeeping);
   } finally {
     releaseTopLevelCallbackBookKeeping(bookKeeping);
   }

packages/react-dom/src/fire/ReactFire.js

@@ -31,10 +31,11 @@ import {
   flushRoot,
   createContainer,
   updateContainer,
+  batchedEventUpdates,
   batchedUpdates,
   unbatchedUpdates,
-  interactiveUpdates,
-  flushInteractiveUpdates,
+  discreteUpdates,
+  flushDiscreteUpdates,
   flushSync,
   flushControlled,
   injectIntoDevTools,
@@ -487,8 +488,9 @@ function shouldHydrateDueToLegacyHeuristic(container) {
 
 setBatchingImplementation(
   batchedUpdates,
-  interactiveUpdates,
-  flushInteractiveUpdates,
+  discreteUpdates,
+  flushDiscreteUpdates,
+  batchedEventUpdates,
 );
 
 let warnedAboutHydrateAPI = false;
@@ -789,7 +791,14 @@ const ReactDOM: Object = {
 
   unstable_batchedUpdates: batchedUpdates,
 
-  unstable_interactiveUpdates: interactiveUpdates,
+  // TODO remove this legacy method, unstable_discreteUpdates replaces it
+  unstable_interactiveUpdates: (fn, a, b, c) => {
+    flushDiscreteUpdates();
+    return discreteUpdates(fn, a, b, c);
+  },
+
+  unstable_discreteUpdates: discreteUpdates,
+  unstable_flushDiscreteUpdates: flushDiscreteUpdates,
 
   flushSync: flushSync,