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.

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

[RN] Consolidate eventTypes registry with view configs #12556

Merged
merged 1 commit into from
Apr 10, 2018

Conversation

sebmarkbage
Copy link
Collaborator

We already have one stateful module that contains all the view config.
We might as well store the event types there too. That way the shared
state is compartmentalized (and I can move it out in a follow up PR).

The view config registry also already has an appropriate place to call
processEventTypes so now we no longer have to do it in RN.

Will follow up with a PR to RN to remove that call.

@@ -49,6 +90,7 @@ export function get(name: string): ReactNativeBaseComponentViewConfig {
);
viewConfigCallbacks.set(name, null);
viewConfig = callback();
processEventTypes(viewConfig);
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Apr 6, 2018

Choose a reason for hiding this comment

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

This is what lets us avoid exposing ReactNativeBridgeEventPlugin.

@facebook facebook deleted a comment Apr 6, 2018
@sebmarkbage sebmarkbage changed the title Consolidate eventTypes registry with view configs [RNConsolidate eventTypes registry with view configs Apr 8, 2018
@sebmarkbage sebmarkbage changed the title [RNConsolidate eventTypes registry with view configs [RN] Consolidate eventTypes registry with view configs Apr 8, 2018
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I think this should be okay, so long as requireNativeComponent is updated before it lands.

@@ -84,7 +83,6 @@ const ReactFabric: ReactNativeType = {
// Used as a mixin in many createClass-based components
NativeMethodsMixin,
// Used by react-native-github/Libraries/ components
ReactNativeBridgeEventPlugin, // requireNativeComponent
Copy link
Contributor

Choose a reason for hiding this comment

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

We can also delete the forwarding scripts/rollup/shims/react-native/ReactNativeBridgeEventPlugin.js now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just get rid of the ReactNativeBridgeEventPlugin entirely now and just move extractEvents over into the view config registry as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch on the shim. I'll delete it.

@sebmarkbage
Copy link
Collaborator Author

extractEvents is part of the event plugin mechanism and is conceptually different.

The registry is just a store of configurable fields and doesn’t change unless we change how RN talks to native.

Extract events goes into the synthetic event system, plugin order and that whole system. A system that we know we’ll want to change and when we do change it, it’ll affect everything in the React repo and nothing in the RN repo so it’s coupled to React.

@bvaughn
Copy link
Contributor

bvaughn commented Apr 9, 2018

Gotcha.

We already have one stateful module that contains all the view config.
We might as well store the event types there too. That way the shared
state is compartmentalized (and I can move it out in a follow up PR).

The view config registry also already has an appropriate place to call
processEventTypes so now we no longer have to do it in RN.

Will follow up with a PR to RN to remove that call.
@sebmarkbage sebmarkbage merged commit b6e0512 into facebook:master Apr 10, 2018
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
We already have one stateful module that contains all the view config.
We might as well store the event types there too. That way the shared
state is compartmentalized (and I can move it out in a follow up PR).

The view config registry also already has an appropriate place to call
processEventTypes so now we no longer have to do it in RN.

Will follow up with a PR to RN to remove that call.
@janicduplessis
Copy link

@sebmarkbage @bvaughn We're using ReactNativeBridgeEventPlugin.processEventTypes in react-native-gesture-handler to register a custom event type for views (see here and here). There isn't a particular native component associated with these events so we cannot use requireNativeComponent (we used to register a dummy native component as a hack but now events are registered lazily so it doesn't work).

Is there an alternative I'm not seeing or could we bring this back as public'ish API (as public as importing react-native/Libraries/Renderer/shims/ReactNativeBridgeEventPlugin gets 😄)?

@gaearon
Copy link
Collaborator

gaearon commented Apr 29, 2018

This is a bit ironic because shims was added as a temporary measure to expose private APIs to RN itself. Didn’t even occur to me third-party libs would attempt to import those 😛

@janicduplessis
Copy link

Yea it is a very low level library and RN doesn't export anything to be able to do that so some creativity was needed. A solution that could also work is to expose a root HOC that renders a native component that registers the view types but it seems like quite a bit of overhead just to register the events (both the extra API in react-native-gesture-handler and the runtime overhead of the extra view).

If we decide to bring this back we could add it as extra RN public API so it is less likely to break in the future, although I'm not familiar enough with this API to know how stable it is. Something like an undocumented UNSTABLE_registerExtraEventType might be good.

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.

5 participants