[WIP] React Native: Turn HostComponent into an EventEmitter #23278
Conversation
|
Comparing: 9b5e051...a994a3a Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| } | ||
| } | ||
| for (var listenerObj of toRemove) { | ||
| stateNode.canonical.removeEventListener_unstable(registrationName, listenerObj.listener, listenerObj.capture); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Very fair and great feedback, I can make that change (don't remove until it's actually executed).
| } | ||
|
|
||
| return listener; | ||
| // We need to call at least 2 event handlers | ||
| return function () { |
There was a problem hiding this comment.
Seems a bit concerning we're creating an extra function per listener as we're accumulating them across the tree.
There was a problem hiding this comment.
Yeah, this was easier for prototyping but I think I need to change the signature of this function to return an array of Functions.
|
Where can I read about the background for these changes? I'm curious if we're adding something to each instance that we expect to be used widely, or if it's meant as a compat layer but we expect that most apps will only have a few calls to these. If it's mostly for compat, it might be beneficial to avoid allocations until it's used (e.g. begin with event listeners set to null instead of empty object, skip the |
|
@gaearon There's no central design doc yet, we've been having some discussions internally. tl;dr is that we're trying to make HostComponent act more like a DOM node and in particular adhere to the Event interface (see links to spec in code). I think you're right about the optimization of setting listeners to null initially. That makes sense and I can easily do that. |
…t supports the RN Event polyfill
…istener to add to it - to avoid creating empty arrays that aren't ever used in a hot path
…istener to add to it - to avoid creating empty arrays that aren't ever used in a hot path
There was a problem hiding this comment.
I don't feel comfortable accepting it without a look from @sebmarkbage. I wrote a few comments for the places that I'm not sure about. Hope this is helpful!
| @@ -122,6 +145,7 @@ class ReactFabricHostComponent { | |||
| this.viewConfig = viewConfig; | |||
| this.currentProps = props; | |||
| this._internalInstanceHandle = internalInstanceHandle; | |||
| this._eventListeners = null; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
| // * 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
this direction seems like a reversal of that strategy
The other option was to do addEventListener(hostElement, ...)
| return; | ||
| } | ||
|
|
||
| eventListeners[eventType] = namedEventListeners.filter(listenerObj => { |
There was a problem hiding this comment.
Is this allocation unavoidable? remove/add resubscriptions can be a hot path. It would be nice if removing and adding was very cheap.
| event._dispatchListeners = accumulateInto( | ||
| event._dispatchListeners, | ||
| listener, | ||
| listeners, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Tried to address this in the next PR by returning T | Array | null from getEventListeners and avoid allocations more aggressively
| const insts = listeners.map(() => { | ||
| return inst; | ||
| }); | ||
| event._dispatchInstances = accumulateInto(event._dispatchInstances, insts); |
There was a problem hiding this comment.
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.
| @@ -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) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I believe this PR is mostly orthogonal.
This is for capturable-but-not-bubbling CustomEvents since those are legal under the W3C spec.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm submitting a new PR that just adds addEventListener/removeEventListener, so we don't need any code related to CustomEvent for now.
| 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( |
There was a problem hiding this comment.
Same comments about getListeners => pushListeners or something to prevent unnecessary allocation and later unnecessary accumulateInto checks.
| @@ -77,6 +77,11 @@ function SyntheticEvent( | |||
| this._dispatchListeners = null; | |||
| this._dispatchInstances = null; | |||
|
|
|||
| // React Native Event polyfill - enable Event proxying calls to SyntheticEvent | |||
| if (nativeEvent.setSyntheticEvent) { | |||
There was a problem hiding this comment.
I'm not sure what this checks for, but I worry about potential deopts here. This is very hot code. If we're checking non-existent property, if I recall correctly it can trigger make it slower. I'm also not sure I understand the purpose of "binding" a native event object back to this one. For example, in DOM we don't need to do that. What's different?
There was a problem hiding this comment.
We have two APIs we're trying to support now - the props-based one that takes a SyntheticEvent, and the W3C-compliant EventEmitter one that expects an Event/CustomEvent. If a CustomEvent is passed through it gets wrapped in SyntheticEvent by React, but then we actually want calls on one to impact the other, so we need some mechanism to keep them synced.
My approach is admittedly pretty brute-force. There might be a cleaner way to keep the two in sync.
| // at runtime. We should instead inject the version number as part of the build | ||
| // process, and use the ReactVersions.js module as the single source of truth. | ||
| export default '17.0.3'; | ||
| export default '18.0.0-rc.0-experimental-049a50b73-20220214'; |
Summary
The primary goal is to make the Fabric HostComponent into a W3C spec-compliant EventEmitter.
TODO: filling this out more in an hour or so
Open questions for later:
Eventclass live for React Native? A: this will live in React Native, in this repo we'll assume Event exists on the global scopeEvent(not SyntheticEvent) is passed to the event handler. However, this could mean that, for native events passed to an event listener, they either (1) need to be passed the SyntheticEvent or (2) won't be able to stop bubbling, propagation, etc, on native events only.How did you test this change?
tbd