-
Notifications
You must be signed in to change notification settings - Fork 125
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
Migrate methods like setNativeProps
off of refs.
#72
Comments
Re: migration in userland, I think that a custom hook based off of The user would then use the hook to make a This hypothetical hook would be provided either as part of react native, or be located in another package. Would be strictly for userland, as I think that moving all of the callsites inside of RN to the "new-style" should be fairly easy. // The custom hook that would sit in RNCore
// The second hypothetical argument is for native components
// that have their own custom methods, like TextInput
function useNativeMethods(ref, extraMethods = () => {}) {
useImperativeMethods(ref, () => ({
...extraMethods(),
setNativeProps: (props) => {
setNativeProps(inputRef.current, props);
},
// Rinse and repeat for the other methods
}));
}
// A wrapper that a user would write. The component that depended
// on `ref.focus()`, etc would be modified to use
// `MyTextInput` rather than `TextInput`
function MyTextInput(props, ref) {
const inputRef = useRef();
useNativeMethods(ref, () => ({
focus: () => {
focus(inputRef.current);
},
blur: () => {
blur(inputRef.current);
}
}));
return <TextInput ref={inputRef} ...props />;
}
MyTextInput = forwardRef(MyTextInput); Also, I wonder if during this transition period these callback-based APIs could be converted over to using promises:
|
Interesting thought on providing a hook. I’m not sure how this would help people migrate though. It seems like it would require a migration to that API vs just changing the call to setNativeProps or whatever other Native method. Is there something I’m overlooking? Also, we still can’t use hooks yet in RN core until React ships a stable release or we are willing to ship an alpha in RN. |
This comment has been minimized.
This comment has been minimized.
Yep, yep. I’m familiar with useImperativeMethods. I’m just not sure how adding a hook for people to use helps us migrate everything to top level methods exposed from the public API of React-Native. |
Yeah I'm not sure either. The hooks you're proposing is useful in userland components which expose |
Yeah sorry, I wasn't clear - the hook idea is strictly for userland code to help users migrate a component that was heavily dependent on using the old imperative methods, like edit: reworded my original comment on this to hopefully clarify |
If I understood correctly it will be a great benefit for apps with complex components tree. Go on with less wrappers around! 😄 |
I have little context about this but I'm curious to understand more about the proposed API: isn't it effectively the same as |
I think this is a question best answered by @sebmarkbage, however I think he’s told me a couple of times so I’ll try to repeat. Yes, this is a similar API to ReactDOM.findDOMNode in terms of looks. However,y understanding is that the problem isn’t where the api is exposed but rather that it is bad practice to specifically have an API that behaves like findDOMNode. My understanding is that the equivalent in RN is doing operations on a reactTag which I think I remember someone saying something about how we want to change that in Fabric. But that is a separate topic, there is no concern about these specific methods in the proposal existing at the top level and taking a ref as an argument. Cc @shergin as I believe he has some context too. |
There are a number of problems with findNodeHandle. Hooks doesn't really solve any of them but it's basically just hard deprecated in StrictMode. With a possible replacement alternative RFC. One thing that this proposal doesn't solve is the ability to constrain which methods are allowed on a particular abstraction, without adding runtime overhead. This is similar to how findNodeHandle lets you do anything you want with the handle which breaks abstractions. However, that can be solved at the type system level by exposing subtypes or opaque types that can only be used with certain methods. I wrote this for a separate context but it's relevant for this thread so I'll post it here too... There will always be some object (e.g. shadow node) or tag that represents the native instance. If we use virtual dispatch, the problem is that we need to now create a special wrapper object for this thing, instead of the "shadow node" that we get from native or just a raw pointer. Just to hold all the built-in ones. For things like
Sometimes there needs to be a special method like
However, ofc, it also needs the "built-in" ones so you end up spreading those in too.
Mixin-hell. Creates extra hooks state that needs to be preserved. We know objects in general are hard to optimize. Any dispatch like this can't be easily inlined since essentially any virtual dispatch through an object can't be known (devirtualized). What if the standard React convention was to use static function style like we do for everything else, insetad of OO-style?
That way we don't have to clear any extra wrapper objects in the whole system and we don't have to manage any extra hooks state in wrappers:
And to use it:
Making sure that you only pass TextInput refs to However, there are still use cases for something like
In this case you might want to expose a tuple of values. This is the unusual case but can still be supported. Note: I had to shift to calling focus() on the TextInput module here. So some of the "standard API" problems occur here but this is the unusual case and Flow coverage will handle this. Sometimes I might also need to reference state:
In this model, the name It's just a reference to some opaque object. That's why we'll likely rename it to |
Thanks for that context @sebmarkbage. So, concretely it sounds like instead of these APIs being exports from ReactNative they should be exports from the individual components. That makes sense for things like clear, focus, and blur. Would you expect setNativeProps and the measure functions to be exported from view? |
Yes. E.g. the measure function doesn't make sense on ReactART components and similar things with different or no layout.
|
Summary: See react-native-community/discussions-and-proposals#72 for more information. This shouldn't be a user facing change. Reviewed By: shergin Differential Revision: D14176217 fbshipit-source-id: 784739e5a69a98ddd1d5db75ee5267b7459e477b
I believe we have sufficiently discussed this issue and @TheSavior has moved forward with changes to React/React Native that we can close this issue. |
Context
React Native has native components like RCTView, RCTText, RCTImage, etc that are implemented in native code. When JS code tries to render them, it is done by rendering a string:
React knows that components that are strings are special native components, vs functions or classes which are JS components. When these special native components are rendered, React creates "host" components in JS that add some additional behavior and metadata. This includes methods like
blur
,focus
,measure
,measureInWindow
,measureLayout
, andsetNativeProps
.Problems
Every time a native component is rendered, React has to construct a wrapper class for this node that contains these methods because React doesn't know if these methods will be accessed on a ref of these components.
Creating these classes results in additional memory usage and runtime perf overhead. There are also a couple of different situations where these methods can be created which leads to a bunch of similar code, bloating the default React Native bundle. Some examples include
NativeMethodsMixin
,ReactFabricHostConfig.js
,ReactNativeComponent.js
, andReactNativeFiberHostComponent.js
.Furthermore, there can be complexities when creating JS components that render to these native components, wanting to provide custom imperative methods while also providing access to the native component methods. For example, native components have
focus
andblur
methods, but if the JS TextInput component wants to expose aclear
method, it isn't able to forward the ref to the native component because that wouldn't includeclear
, and it can't not useforwardRef
because then it wouldn't includefocus
andblur
. This has led us to try to duplicate all the native methods in the JS component as passthrough methods to the underlying ref. Another common approach to this problem is by having an imperative method likegetNativeRef()
. This approach also isn't great because it leaks implementation details and is prone to breakages when refactoring. Definitely not ideal.Proposal
Instead of having to create a wrapper class for every native component just in case one of these methods is called, we propose moving these methods to be an export from
ReactNative
. That means instead of calling the methods on the ref, you would pass the ref to the publicly exported functions.Concretely, this would be an api change from calling setNativeProps like this:
to
The implementation of these methods would still be defined in the ReactNativeRenderer which is required because the implementation for Fabric is different than the current one for Paper. They would be exported from react-native's public API as a re-export from the renderer. This is currently what we do to export
findNodeHandle
Thoughts on Implementation
I think we can make this migration incrementally by first creating the new versions of these methods that take a ref as an argument. We would want this implementation to be the source of truth for the methods instead of having these just call the method on the ref.
We can then change the
NativeMethodsMixin
andHostComponents
to call the top level API. This also gives us a layer at which to add a deprecation warning to ask out of repo callsites to migrate.Once we decide to finally remove the old implementation we can clean up the renderers removing the old approach and realize the performance and memory wins.
cc @sebmarkbage
The text was updated successfully, but these errors were encountered: