createAnimatedComponent should use forwardRef ?

[slorber]

For Discussion

Hi,

Currently createAnimatedComponent does not use forwardRef.
This is probably the case because we actually sometimes need access to the AnimatedComponent instance.

I propose to use forwardRef by default, and to use another prop like animatedRef to get a reference to the animated wrapper, as it is probably a less common usecase (not sure of that but it looks to me)

Currently, we need to call getNode on the animated component. This was added by @gre who seems to agree for using forwardRef by default (#9944 (comment))

This would simplify client code.

For example in my case I created a ScrollView HOC react-native-scroll-into-view, and it must work with both animated and normal scrollviews.

This forces me to provide a fragile implementation like this to be sure to I can use the ScrollView imperative methods to get the responder:

  getScrollViewNode: ref => {
    if ( ref.getNode ) {
      return ref.getNode();
    }
    else {
      return ref;
    }
  };

[satya164]

FYI this will break all existing libraries which rely on getNode currently and they will need to be updated.

[kelset]

If you check the latest ~10 commits in master you can see that they tried adding forwardRef support - but then reverted it. So not sure tbh

[TheSavior]

This probably makes sense but it would be useful to see a very small complete sample that will help us understand how this is being used and what value you are looking for from both of the refs you are proposing.

[slorber]

hi @TheSavior I'm not sure what you are expecting from me. My personal opinion is that it would be better to have animated and non-animated components return the same kind of ref, so that people do not have to think about that at all (end users or library authors). Not sure what kind of example you want me to provide.

Yes that would be a breaking change, it is annoying and will break code for sure but I think this kind of usecase is the point of forwardRef in the first place, will be better in the long run, and anyway, any library that want to migrate to forwardRef (react-intl, react-redux or anything else) will probably have to deal with a breaking change for that

[satya164]

I don't think breaking all the libraries out there without any deprecation notice is a good idea.

any library that want to migrate to forwardRef (react-intl, react-redux or anything else) will probably have to deal with a breaking change for that

Yeah, but react-native is not just a library, it's a framework. The change is more invasive and will break many libraries in its ecosystem. Whereas react-redux breaking this behavior will mostly require only the app code to be upgraded which is under user's control.

I'm not against change, but we need to figure out a proper deprecation path before changing the API.

[slorber]

yes I totally agree with that, here it's just to validate what we want in the future, I'm not asking for a breaking change right now :)

[TheSavior]

Got it, I understand better now. Thanks for helping me get there. I think I agree that we should have a supported way to get the underlying node, we just need a good plan. 👍

[gavrix]

There is a workaround to make AnimatedComponent that forwards ref of its wrapped component:

export default function createAnimatedComponentFrowardingRef<P, S>(Component: React.ComponentClass<P, S>) {
  return React.forwardRef<React.Component<P, S>, P>((props, ref) => {
    class Wrapper extends React.Component<P, S> {
      render() {
        return <Component {...this.props} ref={ref} />
      }
    }
    const AnimatedWrapper = Animated.createAnimatedComponent(Wrapper)
    return <AnimatedWrapper {...props} />
  })
}

[slorber]

Great, didn't think about recreating the raw animated compos this way, makes sense to do that in userland.

keeping this issue open for discussion whether or not react should automatically forward refs in the long term

[cpojer]

I'm gonna close this, as the remaining discussion should probably be handled somewhere else (React repo?) and we aren't planning on changing this API on RN itself right now. If you feel strongly, please do come forward with a plan and start implementing it in PRs.

[numandev1]

it is giving me this error @gavrix

[numandev1]

you can use
let getRef = this.refs.scroll.getNode();
getRef.function({ index: index });