-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Adding ReactNative.setNativeProps that takes a ref #14907
Conversation
// Module provided by RN: | ||
import UIManager from 'UIManager'; | ||
|
||
export function setNativeProps(handle: any, nativeProps: Object) { |
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 there a better flow type I should be using for handle
?
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 used to pretend that native host instances were ReactNativeComponent. That’s not technically true because they’re not instanceof
.
With forwardRef, we get the inner type. I would’ve expected that you have to supply the ref type to Flow. What does Flow think the type of a ref is?
Ideally it would be an opaque type which will help catch anyone trying to call something else on it.
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.
Alright, seems like a problem for another PR at another time. :)
Details of bundled changes.Comparing: 4f4aa69...ada38e2 react-native-renderer
Generated by 🚫 dangerJS |
Prettier. |
@@ -0,0 +1,43 @@ | |||
/** |
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.
Since you’re going to add more of these, can we name it something more general so we don’t need a file per method?
As part of react-native-community/discussions-and-proposals#72 this adds a new top level API on ReactNative that we can use to setNativeProps.
This can only be called with a ref to a native component and will warn/no-op if called with an invalid component ref.
Other than jest tests, I also verified this worked by running
and copied the output files into FB's codebase. I added a console.warn to ensure it was using the renderer I copied in, modified
react-native-implementation.js
to expose this and then verifiedsetNativeProps
worked correctly from the top level for the paper renderer via a playground app.