-
Notifications
You must be signed in to change notification settings - Fork 49k
ReactNativeBridgeEventPlugin supports lazily-registered event types #10679
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
ReactNativeBridgeEventPlugin supports lazily-registered event types #10679
Conversation
This accompanies changes on the native side to now specify which event types each view manager supports as part of its view config. Ultimately the goal is to lazily initialize view managers on the native side as well.
Friendly ping 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. A few inlines.
If we're going to have things require ReactNativeBridgeEventPlugin directly, can we remove RCTEventEmitter.register from the injection file (and delete that module entirely)?
ResponderEventPlugin = require('ResponderEventPlugin'); | ||
UIManager = require('UIManager'); | ||
createReactNativeComponentClass = require('createReactNativeComponentClass'); | ||
}); | ||
|
||
it('fails if unknown/unsupported event types are dispatch', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dispatched
ResponderEventPlugin = require('ResponderEventPlugin'); | ||
UIManager = require('UIManager'); | ||
createReactNativeComponentClass = require('createReactNativeComponentClass'); | ||
}); | ||
|
||
it('fails if unknown/unsupported event types are dispatch', () => { | ||
expect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the .receiveTouches that you expect to throw? If so, can we move the other calls out? An expect() failure also throws an exception – so for example, if RCTEventEmitter.register was never called, or the snapshot didn't match, this test would pass. Even better if you can include the full exception message to ensure it's the error you think it is and not a different bug in receiveTouches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Yes, good suggestion.
|
||
for (const topLevelType in bubblingEventTypes) { | ||
if (customBubblingEventTypes[topLevelType] == null) { | ||
ReactNativeBridgeEventPlugin.eventTypes[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that you'd need to call publishEventForPlugin in order for the event system to work properly. I guess we might only use the fields that populates (ex: EventPluginRegistry.possibleRegistrationNames) for DOM – but can we call publishEventForPlugin from this function too to make sure these are (mostly) going through the same paths as before and the same paths as if the event types are available on startup? Mostly in case we change how event registration works later and forget to update this.
It will validate that you don't supply the same event name twice but since you have the customBubblingEventTypes[topLevelType] == null
check it should be OK I think.
This should help ease transition for pre-existing JS-only componetns.
I'm not sure I understand this comment. I think I might be misunderstanding something here. 😄 |
I thought RCTEventEmitter.receiveEvent calls to ReactNativeEventEmitter.receiveEvent – that's how events come in – and I was wondering if that indirection could be removed. I guess it calls from native though, not JS, and those are a little different from yours which is used in requireNativeComponent. Probably fine to leave as-is then but I thought the inversion was always a little odd instead of native requiring ReactNativeEventEmitter directly. |
Ah, I think I understand. If you're okay with it, maybe we can follow up with additional cleanup like that? |
Yes, up to you! We also don't need to clean up if you think it's not important. Can you make the other two changes though? |
Yup, those were already made before I left my comment 😄 |
Gah! Apparently I forgot to push them though. 😊 Whoops. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanku
Previously, React Native loaded event type info from
UIManager
during initialization to configure the event plugin. This communication withUIManager
caused Prepack deopts and was removed with #10567 in favor of hard-coded event types.Unfortunately this change had the unexpected result of blocking custom (3rd party) view managers from defining their own event types. After some consideration, I believe the best path forward is to lazily read event types for each view manager. (This approach unlocks the ability to lazily initialize view managers on the native side as well.)
This PR deletes the hard-coded event list (formerly
ReactNativeEventTypes
) and adds a new method ,processEventTypes
, toReactNativeBridgeEventPlugin
(to be called byrequireNativeComponent
each time a new view manager is initialized). I've also updated event-related tests to roughly mimic what happens in the native context.