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
how to expose component ref for react-navigation HOC? #22
Comments
I think
I think it's unnecessary. We just need to check if the component is a function component and not add the ref in that case. |
You mean checking for:
And if it's a class, automatically ask for ref? ping @gaearon @markerikson @timdorr @ericf as this solution might be backported in existing HOC libs |
We shouldn't create it if we don't need it. It's a wasted effort, especially if you have a large number of connected/HOC'ed components. |
@timdorr can you elaborate? does it have any performance implications? would be great if you could post a link where I can read more. |
There's a new API coming for refs that would make the instantiation of the ref explicit, and therefore it's upfront cost as well: https://github.com/reactjs/rfcs/blob/master/text/0017-new-create-ref.md |
@timdorr it's just a convenience API for the callback ref, right? we are not planning to use that API right now. but what is the cost? memory usage or attaching a ref deopts something? |
I don't know specifically. There's no code to go on. But one would assume createRef has some sort of cost associated with it. So, creating that ref and never using it would be a wasted effort. I'm not trying to make it into a big deal, just pointing out the inefficiency (no matter how trivial). |
This question has been raised on PR: react-navigation/react-navigation#3512
Should we provide
onRef
prop (current solution) or use something similar to react-redux, react-intl, and store ref inside HOC, and expose agetWrappedInstance
method? Does the HOC needwithRef: true
config for that?Here is how it's done in other libraries:
https://github.com/callstack/react-native-paper/blob/master/src/core/withTheme.js#L84
https://github.com/callstack/react-native-paper/blob/master/src/core/withTheme.js#L114
https://github.com/reactjs/react-redux/blob/76dd7faa90981dd2f9efa76f3e2f26ecf2c12cf7/src/components/connectAdvanced.js#L71
https://github.com/reactjs/react-redux/blob/76dd7faa90981dd2f9efa76f3e2f26ecf2c12cf7/src/components/connectAdvanced.js#L176
https://github.com/yahoo/react-intl/blob/f82383e02cb0d38dfd28649dc2442a2ea2796a44/src/inject.js#L36
https://github.com/yahoo/react-intl/blob/f82383e02cb0d38dfd28649dc2442a2ea2796a44/src/inject.js#L20
cc @satya164 @brentvatne @ericvicenti
The text was updated successfully, but these errors were encountered: