Address feedback for accumulateTwoPhaseListeners (facebook#18289)

trueadm · web-flow · commit d3ec42020d22 · 2020-03-12T21:48:29.000Z
diff --git a/packages/legacy-events/EventPropagators.js b/packages/legacy-events/EventPropagators.js
@@ -3,30 +3,19 @@
  *
  * This source code is licensed under the MIT license found in the
  * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
  */
 
-import {
-  getParentInstance,
-  traverseTwoPhase,
-  traverseEnterLeave,
-} from 'shared/ReactTreeTraversal';
+import type {Fiber} from 'react-reconciler/src/ReactFiber';
+import type {ReactSyntheticEvent} from 'legacy-events/ReactSyntheticEventType';
 
 import getListener from 'legacy-events/getListener';
+
+import {traverseEnterLeave} from 'shared/ReactTreeTraversal';
 import accumulateInto from './accumulateInto';
 import forEachAccumulated from './forEachAccumulated';
 
-type PropagationPhases = 'bubbled' | 'captured';
-
-/**
- * Some event types have a notion of different registration names for different
- * "phases" of propagation. This finds listeners by a given phase.
- */
-function listenerAtPhase(inst, event, propagationPhase: PropagationPhases) {
-  const registrationName =
-    event.dispatchConfig.phasedRegistrationNames[propagationPhase];
-  return getListener(inst, registrationName);
-}
-
 /**
  * A small set of propagation patterns, each of which will accept a small amount
  * of information, and generate a set of "dispatch ready event objects" - which
@@ -37,58 +26,16 @@ function listenerAtPhase(inst, event, propagationPhase: PropagationPhases) {
  * single one.
  */
 
-/**
- * Tags a `SyntheticEvent` with dispatched listeners. Creating this function
- * here, allows us to not have to bind or create functions for each event.
- * Mutating the event's members allows us to not have to create a wrapping
- * "dispatch" object that pairs the event with the listener.
- */
-function accumulateDirectionalDispatches(inst, phase, event) {
-  if (__DEV__) {
-    if (!inst) {
-      console.error('Dispatching inst must not be null');
-    }
-  }
-  const listener = listenerAtPhase(inst, event, phase);
-  if (listener) {
-    event._dispatchListeners = accumulateInto(
-      event._dispatchListeners,
-      listener,
-    );
-    event._dispatchInstances = accumulateInto(event._dispatchInstances, inst);
-  }
-}
-
-/**
- * Collect dispatches (must be entirely collected before dispatching - see unit
- * tests). Lazily allocate the array to conserve memory.  We must loop through
- * each event and perform the traversal for each one. We cannot perform a
- * single traversal for the entire collection of events because each event may
- * have a different target.
- */
-export function accumulateTwoPhaseDispatchesSingle(event) {
-  if (event && event.dispatchConfig.phasedRegistrationNames) {
-    traverseTwoPhase(event._targetInst, accumulateDirectionalDispatches, event);
-  }
-}
-
-/**
- * Same as `accumulateTwoPhaseDispatchesSingle`, but skips over the targetID.
- */
-function accumulateTwoPhaseDispatchesSingleSkipTarget(event) {
-  if (event && event.dispatchConfig.phasedRegistrationNames) {
-    const targetInst = event._targetInst;
-    const parentInst = targetInst ? getParentInstance(targetInst) : null;
-    traverseTwoPhase(parentInst, accumulateDirectionalDispatches, event);
-  }
-}
-
 /**
  * Accumulates without regard to direction, does not look for phased
  * registration names. Same as `accumulateDirectDispatchesSingle` but without
  * requiring that the `dispatchMarker` be the same as the dispatched ID.
  */
-function accumulateDispatches(inst, ignoredDirection, event) {
+function accumulateDispatches(
+  inst: Fiber,
+  ignoredDirection: ?boolean,
+  event: ReactSyntheticEvent,
+): void {
   if (inst && event && event.dispatchConfig.registrationName) {
     const registrationName = event.dispatchConfig.registrationName;
     const listener = getListener(inst, registrationName);
@@ -107,24 +54,23 @@ function accumulateDispatches(inst, ignoredDirection, event) {
  * `dispatchMarker`.
  * @param {SyntheticEvent} event
  */
-function accumulateDirectDispatchesSingle(event) {
+function accumulateDirectDispatchesSingle(event: ReactSyntheticEvent) {
   if (event && event.dispatchConfig.registrationName) {
     accumulateDispatches(event._targetInst, null, event);
   }
 }
 
-export function accumulateTwoPhaseDispatches(events) {
-  forEachAccumulated(events, accumulateTwoPhaseDispatchesSingle);
-}
-
-export function accumulateTwoPhaseDispatchesSkipTarget(events) {
-  forEachAccumulated(events, accumulateTwoPhaseDispatchesSingleSkipTarget);
-}
-
-export function accumulateEnterLeaveDispatches(leave, enter, from, to) {
+export function accumulateEnterLeaveDispatches(
+  leave: ReactSyntheticEvent,
+  enter: ReactSyntheticEvent,
+  from: Fiber,
+  to: Fiber,
+) {
   traverseEnterLeave(from, to, accumulateDispatches, leave, enter);
 }
 
-export function accumulateDirectDispatches(events) {
+export function accumulateDirectDispatches(
+  events: ?(Array<ReactSyntheticEvent> | ReactSyntheticEvent),
+) {
   forEachAccumulated(events, accumulateDirectDispatchesSingle);
 }
diff --git a/packages/legacy-events/ReactSyntheticEventType.js b/packages/legacy-events/ReactSyntheticEventType.js
@@ -31,8 +31,8 @@ export type ReactSyntheticEvent = {|
     nativeEventTarget: EventTarget,
   ) => ReactSyntheticEvent,
   isPersistent: () => boolean,
-  _dispatchInstances: null | Array<Fiber>,
-  _dispatchListeners: null | Array<Function>,
-  _targetInst: null | Fiber,
+  _dispatchInstances: null | Array<Fiber> | Fiber,
+  _dispatchListeners: null | Array<Function> | Function,
+  _targetInst: Fiber,
   type: string,
 |};
diff --git a/packages/legacy-events/ResponderEventPlugin.js b/packages/legacy-events/ResponderEventPlugin.js
@@ -5,19 +5,20 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import {getLowestCommonAncestor, isAncestor} from 'shared/ReactTreeTraversal';
+import {
+  getLowestCommonAncestor,
+  isAncestor,
+  getParentInstance,
+  traverseTwoPhase,
+} from 'shared/ReactTreeTraversal';
 
 import {
   executeDirectDispatch,
   hasDispatches,
   executeDispatchesInOrderStopAtTrue,
   getInstanceFromNode,
 } from './EventPluginUtils';
-import {
-  accumulateDirectDispatches,
-  accumulateTwoPhaseDispatches,
-  accumulateTwoPhaseDispatchesSkipTarget,
-} from './EventPropagators';
+import {accumulateDirectDispatches} from './EventPropagators';
 import ResponderSyntheticEvent from './ResponderSyntheticEvent';
 import ResponderTouchHistoryStore from './ResponderTouchHistoryStore';
 import accumulate from './accumulate';
@@ -32,6 +33,9 @@ import {
   moveDependencies,
   endDependencies,
 } from './ResponderTopLevelEventTypes';
+import getListener from './getListener';
+import accumulateInto from './accumulateInto';
+import forEachAccumulated from './forEachAccumulated';
 
 /**
  * Instance of element that should respond to touch/move types of interactions,
@@ -151,6 +155,54 @@ const eventTypes = {
   },
 };
 
+// Start of inline: the below functions were inlined from
+// EventPropagator.js, as they deviated from ReactDOM's newer
+// implementations.
+function listenerAtPhase(inst, event, propagationPhase: PropagationPhases) {
+  const registrationName =
+    event.dispatchConfig.phasedRegistrationNames[propagationPhase];
+  return getListener(inst, registrationName);
+}
+
+function accumulateDirectionalDispatches(inst, phase, event) {
+  if (__DEV__) {
+    if (!inst) {
+      console.error('Dispatching inst must not be null');
+    }
+  }
+  const listener = listenerAtPhase(inst, event, phase);
+  if (listener) {
+    event._dispatchListeners = accumulateInto(
+      event._dispatchListeners,
+      listener,
+    );
+    event._dispatchInstances = accumulateInto(event._dispatchInstances, inst);
+  }
+}
+
+function accumulateTwoPhaseDispatchesSingleSkipTarget(event) {
+  if (event && event.dispatchConfig.phasedRegistrationNames) {
+    const targetInst = event._targetInst;
+    const parentInst = targetInst ? getParentInstance(targetInst) : null;
+    traverseTwoPhase(parentInst, accumulateDirectionalDispatches, event);
+  }
+}
+
+function accumulateTwoPhaseDispatchesSkipTarget(events) {
+  forEachAccumulated(events, accumulateTwoPhaseDispatchesSingleSkipTarget);
+}
+
+function accumulateTwoPhaseDispatchesSingle(event) {
+  if (event && event.dispatchConfig.phasedRegistrationNames) {
+    traverseTwoPhase(event._targetInst, accumulateDirectionalDispatches, event);
+  }
+}
+
+function accumulateTwoPhaseDispatches(events) {
+  forEachAccumulated(events, accumulateTwoPhaseDispatchesSingle);
+}
+// End of inline
+
 /**
  *
  * Responder System:
diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js
@@ -50,10 +50,6 @@ import {
   eventNameDispatchConfigs,
   injectEventPluginsByName,
 } from 'legacy-events/EventPluginRegistry';
-import {
-  accumulateTwoPhaseDispatches,
-  accumulateDirectDispatches,
-} from 'legacy-events/EventPropagators';
 import ReactVersion from 'shared/ReactVersion';
 import invariant from 'shared/invariant';
 import {warnUnstableRenderSubtreeIntoContainer} from 'shared/ReactFeatureFlags';
@@ -184,8 +180,6 @@ const Internals = {
     getFiberCurrentPropsFromNode,
     injectEventPluginsByName,
     eventNameDispatchConfigs,
-    accumulateTwoPhaseDispatches,
-    accumulateDirectDispatches,
     enqueueStateRestore,
     restoreStateIfNeeded,
     dispatchEvent,
diff --git a/packages/react-dom/src/events/BeforeInputEventPlugin.js b/packages/react-dom/src/events/BeforeInputEventPlugin.js
@@ -28,7 +28,7 @@ import {
 } from './FallbackCompositionState';
 import SyntheticCompositionEvent from './SyntheticCompositionEvent';
 import SyntheticInputEvent from './SyntheticInputEvent';
-import {accumulateTwoPhaseListeners} from './DOMModernPluginEventSystem';
+import accumulateTwoPhaseListeners from './accumulateTwoPhaseListeners';
 
 const END_KEYCODES = [9, 13, 27, 32]; // Tab, Return, Esc, Space
 const START_KEYCODE = 229;
diff --git a/packages/react-dom/src/events/ChangeEventPlugin.js b/packages/react-dom/src/events/ChangeEventPlugin.js
@@ -27,9 +27,9 @@ import isEventSupported from './isEventSupported';
 import {getNodeFromInstance} from '../client/ReactDOMComponentTree';
 import {updateValueIfChanged} from '../client/inputValueTracking';
 import {setDefaultValue} from '../client/ReactDOMInput';
-import {accumulateTwoPhaseListeners} from './DOMModernPluginEventSystem';
 
 import {disableInputAttributeSyncing} from 'shared/ReactFeatureFlags';
+import accumulateTwoPhaseListeners from './accumulateTwoPhaseListeners';
 
 const eventTypes = {
   change: {
diff --git a/packages/react-dom/src/events/DOMModernPluginEventSystem.js b/packages/react-dom/src/events/DOMModernPluginEventSystem.js
@@ -19,9 +19,8 @@ import {registrationNameDependencies} from 'legacy-events/EventPluginRegistry';
 import {batchedEventUpdates} from 'legacy-events/ReactGenericBatching';
 import {executeDispatchesInOrder} from 'legacy-events/EventPluginUtils';
 import {plugins} from 'legacy-events/EventPluginRegistry';
-import getListener from 'legacy-events/getListener';
 
-import {HostRoot, HostPortal, HostComponent} from 'shared/ReactWorkTags';
+import {HostRoot, HostPortal} from 'shared/ReactWorkTags';
 
 import {addTrappedEventListener} from './ReactDOMEventListener';
 import getEventTarget from './getEventTarget';
@@ -315,46 +314,3 @@ export function attachElementListener(listener: ReactDOMListener): void {
 export function detachElementListener(listener: ReactDOMListener): void {
   // TODO
 }
-
-export function accumulateTwoPhaseListeners(event: ReactSyntheticEvent): void {
-  const phasedRegistrationNames = event.dispatchConfig.phasedRegistrationNames;
-  if (phasedRegistrationNames == null) {
-    return;
-  }
-  const {bubbled, captured} = phasedRegistrationNames;
-  const dispatchListeners = [];
-  const dispatchInstances = [];
-  let node = event._targetInst;
-  let hasListeners = false;
-
-  // Accumulate all instances and listeners via the target -> root path.
-  while (node !== null) {
-    // We only care for listeners that are on HostComponents (i.e. <div>)
-    if (node.tag === HostComponent) {
-      // Standard React on* listeners, i.e. onClick prop
-      const captureListener = getListener(node, captured);
-      if (captureListener != null) {
-        hasListeners = true;
-        // Capture listeners/instances should go at the start, so we
-        // unshift them to the start of the array.
-        dispatchListeners.unshift(captureListener);
-        dispatchInstances.unshift(node);
-      }
-      const bubbleListener = getListener(node, bubbled);
-      if (bubbleListener != null) {
-        hasListeners = true;
-        // Bubble listeners/instances should go at the end, so we
-        // push them to the end of the array.
-        dispatchListeners.push(bubbleListener);
-        dispatchInstances.push(node);
-      }
-    }
-    node = node.return;
-  }
-  // To prevent allocation to the event unless we actually
-  // have listeners we use the flag we would have set above.
-  if (hasListeners) {
-    event._dispatchListeners = dispatchListeners;
-    event._dispatchInstances = dispatchInstances;
-  }
-}
diff --git a/packages/react-dom/src/events/SelectEventPlugin.js b/packages/react-dom/src/events/SelectEventPlugin.js
@@ -26,7 +26,7 @@ import {getNodeFromInstance} from '../client/ReactDOMComponentTree';
 import {hasSelectionCapabilities} from '../client/ReactInputSelection';
 import {DOCUMENT_NODE} from '../shared/HTMLNodeType';
 import {isListeningToAllDependencies} from './DOMEventListenerMap';
-import {accumulateTwoPhaseListeners} from './DOMModernPluginEventSystem';
+import accumulateTwoPhaseListeners from './accumulateTwoPhaseListeners';
 
 const skipSelectionChangeEvent =
   canUseDOM && 'documentMode' in document && document.documentMode <= 11;
diff --git a/packages/react-dom/src/events/SimpleEventPlugin.js b/packages/react-dom/src/events/SimpleEventPlugin.js
@@ -36,7 +36,7 @@ import SyntheticTransitionEvent from './SyntheticTransitionEvent';
 import SyntheticUIEvent from './SyntheticUIEvent';
 import SyntheticWheelEvent from './SyntheticWheelEvent';
 import getEventCharCode from './getEventCharCode';
-import {accumulateTwoPhaseListeners} from './DOMModernPluginEventSystem';
+import accumulateTwoPhaseListeners from './accumulateTwoPhaseListeners';
 
 // Only used in DEV for exhaustiveness validation.
 const knownHTMLTopLevelTypes: Array<DOMTopLevelEventType> = [
diff --git a/packages/react-dom/src/events/accumulateTwoPhaseListeners.js b/packages/react-dom/src/events/accumulateTwoPhaseListeners.js
diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js
diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js
diff --git a/packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js b/packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js
