Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 27 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
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
32 changes: 20 additions & 12 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,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 @@ -125,7 +127,7 @@ class ReactFabricHostComponent {
viewConfig: ViewConfig;
currentProps: Props;
_internalInstanceHandle: Object;
_eventListeners: { [string]: $ReadOnly<{| listener: EventListener, options: EventListenerOptions |}>[] };
_eventListeners: ?InternalEventListeners;

constructor(
tag: number,
Expand All @@ -137,7 +139,7 @@ class ReactFabricHostComponent {
this.viewConfig = viewConfig;
this.currentProps = props;
this._internalInstanceHandle = internalInstanceHandle;
this._eventListeners = {};
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 @@ -236,16 +238,22 @@ class ReactFabricHostComponent {
const passive = optionsObj.passive || false;
const signal = null; // TODO: implement signal/AbortSignal

this._eventListeners[eventType] = this._eventListeners[eventType] || [];
this._eventListeners[eventType].push({
listener: listener,
options: {
capture: capture,
once: once,
passive: passive,
signal: signal
}
});
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
Expand Down
2 changes: 1 addition & 1 deletion packages/react-native-renderer/src/ReactNativeGetListener.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ export default function getListener(
}

// Get imperative event listeners for this event
if (stateNode.canonical._eventListeners[registrationName] && stateNode.canonical._eventListeners[registrationName].length > 0) {
if (stateNode.canonical?._eventListeners?.[registrationName]?.length > 0) {
JoshuaGross marked this conversation as resolved.
Show resolved Hide resolved
var toRemove = [];
for (var listenerObj of stateNode.canonical._eventListeners[registrationName]) {
// Make sure phase of listener matches requested phase
Expand Down