connect() does not work with React.forwardRef

[malectro]

Specifically, if I compose connect with another HOC that uses forwardRef, the result of forwardRef (an object) is passed as the WrappedComponent, which throws an error claiming it should only be a function.

Example

const MyComponent = React.forwardRef(() => null);
connect()(MyComponent);

[gaearon]

The fix would be to use require('react-is').isValidElementType() instead of a strict function check.
See facebook/react#12453.

react-is is a new package we just published.

[gaearon]

(We'd happily take a PR that adds an ESM build to react-is. We're using Rollup so it should be easy to expose it.)

[malectro]

Does passing a forwarded ref type also break displayName and hoistStatics as well?

Edit: (I tried making a PR for this but realized it wasn't very simple.)

[bvaughn]

Does passing a forwarded ref type also break displayName and hoistStatics as well?

Yes.

[gaearon]

We chatted with @sebmarkbage and he pointed out that if you want to "redirect" ref to some inner component, forwardRef needs to be outside of connect, not inside it.

class MyComponent extends React.Component {
  render() {
    return <SomeInnerComponent ref={this.props.myForwardedRef} />;
  }
}

const ConnectedMyComponent = connect(
  mapStateToProps
)(MyComponent);

export default forwardRef((props, ref) =>
  <ConnectedMyComponent {...props} myForwardedRef={ref} />
);

There is also another scenario in which you might want to point connect()ed component's ref to the wrapped component's own instance (MyComponent in this example). Arguably this is what most React Redux users would want. You can't do this in userland. It would need to be done by connect() itself because it would need to pass ref={forwardedRef} to the wrapped instance (which isn't possible from the user code).

[malectro]

So it sounds like the solution is to avoid returning forwarded-ref components from HOCs when they're composed and have the user call it as needed?

I'm not totally sure if this example works with the ref being in a stateless component.

const MyConnectedComponent = _.flow(
  connect(mapStateToProps),
  myHoc(),
)(({myForwardedRef, ...props}) => <MyComponent ref={props.myForwardedRef} {...props} />);

export default forwardRef((props, ref) =>
  <ConnectedMyComponent {...props} myForwardedRef={ref} />
);

[timdorr]

Do we not pass the ref prop through? In which case, wouldn't this work?

class MyComponent extends React.Component {
  render() {
    return <SomeInnerComponent ref={this.props.myForwardedRef} />;
  }
}

const ConnectedMyComponent = connect(
  mapStateToProps
)(MyComponent);

export default forwardRef((props, ref) =>
  <ConnectedMyComponent {...props} ref={ref} />
);

[ovidiuch]

You can't do this in userland. It would need to be done by connect() itself because it would need to pass ref={forwardedRef} to the wrapped instance (which isn't possible from the user code).

For a user like me, this sounds ideal. Is this desirable from your perspective? (Redux core maintainers)

[gaearon]

It makes sense to me that React Redux should do this. But it would be a breaking change.

[timdorr]

Again, we already do this. Otherwise, this test would fail along with a few others in that file. Am I missing something?

[gaearon]

Connect is passing all props down. So yes, if the consumer uses forwardRef it will work.

The proposal is that connect itself should use forwardRef internally. Then this would work:

class Something extends Component {
  focus() { ... }
}

const Connected = connect(Something)(mapStateToProps)

// Later
<Connected ref={inst => inst && inst.focus()} />

forwardRef() was added primarily for HOCs (which connect is) to be used inside of them.

If this change was made then the connect() wrapper itself would be unobservable. And getWrappedInstance() would need to be removed.

Otherwise, this test would fail

I'm not sure why it's related. This test doesn't assert anything about whether this.c is the connect wrapper or the inner component. Moreover it is impossible to "redirect" the ref prop without forwardRef so I don't see what you mean by it working already.

[markerikson]

Seems to me like we're starting to pile up a pretty big list of potential breaking changes here, between this and all the other 16.x-related aspects. And React.forwardRef() is 16.3-only.

[timdorr]

Sorry, I'm understanding the issue now: The ref prop doesn't show up on this.props without forwardRef. I wasn't making that connection.

We can definitely look at this with regards to going 16+ only. Is there any sort of polyfill available?

[ovidiuch]

The proposal is that connect itself should use forwardRef internally.

Yes, this is what I was referring to. I'm willing to take a stab at implementing this if you're interested in merging it in.

With regards to the breaking change: Maybe it can be optional? Not sure where would options for connect go, but I can imagine a minority of users preferring to opt out of this new behavior, even with 16.3+. It should be discouraged going forward, but maybe some code out there depends on reading (private) stuff from the connect instance.

[gaearon]

The ref prop doesn't show up on this.props without forwardRef. I wasn't making that connection.

It won't show up on this.props even with forwardRef btw. It is passed as a second argument to forwardRef callback.

forwardRef((props, ref) => ...)

[gaearon]

With regards to the breaking change: Maybe it can be optional?

I don't think this makes a lot of sense. There's already a lot of surface area. I think it should be the default behavior (but in the next major).

[ovidiuch]

I don't think this makes a lot of sense. There's already a lot of surface area. I think it should be the default behavior (but in the next major).

Fair enough.

Maybe a silly approach, but can we check if React.forwardRef exists and if not fall back to existing behavior? It's still a breaking change, no doubt. But this retains backward compatibility and the next major version of react-redux doesn't need to be restricted to React 16.3+.

Edit: Then again React is really easy to upgrade, so maybe it's not worth the effort.

[timdorr]

Started a poll to check into this stuff: https://twitter.com/timdorr/status/988448040750067715

Also, found a polyfill, but it looks pretty young: https://github.com/soupaJ/create-react-ref

[gaearon]

This is not a full polyfill (https://github.com/soupaJ/create-react-ref#caveats).
We shouldn't be using this in a library.

[bvaughn]

Also, found a polyfill, but it looks pretty young: https://github.com/soupaJ/create-react-ref

I would recommend against using this polyfill for this reason:

Caveats

The polyfilled forwardRef is only compatible with refs created from createRef and not compatible with ref callbacks/functions. If you attach a ref callback to a component returned from the polyfilled forwardRef, you will get a RefForwarder component instance. This is one instance of how this library differs from React's implementation.

The forwardRef method would be nice to polyfill, except that it can't be done without introducing a non-standard API. Anyone that used the polyfill would have to deviate from the official API, which would probably cause confusion in the larger community and be more trouble than it's worth.

[timdorr]

Good news, according to my poll, is 90%+ of folks either are at 16.3 or are able to upgrade to it.

[markerikson]

Was literally just about to say that :) Granted, it's a pair of Twitter polls and with only 70-ish results per poll atm, but it's a good indicator.

I personally would say we leave 5.x as the "<= 16.2" line, and focus 6.x as the "compat with 16.3" line. (And then we're probably looking at 7.x as the "rewrite to something something async React rendering" line.)

[jedwards1211]

This just burned me when react-jss started using React.forwardRef: https://github.com/cssinjs/react-jss/issues/255
😢
The fact that forwarding a ref to the wrapped component in an HoC is difficult and this attempted fix is just causing more problems tells me that we really shouldn't be using HoCs any more. Render props components are far less prone to all this annoying awkwardness.

[gaearon]

Looks like this is actively impacting users, and #914 (comment) would be an easy backwards-compatible fix. Am I missing something?

[stevemao]

This breaks withTheme of styled-components v4 as they are using forwardRef:

https://github.com/styled-components/styled-components/blob/2b87a5f7e307b16e1b18dc31467d1f04cfe4738a/src/hoc/withTheme.js#L9

[markerikson]

We were originally planning to address this as part of the larger React-Redux v6 work, as seen in #995 and #1000 . However, given that this breaks with React 16.6's React.memo(), I'm inclined to say we should put out a 5.0.8 / 5.1.0 release that only switches to using react-is instead of the current check.

I don't have time to tackle this one myself right now, but if someone can file a PR, @timdorr and I can figure out the release strategy.

[timdorr]

I'll look into this today at some point.

[timdorr]

#1062 just landed and closes this out. I'll look at packaging it up for release (namely, if react-is still bloats things a lot). Hopefully today, but I an just a mere mortal.

[timdorr]

5.1.0 is out! https://github.com/reduxjs/react-redux/releases/tag/v5.1.0

[JLarky]

I was confused on what that exactly means, so I can spare you the research: use { forwardRef: true } as option to connect

[markerikson]

More specifically, do that only in 6.0+.

[markerikson]

@ywen : yes, I think you are misunderstanding. The entire point of withRef / forwardRef has been to let you get a reference to the actual instance of your own component that has been wrapped with connect(). You shouldn't be using forwardRef to try to access any DOM nodes here.

[ywen]

By setting { forwardRef: true } sets the ref to wrapped component. But what we really need is whatever returned by useImperativeHandle in the wrapped component. The only reason we want to use forwardRef is to be able to access the doms in a child component. To return the child component seems to defeat the purpose. Do I misunderstand anything here? Thanks

Never mind, upgrade to react-redux 6.0.0 (from 5.1.1) solved the issue. I am deleting the above comment

[maiya-22]

Actually, never mind this comment. I just realized that all I did was recreate .getElementById() ... and I know you're supposed to aviod that. Please feel free to delete this if you have a chance. Thanks.

Hi - I was a little afraid to comment b/c I might be out of my depth, but in order to get around the errors that I was getting with trying to pass refs to a connected component, I did this work around. Does anyone know if this is okay to do, or if it causes problems under the hood? My app is really simple, but has animations, and this format is working for me so far. Should I not do it this way?

In definition of the parent (class) component:
put the .createRef() on the state, and bind the setState function

MyClassComponent extends Component
       constructor(props) {
             super(props)
             this.state = {
                     myRef: React.createRef() 
             }
             this.setState = this.setState.bind(this) // binding so can pass as prop
      }

In the render of the parent (class) component:
pass the setState down to the component

<MyFunctionalComponent  setState={this.setState} />
 

In the child (functional) component
use the useEffect hook (with a second argument so it only calls one time), and put a helper function that finds the DOM element and sets it to the parent component's state

const MyFunctionalComponent = (props) => {
         const {setState} = props
         useEffect(() => {
                   //helper function:
                     findDomElement(window.document, setState)  
          }, [])

Helper function:
finds the dom element by searching down the DOM tree for an element with this component's id, and uses setState to store it to the parent component

function findDomElement(rootElement, setState) {
  //   console.log('findDomElement fired', rootElement)
  if (rootElement.id === 'my-component-id') {
    // if the element's id is that of your component, store the element to the state
    setState({carouselRef: rootElement})
    return null
  }
  let {children} = rootElement
  if (!children) return null
  for (let i = 0; i < children.length; i++) {
    let childNode = children[i]
    findDomElement(childNode, setState)
  }
}

[evannorton]

I created a helper function to be used in place of connect that may help people who find this thread. The first parameter is your component (not wrapped with forwardRef). The rest are just connect's parameters.

import { forwardRef } from "react";

const connectAndForwardRef (component, mapStateToProps = null, mapDispatchToProps = null, mergeProps = null, options = {}) => connect(mapStateToProps, mapDispatchToProps, mergeProps, {
    ...options,
    forwardRef: true
})(forwardRef(component));

[Akiyamka]

I slightly changed the solution proposed by @evannorton so that it could be drop-in replacement of connect.

import { forwardRef } from 'react';
import { connect } from 'react-redux';

const connectAndForwardRef = (
  mapStateToProps = null,
  mapDispatchToProps = null,
  mergeProps = null,
  options = {},
) => component => connect(
  mapStateToProps,
  mapDispatchToProps,
  mergeProps,
  {
    ...options,
    forwardRef: true,
  },
)(forwardRef(component));

const ConnectedMyComponent = connectAndForwardRef(mapStateToProps, mapDispatchToProps)(MyComponent)

[amitpatil321]

@JLarky : As JLarky suggested passing extra parameter did the trick

export default connect(mapStateToProps, mapDispatchToProps, null, { forwardRef: true })(MyContainer);