react-navigation / rfcs

RFCs for changes to React Navigation
Other
88 stars 16 forks source link

how to expose component ref for react-navigation HOC? #22

Open slorber opened 6 years ago

slorber commented 6 years ago

This question has been raised on PR: https://github.com/react-navigation/react-navigation/pull/3512

Should we provide onRef prop (current solution) or use something similar to react-redux, react-intl, and store ref inside HOC, and expose a getWrappedInstance method? Does the HOC need withRef: 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

satya164 commented 6 years ago

I think onRef is a nicer API, but maybe getWrappedInstance is a better solution since the community seems to have adopted it already.

Does the HOC need withRef: true config for that

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.

slorber commented 6 years ago

You mean checking for:

const isClassComponent = (Component: Function) => !!Component.prototype.render; ?

And if it's a class, automatically ask for ref? Does it have any performance cost or any side effect to ask a ref even when client does not need it?

ping @gaearon @markerikson @timdorr @ericf as this solution might be backported in existing HOC libs

timdorr commented 6 years ago

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.

satya164 commented 6 years ago

@timdorr can you elaborate? does it have any performance implications? would be great if you could post a link where I can read more.

timdorr commented 6 years ago

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

satya164 commented 6 years ago

@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?

timdorr commented 6 years ago

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).

satya164 commented 6 years ago

https://github.com/bvaughn/rfcs/blob/ref-forwarding/text/0000-ref-forwarding.md