Skip to content

[WIP] React Native: Turn HostComponent into an EventEmitter #23278

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a33b351
Turn HostComponent into an EventEmitter
JoshuaGross Feb 11, 2022
83e9bc9
_eventListeners is null by default to save on memory/perf
JoshuaGross Feb 11, 2022
d73dd66
make listeners nullable
JoshuaGross Feb 11, 2022
d35b766
New mechanism for 'once'
JoshuaGross Feb 12, 2022
29bf6a4
fix typo, add comments
JoshuaGross Feb 14, 2022
049a50b
CustomEvent should be typed in react, but implemented in RN repo
JoshuaGross Feb 14, 2022
88cfd64
Handle CustomEvent in ReactNativeBridgeEventPlugin
JoshuaGross Feb 18, 2022
f79c3fc
getEventListener returns an array of functions instead of Function | …
JoshuaGross Feb 18, 2022
7f2a1b3
Send data back and forth between Event <> SyntheticEvent in a way tha…
JoshuaGross Feb 24, 2022
7f3a82c
typo
JoshuaGross Feb 24, 2022
5a8047d
fix null deref
JoshuaGross Feb 24, 2022
3f38d4a
fix dispatch / listener code to accept 0 or more listeners and handle…
JoshuaGross Feb 24, 2022
1055e56
fix another site where insts.length needs to == listeners.length
JoshuaGross Feb 24, 2022
f1746d2
getListener -> getListeners
JoshuaGross Feb 24, 2022
df4a1fd
fix flow
JoshuaGross Feb 24, 2022
e5f5199
missing files
JoshuaGross Feb 24, 2022
901742a
prettier
JoshuaGross Feb 24, 2022
a78b098
fix lints, clean up
JoshuaGross Feb 24, 2022
633f9eb
initialize listeners to null, only create empty array if there is a l…
JoshuaGross Feb 24, 2022
7adec88
initialize listeners to null, only create empty array if there is a l…
JoshuaGross Feb 24, 2022
5bc7103
var in for loop to make ci happy?
JoshuaGross Feb 24, 2022
e50bf63
yarn replace-fork
JoshuaGross Feb 24, 2022
f01bd0c
turn for loop into forEach
JoshuaGross Feb 24, 2022
d1c0843
yarn prettier-all
JoshuaGross Feb 24, 2022
050211d
attempt to provide a working CustomEvent jest mock so that tests can run
JoshuaGross Feb 24, 2022
b37c1af
remove console.log
JoshuaGross Feb 24, 2022
a994a3a
fix capture phase registration
JoshuaGross Feb 25, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-native-renderer/src/ReactFabricEventEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ import {batchedUpdates} from './legacy-events/ReactGenericBatching';
import accumulateInto from './legacy-events/accumulateInto';

import {plugins} from './legacy-events/EventPluginRegistry';
import getListener from './ReactNativeGetListener';
import getListeners from './ReactNativeGetListeners';
import {runEventsInBatch} from './legacy-events/EventBatching';

import {RawEventEmitter} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface';

export {getListener, registrationNameModules as registrationNames};
export {getListeners, registrationNameModules as registrationNames};

/**
* Allows registered plugins an opportunity to extract events from top-level
Expand Down
119 changes: 119 additions & 0 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import type {ReactNodeList, OffscreenMode} from 'shared/ReactTypes';
import type {ElementRef} from 'react';
import type {
CustomEvent,
HostComponent,
MeasureInWindowOnSuccessCallback,
MeasureLayoutOnSuccessCallback,
Expand Down Expand Up @@ -95,6 +96,19 @@ 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;

// TODO: Remove this conditional once all changes have propagated.
if (registerEventHandler) {
/**
Expand All @@ -103,6 +117,14 @@ if (registerEventHandler) {
registerEventHandler(dispatchEvent);
}

type InternalEventListeners = {
[string]: {|
listener: EventListener,
options: EventListenerOptions,
invalidated: boolean,
|}[],
};

/**
* This is used for refs on host components.
*/
Expand All @@ -111,6 +133,7 @@ class ReactFabricHostComponent {
viewConfig: ViewConfig;
currentProps: Props;
_internalInstanceHandle: Object;
_eventListeners: ?InternalEventListeners;

constructor(
tag: number,
Expand All @@ -122,6 +145,7 @@ class ReactFabricHostComponent {
this.viewConfig = viewConfig;
this.currentProps = props;
this._internalInstanceHandle = internalInstanceHandle;
this._eventListeners = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we initialize it lazily, I'm not sure I feel great about adding a new field to every single host instance, even if it's set to null. We're trying to minimize memory usage. And this is doing that for a (relatively uncommon) feature in the tree. I wonder if there's any way we could only pay the cost for the components using it. E.g. some sort of a Map outside. I guess it would have to be a WeakMap. I think we need to get @sebmarkbage opinion on this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One extra field is probably not the biggest deal here. The big deal is that this object exists at all and that methods use virtual dispatch and that all methods must always be loaded instead of lazily.

The goal was to get rid of it but if the goal is to preserve DOM-like looks, then maybe the whole object can at least be made lazy only if there are refs on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The big deal is that this object exists at all ... The goal was to get rid of it

By "this object" do you mean the ReactFabricHostComponent object? Can you describe what the alternative would be - just using a raw JSI-bridged native object? I was not aware of those plans. Worth discussing that more for sure if we want to move further in that direction, I'd like to hear pros/cons and it'd be good to have an idea of what the cost of this class and fields are

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the whole object can at least be made lazy only if there are refs on it

Do you mean something like - ReactFabricHostComponent is not instantiated for a given ShadowNode until/unless it is requested via the ref prop? That is a pretty interesting optimization that I would be happy to drive (in a few months - I'm going on leave soon) - I would be happy with that, hopefully we agree that that is outside of the scope of this PR?

}

blur() {
Expand Down Expand Up @@ -193,6 +217,101 @@ class ReactFabricHostComponent {

return;
}

// This API (dispatchEvent, 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/dispatchEvent
// * https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
// * https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener
dispatchEvent_unstable(event: CustomEvent) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding stuff to the prototype should be ok, I guess. Feels a bit strange that these instances are a mix of "private" things like viewConfig and currentProps and actual "public" APIs. Maybe we should've made a separate "public instance" so that private stuff isn't exposed to refs. But I understand this follows the existing precedent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally we designed this object to be completely removed so that we could reclaim the memory and to optimize calls by removing virtual dispatch. Eg by introducing apis like focus(ref) instead.

These other methods were just there for backwards compatibility.

That was never followed through and so this was never deleted.

So this direction seems like a reversal of that strategy and to keep living with more memory usage and worse performance. If that’s the direction then there are probably more things that should be cleaned up.

Copy link
Contributor

@necolas necolas Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this direction seems like a reversal of that strategy

The other option was to do addEventListener(hostElement, ...)

dispatchEvent(this._internalInstanceHandle, event.type, event);
}

// 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,
) {
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

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;
}

eventListeners[eventType] = namedEventListeners.filter(listenerObj => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this allocation unavoidable? remove/add resubscriptions can be a hot path. It would be nice if removing and adding was very cheap.

return !(
listenerObj.listener === listener &&
listenerObj.options.capture === capture
);
});
}
}

// eslint-disable-next-line no-unused-expressions
Expand Down
117 changes: 99 additions & 18 deletions packages/react-native-renderer/src/ReactNativeBridgeEventPlugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@
import type {AnyNativeEvent} 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 {
CustomEvent,
ReactNativeViewConfigRegistry,
} from 'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface';
import accumulateInto from './legacy-events/accumulateInto';
import getListener from './ReactNativeGetListener';
import getListeners from './ReactNativeGetListeners';
import forEachAccumulated from './legacy-events/forEachAccumulated';
import {HostComponent} from 'react-reconciler/src/ReactWorkTags';

Expand All @@ -26,10 +30,15 @@ const {
// 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) {
function listenersAtPhase(
inst,
event,
propagationPhase: PropagationPhases,
isCustomEvent: boolean,
) {
const registrationName =
event.dispatchConfig.phasedRegistrationNames[propagationPhase];
return getListener(inst, registrationName);
return getListeners(inst, registrationName, propagationPhase, isCustomEvent);
}

function accumulateDirectionalDispatches(inst, phase, event) {
Expand All @@ -38,13 +47,21 @@ function accumulateDirectionalDispatches(inst, phase, event) {
console.error('Dispatching inst must not be null');
}
}
const listener = listenerAtPhase(inst, event, phase);
if (listener) {
const listeners = listenersAtPhase(
inst,
event,
phase,
event instanceof CustomEvent,
);
if (listeners && listeners.length > 0) {
event._dispatchListeners = accumulateInto(
event._dispatchListeners,
listener,
listeners,
Copy link
Collaborator

@gaearon gaearon Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The vast majority case is going to be one listener. It feels a bit wrong to me that we're now paying the runtime cost assuming there's more than one. I.e. always having an array.

Maybe we can simplify the code. E.g. accumulateInto here is a helper that tries to add T | Array<T> on the right side to T | Array<T> on the left side. But now our right side is always an array. So the isArray check inside of accumulateInto is pointless. And creating an intermediate array only to stuff it back into another array also seems pointless.

I would suggest that if we're doing this, let's get rid of accumulateInto altogether. Instead of listenersAtPhase that always allocates, you could have something like pushListenersAtPhase which will lazily create event._dispatchListeners if needed, and then push into it. No accumulateInto, no intermediate checks, no extra arrays. It already has the access to event, anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to address this in the next PR by returning T | Array | null from getEventListeners and avoid allocations more aggressively

);
event._dispatchInstances = accumulateInto(event._dispatchInstances, inst);
const insts = listeners.map(() => {
return inst;
});
event._dispatchInstances = accumulateInto(event._dispatchInstances, insts);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, too, we create an intermediate array via map only to copy it into another array. Let's rewrite this as plainly as possible? If we do the refactoring I suggested earlier then we can push into event._dispatchInstances from pushListenersAtPhase (in the same place where we're pushing into event._dispatchListeners) and we won't need this map. You already have access to inst there, too.

}
}

Expand All @@ -66,7 +83,12 @@ function getParent(inst) {
/**
* Simulates the traversal of a two-phase, capture/bubble event dispatch.
*/
export function traverseTwoPhase(inst: Object, fn: Function, arg: Function) {
export function traverseTwoPhase(
inst: Object,
fn: Function,
arg: Function,
bubbles: boolean,
) {
const path = [];
while (inst) {
path.push(inst);
Expand All @@ -78,12 +100,28 @@ export function traverseTwoPhase(inst: Object, fn: Function, arg: Function) {
}
for (i = 0; i < path.length; i++) {
fn(path[i], 'bubbled', arg);
// It's possible this is false for custom events.
if (!bubbles) {
Copy link
Collaborator

@gaearon gaearon Feb 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this related to adding support for capturable-but-not-bubbling events? I had concerns with this that I described in the post about events and composition. I'd like to make sure we have addressed these concerns before adding/exposing that feature. I'm not sure if this PR is orthogonal to what @lunaleaps tried originally, or whether it includes a similar feature. If it includes that feature, then I’d like to make sure we have @sebmarkbage’s stamp on that. Luna has the context on my concerns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this PR is mostly orthogonal.

This is for capturable-but-not-bubbling CustomEvents since those are legal under the W3C spec.

Copy link
Contributor

@lunaleaps lunaleaps Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this change is similar to what I was trying to add to support pointer events that skipBubbling -- maybe we can split this out to the separate PR https://github.com/facebook/react/pull/23366/files#diff-64c0fc0cf584da861da7ea6b3b66f74bebbc8fa4857be0c4021f186f812219ddR96

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm submitting a new PR that just adds addEventListener/removeEventListener, so we don't need any code related to CustomEvent for now.

break;
}
}
}

function accumulateTwoPhaseDispatchesSingle(event) {
if (event && event.dispatchConfig.phasedRegistrationNames) {
traverseTwoPhase(event._targetInst, accumulateDirectionalDispatches, event);
// bubbles is only set on the dispatchConfig for custom events.
// The `event` param here at this point is a SyntheticEvent, not an Event or CustomEvent.
const bubbles =
event.dispatchConfig.isCustomEvent === true
? !!event.dispatchConfig.bubbles
: true;

traverseTwoPhase(
event._targetInst,
accumulateDirectionalDispatches,
event,
bubbles,
);
}
}

Expand All @@ -103,13 +141,27 @@ function accumulateDispatches(
): void {
if (inst && event && event.dispatchConfig.registrationName) {
const registrationName = event.dispatchConfig.registrationName;
const listener = getListener(inst, registrationName);
if (listener) {
// Since we "do not look for phased registration names", that
// should be the same as "bubbled" here, for all intents and purposes...?
const listeners = getListeners(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments about getListeners => pushListeners or something to prevent unnecessary allocation and later unnecessary accumulateInto checks.

inst,
registrationName,
'bubbled',
!!event.dispatchConfig.isCustomEvent,
);
if (listeners) {
event._dispatchListeners = accumulateInto(
event._dispatchListeners,
listener,
listeners,
);
// an inst for every listener
const insts = listeners.map(() => {
return inst;
});
event._dispatchInstances = accumulateInto(
event._dispatchInstances,
insts,
);
event._dispatchInstances = accumulateInto(event._dispatchInstances, inst);
}
}
}
Expand All @@ -130,7 +182,6 @@ function accumulateDirectDispatches(events: ?(Array<Object> | Object)) {
}

// End of inline
type PropagationPhases = 'bubbled' | 'captured';

const ReactNativeBridgeEventPlugin = {
eventTypes: {},
Expand All @@ -147,23 +198,53 @@ const ReactNativeBridgeEventPlugin = {
}
const bubbleDispatchConfig = customBubblingEventTypes[topLevelType];
const directDispatchConfig = customDirectEventTypes[topLevelType];
let customEventConfig = null;
if (nativeEvent instanceof CustomEvent) {
// $FlowFixMe
if (topLevelType.indexOf('on') !== 0) {
throw new Error('Custom event name must start with "on"');
}
nativeEvent.isTrusted = false;
// For now, this custom event name should technically not be used -
// CustomEvents emitted in the system do not result in calling prop handlers.
customEventConfig = {
registrationName: topLevelType,
isCustomEvent: true,
bubbles: nativeEvent.bubbles,
phasedRegistrationNames: {
bubbled: topLevelType,
// Unlike with the props-based handlers, capture events are registered
// to the HostComponent event emitter with the same name but a flag indicating
// that the handler is for the capture phase.
captured: topLevelType,
},
};
}

if (!bubbleDispatchConfig && !directDispatchConfig) {
if (!bubbleDispatchConfig && !directDispatchConfig && !customEventConfig) {
throw new Error(
// $FlowFixMe - Flow doesn't like this string coercion because DOMTopLevelEventType is opaque
`Unsupported top level event type "${topLevelType}" dispatched`,
);
}

const event = SyntheticEvent.getPooled(
bubbleDispatchConfig || directDispatchConfig,
bubbleDispatchConfig || directDispatchConfig || customEventConfig,
targetInst,
nativeEvent,
nativeEventTarget,
);
if (bubbleDispatchConfig) {
if (bubbleDispatchConfig || customEventConfig) {
// All CustomEvents go through two-phase dispatching, even if they
// are non-bubbling events, which is why we put the `bubbles` param
// in the config for CustomEvents only.
// CustomEvents are not emitted to prop handler functions ever.
// Native two-phase events will be emitted to prop handler functions
// and to HostComponent event listeners.
accumulateTwoPhaseDispatches(event);
} else if (directDispatchConfig) {
// Direct dispatched events do not go to HostComponent EventEmitters,
// they *only* go to the prop function handlers.
accumulateDirectDispatches(event);
} else {
return null;
Expand Down
4 changes: 2 additions & 2 deletions packages/react-native-renderer/src/ReactNativeEventEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ import {registrationNameModules} from './legacy-events/EventPluginRegistry';
import {batchedUpdates} from './legacy-events/ReactGenericBatching';
import {runEventsInBatch} from './legacy-events/EventBatching';
import {plugins} from './legacy-events/EventPluginRegistry';
import getListener from './ReactNativeGetListener';
import getListeners from './ReactNativeGetListeners';
import accumulateInto from './legacy-events/accumulateInto';

import {getInstanceFromNode} from './ReactNativeComponentTree';

export {getListener, registrationNameModules as registrationNames};
export {getListeners, registrationNameModules as registrationNames};

/**
* Version of `ReactBrowserEventEmitter` that works on the receiving side of a
Expand Down
Loading