Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
40 changes: 40 additions & 0 deletions packages/react-native-renderer/src/CustomEvent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* 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.
*
* @format
* @flow strict-local
*/

export type CustomEventOptions = $ReadOnly<{|
bubbles?: boolean,
cancelable?: boolean,
detail?: { ... }
|}>;

// TODO: should extend an Event base-class that has most of these properties
class CustomEvent extends Event {
type: string;
detail: ?{ ... };
bubbles: boolean;
cancelable: boolean;
isTrusted: boolean;

constructor(typeArg: string, options: CustomEventOptions) {
const { bubbles, cancelable } = options;
// TODO: support passing in the `composed` param
super(typeArg, { bubbles, cancelable });

this.detail = options.detail; // this would correspond to `NativeEvent` in SyntheticEvent

// TODO: do we need these since they should be on Event? (for RN we probably need a polyfill)
this.type = typeArg;
this.bubbles = !!(bubbles || false);
this.cancelable = !!(cancelable || false);
this.isTrusted = false;
}
}

export default CustomEvent;
80 changes: 80 additions & 0 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {mountSafeCallback_NOT_REALLY_SAFE} from './NativeMethodsMixinUtils';
import {create, diff} from './ReactNativeAttributePayload';

import {dispatchEvent} from './ReactFabricEventEmitter';
import CustomEvent from './CustomEvent';

import {
DefaultEventPriority,
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,8 @@ if (registerEventHandler) {
registerEventHandler(dispatchEvent);
}

type InternalEventListeners = { [string]: $ReadOnly<{| listener: EventListener, options: EventListenerOptions |}>[] };

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

constructor(
tag: number,
Expand All @@ -122,6 +139,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 +211,68 @@ class ReactFabricHostComponent {

return;
}

dipatchEvent_unstable(event: CustomEvent) {
dispatchEvent(this._internalInstanceHandle, event.type, event);
}

// This API adheres to the w3c addEventListener spec.
// See https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener
// See https://www.w3.org/TR/DOM-Level-2-Events/events.html
//
// Exceptions/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

if (this._eventListeners === null) {
this._eventListeners = {};
}
const eventListeners: InternalEventListeners = this._eventListeners;
if (eventListeners != null) {
eventListeners[eventType] = eventListeners[eventType] || [];
eventListeners[eventType].push({
listener: listener,
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 (!this._eventListeners[eventType]) {
return;
}

this._eventListeners[eventType] = this._eventListeners[eventType].filter(listenerObj => {
return !(listenerObj.listener === listener && listenerObj.options.capture === capture);
});
};
}

// eslint-disable-next-line no-unused-expressions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
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';
Expand All @@ -29,7 +30,7 @@ const {
function listenerAtPhase(inst, event, propagationPhase: PropagationPhases) {
const registrationName =
event.dispatchConfig.phasedRegistrationNames[propagationPhase];
return getListener(inst, registrationName);
return getListener(inst, registrationName, propagationPhase);
}

function accumulateDirectionalDispatches(inst, phase, event) {
Expand Down Expand Up @@ -103,7 +104,9 @@ function accumulateDispatches(
): void {
if (inst && event && event.dispatchConfig.registrationName) {
const registrationName = event.dispatchConfig.registrationName;
const listener = getListener(inst, registrationName);
// Since we "do not look for phased registration names", that
// should be the same as "bubbled" here, for all intents and purposes...?
const listener = getListener(inst, registrationName, 'bubbled');
if (listener) {
event._dispatchListeners = accumulateInto(
event._dispatchListeners,
Expand All @@ -130,7 +133,6 @@ function accumulateDirectDispatches(events: ?(Array<Object> | Object)) {
}

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

const ReactNativeBridgeEventPlugin = {
eventTypes: {},
Expand Down
76 changes: 64 additions & 12 deletions packages/react-native-renderer/src/ReactNativeGetListener.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,82 @@
*/

import type {Fiber} from 'react-reconciler/src/ReactInternalTypes';
import type {PropagationPhases} from './legacy-events/PropagationPhases';

import {getFiberCurrentPropsFromNode} from './legacy-events/EventPluginUtils';

export default function getListener(
inst: Fiber,
registrationName: string,
phase: PropagationPhases,
): Function | null {
// 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 still return a single function or null.
const listeners = [];

const stateNode = inst.stateNode;
if (stateNode === null) {
// Work in progress (ex: onload events in incremental mode).
return null;
// If null: Work in progress (ex: onload events in incremental mode).
if (stateNode !== 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.`,
);
}

if (listener) {
listeners.push(listener);
}
}

// Get imperative event listeners for this event
if (stateNode.canonical?._eventListeners?.[registrationName]?.length > 0) {
var toRemove = [];
for (var listenerObj of stateNode.canonical._eventListeners[registrationName]) {
// Make sure phase of listener matches requested phase
const isCaptureEvent = listenerObj.options.capture != null && listenerObj.options.capture;
if (isCaptureEvent !== (phase === 'captured')) {
continue;
}

listeners.push(listenerObj.listener);

// Only call once? If so, remove from listeners after calling.
if (listenerObj.options.once) {
toRemove.push(listenerObj);
}
}
for (var listenerObj of toRemove) {
stateNode.canonical.removeEventListener_unstable(registrationName, listenerObj.listener, listenerObj.capture);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't have the context on the background of the APIs being added in this PR. But one thing that jumps out to me is that it seems like a hazard that calling getListener has a side effect of modifying the listeners. This seems very easy to accidentally mess up from the caller side because it looks like a pure function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a suggestion here? I agree - but currently with the way this API is used, these listeners are guaranteed to be called in the same frame; and if the semantic is "once", then we need to remove the listener or somehow mark it as "used". Keeping it around forever with a dirty flag doesn't seem good either for memory reasons.

The "once" flag is a (w3c) standard flag, so it's necessary and useful to make sure it works properly.

Thanks for the feedback.

Copy link
Collaborator

@gaearon gaearon Feb 11, 2022

Choose a reason for hiding this comment

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

I think my instinct would be to wrap the listeners that have { once: true } with code that removes the listener imperatively when it actually fires. But make sure removeEventListener still works when the original function is passed. Then the special case is rare and the getListener function stays pure.

Semantic question: if propagation was prevented by a handler above, does the parent handler with {once: true} get removed or not? Seems like this would affect how we implement it. getListener getting called does not guarantee the listener itself will get called.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this shows that in the DOM, the { once: true } parent listener does not get removed if the child stopped propagation: https://codesandbox.io/s/serene-minsky-380ki?file=/src/index.js

This means that getListener() is too early to remove the once listener. We can't do this until we're actually firing listeners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very fair and great feedback, I can make that change (don't remove until it's actually executed).

}
}
const props = getFiberCurrentPropsFromNode(stateNode);
if (props === null) {
// Work in progress.

if (listeners.length === 0) {
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 (listeners.length === 1) {
return listeners[0];
}

return listener;
// We need to call at least 2 event handlers
return function () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a bit concerning we're creating an extra function per listener as we're accumulating them across the tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this was easier for prototyping but I think I need to change the signature of this function to return an array of Functions.

// any arguments that are passed to the event handlers
var args = Array.prototype.slice.call(arguments);

for (var i = 0; i < listeners.length; i++) {
listeners[i] && listeners[i].apply(null, args);
}
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* Copyright (c) Facebook, Inc. and its 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';