rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

how to expose component ref for react-navigation HOC?

Open slorber opened this issue 7 years ago • 8 comments

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

slorber avatar Feb 14 '18 10:02 slorber

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.

satya164 avatar Feb 14 '18 10:02 satya164

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

slorber avatar Feb 14 '18 10:02 slorber

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 avatar Feb 14 '18 12:02 timdorr

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

satya164 avatar Feb 14 '18 12:02 satya164

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 avatar Feb 14 '18 12:02 timdorr

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

satya164 avatar Feb 14 '18 12:02 satya164

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

timdorr avatar Feb 14 '18 12:02 timdorr

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

satya164 avatar Mar 07 '18 22:03 satya164