Skip to content

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

Merged
merged 3 commits into from
Sep 14, 2017

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Sep 12, 2017

Previously, React Native loaded event type info from UIManager during initialization to configure the event plugin. This communication with UIManager 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, to ReactNativeBridgeEventPlugin (to be called by requireNativeComponent each time a new view manager is initialized). I've also updated event-related tests to roughly mimic what happens in the native context.

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.
@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 13, 2017

Friendly ping 😄

Copy link
Collaborator

@sophiebits sophiebits left a 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', () => {
Copy link
Collaborator

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(() => {
Copy link
Collaborator

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.

Copy link
Contributor Author

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[
Copy link
Collaborator

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.
@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 14, 2017

If we're going to have things require ReactNativeBridgeEventPlugin directly, can we remove RCTEventEmitter.register from the injection file (and delete that module entirely)?

I'm not sure I understand this comment. RCTEventEmitter is the RN renderer's point of injection into the react-native codebase. ReactNativeBridgeEventPlugin is just where we track the event type configuration (and only requireNativeComponent should require ReactNativeBridgeEventPlugin).

I think I might be misunderstanding something here. 😄

@sophiebits
Copy link
Collaborator

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.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 14, 2017

Ah, I think I understand.

If you're okay with it, maybe we can follow up with additional cleanup like that?

@sophiebits
Copy link
Collaborator

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?

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 14, 2017

Yup, those were already made before I left my comment 😄

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 14, 2017

Gah! Apparently I forgot to push them though. 😊 Whoops.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

thanku

@bvaughn bvaughn merged commit 836d1c5 into facebook:master Sep 14, 2017
@bvaughn bvaughn deleted the custom-view-manager-event-types branch September 14, 2017 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants