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.