From d49e0e0be0941490fe709f80de137516ba4c0ee3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Thu, 2 Mar 2023 15:54:51 +0000 Subject: [PATCH] Removed unused imperative events implementation from React Native renderer (#26282) ## Summary I'm going to start implementing parts of this proposal https://github.com/react-native-community/discussions-and-proposals/pull/607 As part of that implementation I'm going to refactor a few parts of the interface between React and React Native. One of the main problems we have right now is that we have private parts used by React and React Native in the public instance exported by refs. I want to properly separate that. I saw that a few methods to attach event handlers imperatively on refs were also exposing some things in the public instance (the `_eventListeners`). I checked and these methods are unused, so we can just clean them up instead of having to refactor them too. Adding support for imperative event listeners is in the roadmap after this proposal, and its implementation might differ after this refactor. This is essentially a manual revert of #23386. I'll submit more PRs after this for the rest of the refactor. ## How did you test this change? Existing jest tests. Will test a React sync internally at Meta. --- .../src/ReactFabricEventEmitter.js | 4 +- .../src/ReactFabricHostConfig.js | 122 ------------- .../src/ReactNativeBridgeEventPlugin.js | 57 +++--- .../src/ReactNativeEventEmitter.js | 4 +- .../src/ReactNativeGetListener.js | 37 ++++ .../src/ReactNativeGetListeners.js | 166 ------------------ .../Libraries/ReactPrivate/CustomEvent.js | 14 -- .../ReactNativePrivateInterface.js | 3 - .../src/legacy-events/PropagationPhases.js | 10 -- scripts/flow/react-native-host-hooks.js | 12 -- 10 files changed, 61 insertions(+), 368 deletions(-) create mode 100644 packages/react-native-renderer/src/ReactNativeGetListener.js delete mode 100644 packages/react-native-renderer/src/ReactNativeGetListeners.js delete mode 100644 packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/CustomEvent.js delete mode 100644 packages/react-native-renderer/src/legacy-events/PropagationPhases.js diff --git a/packages/react-native-renderer/src/ReactFabricEventEmitter.js b/packages/react-native-renderer/src/ReactFabricEventEmitter.js index 4f9e4f5106ce7..b4a4694045d82 100644 --- a/packages/react-native-renderer/src/ReactFabricEventEmitter.js +++ b/packages/react-native-renderer/src/ReactFabricEventEmitter.js @@ -25,12 +25,12 @@ import { import {batchedUpdates} from './legacy-events/ReactGenericBatching'; import accumulateInto from './legacy-events/accumulateInto'; -import getListeners from './ReactNativeGetListeners'; +import getListener from './ReactNativeGetListener'; import {runEventsInBatch} from './legacy-events/EventBatching'; import {RawEventEmitter} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface'; -export {getListeners, registrationNameModules as registrationNames}; +export {getListener, registrationNameModules as registrationNames}; /** * Allows registered plugins an opportunity to extract events from top-level diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index cbd6e4937e6d3..f80d57623b7f7 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -99,28 +99,6 @@ export type RendererInspectionConfig = $ReadOnly<{ ) => void, }>; -// TODO?: find a better place for this type to live -export type EventListenerOptions = $ReadOnly<{ - capture?: boolean, - once?: boolean, - passive?: boolean, - signal: mixed, // not yet implemented -}>; -export type EventListenerRemoveOptions = $ReadOnly<{ - capture?: boolean, -}>; - -// TODO?: this will be changed in the future to be w3c-compatible and allow "EventListener" objects as well as functions. -export type EventListener = Function; - -type InternalEventListeners = { - [string]: { - listener: EventListener, - options: EventListenerOptions, - invalidated: boolean, - }[], -}; - // TODO: Remove this conditional once all changes have propagated. if (registerEventHandler) { /** @@ -137,7 +115,6 @@ class ReactFabricHostComponent { viewConfig: ViewConfig; currentProps: Props; _internalInstanceHandle: Object; - _eventListeners: ?InternalEventListeners; constructor( tag: number, @@ -236,105 +213,6 @@ class ReactFabricHostComponent { setNativeProps(stateNode.node, updatePayload); } } - - // This API (addEventListener, removeEventListener) attempts to adhere to the - // w3 Level2 Events spec as much as possible, treating HostComponent as a DOM node. - // - // Unless otherwise noted, these methods should "just work" and adhere to the W3 specs. - // If they deviate in a way that is not explicitly noted here, you've found a bug! - // - // See: - // * https://www.w3.org/TR/DOM-Level-2-Events/events.html - // * https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener - // * https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener - // - // And notably, not implemented (yet?): - // * https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/dispatchEvent - // - // - // Deviations from spec/TODOs: - // (1) listener must currently be a function, we do not support EventListener objects yet. - // (2) we do not support the `signal` option / AbortSignal yet - addEventListener_unstable( - eventType: string, - listener: EventListener, - options: EventListenerOptions | boolean, - // $FlowFixMe[missing-local-annot] - ) { - if (typeof eventType !== 'string') { - throw new Error('addEventListener_unstable eventType must be a string'); - } - if (typeof listener !== 'function') { - throw new Error('addEventListener_unstable listener must be a function'); - } - - // The third argument is either boolean indicating "captures" or an object. - const optionsObj = - typeof options === 'object' && options !== null ? options : {}; - const capture = - (typeof options === 'boolean' ? options : optionsObj.capture) || false; - const once = optionsObj.once || false; - const passive = optionsObj.passive || false; - const signal = null; // TODO: implement signal/AbortSignal - - /* $FlowFixMe the old version of Flow doesn't have a good way to define an - * empty exact object. */ - const eventListeners: InternalEventListeners = this._eventListeners || {}; - if (this._eventListeners == null) { - this._eventListeners = eventListeners; - } - - const namedEventListeners = eventListeners[eventType] || []; - if (eventListeners[eventType] == null) { - eventListeners[eventType] = namedEventListeners; - } - - namedEventListeners.push({ - listener: listener, - invalidated: false, - options: { - capture: capture, - once: once, - passive: passive, - signal: signal, - }, - }); - } - - // See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener - removeEventListener_unstable( - eventType: string, - listener: EventListener, - options: EventListenerRemoveOptions | boolean, - ) { - // eventType and listener must be referentially equal to be removed from the listeners - // data structure, but in "options" we only check the `capture` flag, according to spec. - // That means if you add the same function as a listener with capture set to true and false, - // you must also call removeEventListener twice with capture set to true/false. - const optionsObj = - typeof options === 'object' && options !== null ? options : {}; - const capture = - (typeof options === 'boolean' ? options : optionsObj.capture) || false; - - // If there are no event listeners or named event listeners, we can bail early - our - // job is already done. - const eventListeners = this._eventListeners; - if (!eventListeners) { - return; - } - const namedEventListeners = eventListeners[eventType]; - if (!namedEventListeners) { - return; - } - - // TODO: optimize this path to make remove cheaper - eventListeners[eventType] = namedEventListeners.filter(listenerObj => { - return !( - listenerObj.listener === listener && - listenerObj.options.capture === capture - ); - }); - } } // $FlowFixMe[class-object-subtyping] found when upgrading Flow diff --git a/packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js b/packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js index 4e238ffb9be00..85ad0e6120715 100644 --- a/packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js +++ b/packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js @@ -13,15 +13,13 @@ import type { } from './legacy-events/PluginModuleType'; import type {TopLevelType} from './legacy-events/TopLevelEventTypes'; import SyntheticEvent from './legacy-events/SyntheticEvent'; -import type {PropagationPhases} from './legacy-events/PropagationPhases'; // Module provided by RN: import {ReactNativeViewConfigRegistry} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface'; import accumulateInto from './legacy-events/accumulateInto'; -import getListeners from './ReactNativeGetListeners'; +import getListener from './ReactNativeGetListener'; import forEachAccumulated from './legacy-events/forEachAccumulated'; import {HostComponent} from 'react-reconciler/src/ReactWorkTags'; -import isArray from 'shared/isArray'; const {customBubblingEventTypes, customDirectEventTypes} = ReactNativeViewConfigRegistry; @@ -30,38 +28,10 @@ const {customBubblingEventTypes, customDirectEventTypes} = // EventPropagator.js, as they deviated from ReactDOM's newer // implementations. // $FlowFixMe[missing-local-annot] -function listenersAtPhase(inst, event, propagationPhase: PropagationPhases) { +function listenerAtPhase(inst, event, propagationPhase: PropagationPhases) { const registrationName = event.dispatchConfig.phasedRegistrationNames[propagationPhase]; - return getListeners(inst, registrationName, propagationPhase, true); -} - -// $FlowFixMe[missing-local-annot] -function accumulateListenersAndInstances(inst, event, listeners) { - const listenersLength = listeners - ? isArray(listeners) - ? listeners.length - : 1 - : 0; - if (listenersLength > 0) { - event._dispatchListeners = accumulateInto( - event._dispatchListeners, - listeners, - ); - - // Avoid allocating additional arrays here - if (event._dispatchInstances == null && listenersLength === 1) { - event._dispatchInstances = inst; - } else { - event._dispatchInstances = event._dispatchInstances || []; - if (!isArray(event._dispatchInstances)) { - event._dispatchInstances = [event._dispatchInstances]; - } - for (let i = 0; i < listenersLength; i++) { - event._dispatchInstances.push(inst); - } - } - } + return getListener(inst, registrationName); } // $FlowFixMe[missing-local-annot] @@ -71,8 +41,14 @@ function accumulateDirectionalDispatches(inst, phase, event) { console.error('Dispatching inst must not be null'); } } - const listeners = listenersAtPhase(inst, event, phase); - accumulateListenersAndInstances(inst, event, listeners); + const listener = listenerAtPhase(inst, event, phase); + if (listener) { + event._dispatchListeners = accumulateInto( + event._dispatchListeners, + listener, + ); + event._dispatchInstances = accumulateInto(event._dispatchInstances, inst); + } } // $FlowFixMe[missing-local-annot] @@ -160,8 +136,14 @@ function accumulateDispatches( ): void { if (inst && event && event.dispatchConfig.registrationName) { const registrationName = event.dispatchConfig.registrationName; - const listeners = getListeners(inst, registrationName, 'bubbled', false); - accumulateListenersAndInstances(inst, event, listeners); + const listener = getListener(inst, registrationName); + if (listener) { + event._dispatchListeners = accumulateInto( + event._dispatchListeners, + listener, + ); + event._dispatchInstances = accumulateInto(event._dispatchInstances, inst); + } } } @@ -181,6 +163,7 @@ function accumulateDirectDispatches(events: ?(Array | Object)) { } // End of inline +type PropagationPhases = 'bubbled' | 'captured'; const ReactNativeBridgeEventPlugin = { eventTypes: ({}: EventTypes), diff --git a/packages/react-native-renderer/src/ReactNativeEventEmitter.js b/packages/react-native-renderer/src/ReactNativeEventEmitter.js index cbb756bc49a79..c740e085e863d 100644 --- a/packages/react-native-renderer/src/ReactNativeEventEmitter.js +++ b/packages/react-native-renderer/src/ReactNativeEventEmitter.js @@ -21,12 +21,12 @@ import { } from './legacy-events/EventPluginRegistry'; import {batchedUpdates} from './legacy-events/ReactGenericBatching'; import {runEventsInBatch} from './legacy-events/EventBatching'; -import getListeners from './ReactNativeGetListeners'; +import getListener from './ReactNativeGetListener'; import accumulateInto from './legacy-events/accumulateInto'; import {getInstanceFromNode} from './ReactNativeComponentTree'; -export {getListeners, registrationNameModules as registrationNames}; +export {getListener, registrationNameModules as registrationNames}; /** * Version of `ReactBrowserEventEmitter` that works on the receiving side of a diff --git a/packages/react-native-renderer/src/ReactNativeGetListener.js b/packages/react-native-renderer/src/ReactNativeGetListener.js new file mode 100644 index 0000000000000..a741ee840a5db --- /dev/null +++ b/packages/react-native-renderer/src/ReactNativeGetListener.js @@ -0,0 +1,37 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import type {Fiber} from 'react-reconciler/src/ReactInternalTypes'; + +import {getFiberCurrentPropsFromNode} from './legacy-events/EventPluginUtils'; + +export default function getListener( + inst: Fiber, + registrationName: string, +): Function | null { + const stateNode = inst.stateNode; + if (stateNode === null) { + // Work in progress (ex: onload events in incremental mode). + return null; + } + const props = getFiberCurrentPropsFromNode(stateNode); + if (props === null) { + // Work in progress. + return null; + } + const listener = props[registrationName]; + + if (listener && typeof listener !== 'function') { + throw new Error( + `Expected \`${registrationName}\` listener to be a function, instead got a value of \`${typeof listener}\` type.`, + ); + } + + return listener; +} diff --git a/packages/react-native-renderer/src/ReactNativeGetListeners.js b/packages/react-native-renderer/src/ReactNativeGetListeners.js deleted file mode 100644 index 979b19be3389d..0000000000000 --- a/packages/react-native-renderer/src/ReactNativeGetListeners.js +++ /dev/null @@ -1,166 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * @flow - */ - -import type {Fiber} from 'react-reconciler/src/ReactInternalTypes'; -import type {PropagationPhases} from './legacy-events/PropagationPhases'; - -import {getFiberCurrentPropsFromNode} from './legacy-events/EventPluginUtils'; -import {CustomEvent} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface'; - -/** - * Get a list of listeners for a specific event, in-order. - * For React Native we treat the props-based function handlers - * as the first-class citizens, and they are always executed first - * for both capture and bubbling phase. - * - * We need "phase" propagated to this point to support the HostComponent - * EventEmitter API, which does not mutate the name of the handler based - * on phase (whereas prop handlers are registered as `onMyEvent` and `onMyEvent_Capture`). - * - * Native system events emitted into React Native - * will be emitted both to the prop handler function and to imperative event - * listeners. - * - * This will either return null, a single Function without an array, or - * an array of 2+ items. - */ -export default function getListeners( - inst: Fiber, - registrationName: string, - phase: PropagationPhases, - dispatchToImperativeListeners: boolean, -): null | Function | Array { - const stateNode = inst.stateNode; - - if (stateNode === null) { - return null; - } - - // If null: Work in progress (ex: onload events in incremental mode). - const props = getFiberCurrentPropsFromNode(stateNode); - if (props === null) { - // Work in progress. - return null; - } - - const listener = props[registrationName]; - - if (listener && typeof listener !== 'function') { - throw new Error( - `Expected \`${registrationName}\` listener to be a function, instead got a value of \`${typeof listener}\` type.`, - ); - } - - // If there are no imperative listeners, early exit. - if ( - !( - dispatchToImperativeListeners && - stateNode.canonical && - stateNode.canonical._eventListeners - ) - ) { - return listener; - } - - // Below this is the de-optimized path. - // If you are using _eventListeners, we do not (yet) - // expect this to be as performant as the props-only path. - // If/when this becomes a bottleneck, it can be refactored - // to avoid unnecessary closures and array allocations. - // - // Previously, there was only one possible listener for an event: - // the onEventName property in props. - // Now, it is also possible to have N listeners - // for a specific event on a node. Thus, we accumulate all of the listeners, - // including the props listener, and return a function that calls them all in - // order, starting with the handler prop and then the listeners in order. - // We return either a non-empty array or null. - const listeners = []; - if (listener) { - listeners.push(listener); - } - - // TODO: for now, all of these events get an `rn:` prefix to enforce - // that the user knows they're only getting non-W3C-compliant events - // through this imperative event API. - // Events might not necessarily be noncompliant, but we currently have - // no verification that /any/ events are compliant. - // Thus, we prefix to ensure no collision with W3C event names. - const requestedPhaseIsCapture = phase === 'captured'; - const mangledImperativeRegistrationName = requestedPhaseIsCapture - ? 'rn:' + registrationName.replace(/Capture$/, '') - : 'rn:' + registrationName; - - // Get imperative event listeners for this event - if ( - stateNode.canonical._eventListeners[mangledImperativeRegistrationName] && - stateNode.canonical._eventListeners[mangledImperativeRegistrationName] - .length > 0 - ) { - const eventListeners = - stateNode.canonical._eventListeners[mangledImperativeRegistrationName]; - - eventListeners.forEach(listenerObj => { - // Make sure phase of listener matches requested phase - const isCaptureEvent = - listenerObj.options.capture != null && listenerObj.options.capture; - if (isCaptureEvent !== requestedPhaseIsCapture) { - return; - } - - // For now (this is an area of future optimization) we must wrap - // all imperative event listeners in a function to unwrap the SyntheticEvent - // and pass them an Event. - // When this API is more stable and used more frequently, we can revisit. - // $FlowFixMe[missing-local-annot] - const listenerFnWrapper = function (syntheticEvent, ...args) { - const eventInst = new CustomEvent(mangledImperativeRegistrationName, { - detail: syntheticEvent.nativeEvent, - }); - eventInst.isTrusted = true; - eventInst.setSyntheticEvent(syntheticEvent); - - listenerObj.listener(eventInst, ...args); - }; - - // Only call once? - // If so, we ensure that it's only called once by setting a flag - // and by removing it from eventListeners once it is called (but only - // when it's actually been executed). - if (listenerObj.options.once) { - listeners.push(function (...args) { - // Remove from the event listener once it's been called - stateNode.canonical.removeEventListener_unstable( - mangledImperativeRegistrationName, - listenerObj.listener, - listenerObj.capture, - ); - - // Guard against function being called more than once in - // case there are somehow multiple in-flight references to - // it being processed - if (!listenerObj.invalidated) { - listenerObj.invalidated = true; - listenerObj.listener(...args); - } - }); - } else { - listeners.push(listenerFnWrapper); - } - }); - } - - if (listeners.length === 0) { - return null; - } - if (listeners.length === 1) { - return listeners[0]; - } - - return listeners; -} diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/CustomEvent.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/CustomEvent.js deleted file mode 100644 index 6ac6831fb069a..0000000000000 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/CustomEvent.js +++ /dev/null @@ -1,14 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -'use strict'; - -// See the react-native repository for a full implementation. -// This is just a stub, currently to pass `instanceof` checks. -const CustomEvent = jest.fn(); - -module.exports = {default: CustomEvent}; diff --git a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js index 09e9b1ea7170b..befc3bb617f4f 100644 --- a/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js +++ b/packages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/ReactNativePrivateInterface.js @@ -44,7 +44,4 @@ module.exports = { get RawEventEmitter() { return require('./RawEventEmitter').default; }, - get CustomEvent() { - return require('./CustomEvent').default; - }, }; diff --git a/packages/react-native-renderer/src/legacy-events/PropagationPhases.js b/packages/react-native-renderer/src/legacy-events/PropagationPhases.js deleted file mode 100644 index 6672ab7687ae8..0000000000000 --- a/packages/react-native-renderer/src/legacy-events/PropagationPhases.js +++ /dev/null @@ -1,10 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -export type PropagationPhases = 'bubbled' | 'captured'; diff --git a/scripts/flow/react-native-host-hooks.js b/scripts/flow/react-native-host-hooks.js index 5fcee3b57da7c..e3899db44528d 100644 --- a/scripts/flow/react-native-host-hooks.js +++ b/scripts/flow/react-native-host-hooks.js @@ -143,18 +143,6 @@ declare module 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface' emit: (channel: string, event: RawEventEmitterEvent) => string, ... }; - declare export class CustomEvent { - isTrusted: boolean; - - constructor( - name: string, - { - detail: any, - }, - ): void; - - setSyntheticEvent(event: any): void; - } } declare module 'react-native/Libraries/ReactPrivate/ReactNativePrivateInitializeCore' {