-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Conversation
@@ -49,6 +90,7 @@ export function get(name: string): ReactNativeBaseComponentViewConfig { | |||
); | |||
viewConfigCallbacks.set(name, null); | |||
viewConfig = callback(); | |||
processEventTypes(viewConfig); |
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.
This is what lets us avoid exposing ReactNativeBridgeEventPlugin.
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 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 |
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.
We can also delete the forwarding scripts/rollup/shims/react-native/ReactNativeBridgeEventPlugin.js
now?
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.
Should we just get rid of the ReactNativeBridgeEventPlugin
entirely now and just move extractEvents
over into the view config registry as well?
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.
Good catch on the shim. I'll delete it.
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. |
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.
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 @bvaughn We're using Is there an alternative I'm not seeing or could we bring this back as public'ish API (as public as importing |
This is a bit ironic because |
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 |
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.