ReactNativeBridgeEventPlugin supports lazily-registered event types#10679
ReactNativeBridgeEventPlugin supports lazily-registered event types#10679bvaughn merged 3 commits intofacebook:masterfrom
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 😄 |
sophiebits
left a comment
sophiebits
left a comment
There was a problem hiding this comment.
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)?
| createReactNativeComponentClass = require('createReactNativeComponentClass'); | ||
| }); | ||
|
|
||
| it('fails if unknown/unsupported event types are dispatch', () => { |
| }); | ||
|
|
||
| it('fails if unknown/unsupported event types are dispatch', () => { | ||
| expect(() => { |
There was a problem hiding this comment.
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.
Ah! Yes, good suggestion.
|
|
||
| for (const topLevelType in bubblingEventTypes) { | ||
| if (customBubblingEventTypes[topLevelType] == null) { | ||
| ReactNativeBridgeEventPlugin.eventTypes[ |
There was a problem hiding this comment.
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. |
3 participants
Previously, React Native loaded event type info from
UIManagerduring initialization to configure the event plugin. This communication withUIManagercaused 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 byrequireNativeComponenteach time a new view manager is initialized). I've also updated event-related tests to roughly mimic what happens in the native context.