React-redux: 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 https://github.com/facebook/react/issues/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} />
);

[skidding]

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?

[skidding]

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

[skidding]

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 https://github.com/reduxjs/react-redux/issues/914#issuecomment-377406014 would be an easy backwards-compatible fix. Am I missing something?

[jedwards1211]

@gaearon I had no awareness of this forwardRef stuff until my app started catastrophically failing today, but did it have to be designed in such a way that it returns an object with a magic symbol and a render function, instead of attaching the magic symbol to a FSC function or to a React.Component instance? If the latter is possible it would be more backwards compatible.

[gaearon]

I had no awareness of this forwardRef stuff until my app started catastrophically failing today

Your app broke because you updated a component library (not React!) to a version that includes a breaking change (using forwardRef). Note that React adding forwardRef is not a breaking change; a component library using forwardRef is.

I don't know whether that component library used a major or a minor release for this, but if you don't want your app to break on random dependency updates and people's mistakes, I encourage you to use a lockfile. Both npm and Yarn provide lockfiles by default. If you like "living on the edge" and having unreproducible builds, it's not surprising your app breaks every now and then.

Again, I want to reiterate there was no breaking change on React's part. I'm sorry that this new feature caused confusion and churn for you, but I don't see why it would be a problem on the React side.

Yes, the design you mentioned was considered. It was rejected because it would incur a performance penalty as we'd have to check every single component for a special field (instead of immediately recognizing "special" components through a different type).

[jedwards1211]

@gaearon okay, so it was react-jss's mistake then, not releasing it as a breaking change.

[gaearon]

That is my understanding.

[jedwards1211]

However I would like to complain that the docs for https://reactjs.org/docs/forwarding-refs.html nowhere state that if a library starts using it, it should be released as a breaking change, and if they had this might not have occurred.

[gaearon]

I agree, would you like to send a PR? I think at some point I was planning to add it and then it slipped out of my mind. It was a pretty complex release and took weeks of preparation so we missed this.

[jedwards1211]

@gaearon okay, yes, will do right now

[jedwards1211]

@gaearon I was just thinking, if React provided a React.isValidComponent method, then react-redux could use that for checking the input component to connect. Has anyone proposed such a function? I'm sort of surprised it doesn't already exist.

[cyan33]

What's the current status of this issue now? Does the proposal of using ReactIs.isValidElementType to check WrappedComponent sounds like the correct way to fix this?

[jedwards1211]

Just an aside...isValidElementType is an awkward naming choice. My last comment was redundant because I didn't realize it's really checking for a valid component. Up until now at least, both React.Component subtypes and functional stateless components are "components". Was it decided that the return value of React.forwardRef isn't properly considered a "component", but it's still an "element type"? I hate when names get messy like this.

[cyan33]

@jedwards1211

It includes all of the possible correct type of React components as per the document:

ReactIs.isValidElementType("div"); // true
ReactIs.isValidElementType(ClassComponent); // true
ReactIs.isValidElementType(StatelessComponent); // true
ReactIs.isValidElementType(ForwardRefComponent); // true
ReactIs.isValidElementType(Context.Provider); // true
ReactIs.isValidElementType(Context.Consumer); // true
ReactIs.isValidElementType(React.createFactory("div")); // true

[gaearon]

I think isValidComponent might've been more clearer, but then people would use it like isValidComponent(<Foo />) and expect it to return true (because they don't know the difference between elements and components).

This is supposed to be a very rarely used API. Most developers don't even know react-is exists. So I think verbosity and precision is fine there.

[pgarciacamou]

@ everyone, I added a PR for this, please review.

[ematipico]

Is this feature inside some roadmap? We are removing connect components because they are not able to pass the ref, which is really needed.

[markerikson]

Yes. This would be a breaking change, because we'll be removing the withRef option at that point, so it's targeted for 6.0. Please see the roadmap in #950 for our plans.

[sibelius]

this is breaking Relay >1.6.1 https://github.com/facebook/relay/issues/2499

is there any workaround for this now?

[AlicanC]

This isn't breaking Relay. As mentioned in the official documentation, Relay should have treated moving to forwardRef as a breaking change and bumped major.

If the Relay team doesn't want to follow official React documentation and/or SemVer, it's their choice.

[keshavkaul]

Hey guys, I have tried re-creating connect() API for learning purposes using easy to understand HOC component and which is compatible with forwardRef. I wanted feedback on possible functional differences between react-redux connect and my own.

Here is the github gist for the code -
https://gist.github.com/keshavkaul/9bb67499029d63bc6745c8c23aee86f2#file-connect-jsx

Do let me know all your thoughts, thanks!

[jedwards1211]

@AlicanC Dan Abramov and I made that change to the official documentation recently as a result of issues like this, and the Relay team likely made that release before our documentation changes.

[AlicanC]

@jedwards1211 I was aware of that when I wrote the post.

Relay team have not done anything wrong by moving to forwardRef, because it wasn't known to be breaking at that moment. Relay team will be doing something wrong if they do not act on the fact that forwardRef is now being considered breaking. The fix is as simple as reverting this breaking forwardRef change in Relay and making a new patch release.

If Facebook or the community needs forwardRef, then Relay team can also release a new major with forwardRef and maintain two majors until there is enough support for forwardRefs in the ecosystem and then dump the old major.

So from my point of view, saying "Redux is breaking Relay" is wrong. Redux is not breaking Relay, Relay is breaking our Redux apps.

I don't even know if our app will work with Relay 1.6.1 if Redux starts supporting forwardRef. We have tons of dependencies and more than Redux might not support forwardRef. It looks like the community doesn't see this as a problem in Relay which means we will keep having problems with it being incompatible with our other dependencies which means we will have to delay moving to React Native 0.56 further which means our application and developers will keep not having the features they need for a longer time.

Or Relay team could just revert the forwardRef change so we can move on. (Please?)

[jedwards1211]

@alicanc gotcha, yeah, that doesn't sound like a fun situation.

[sibelius]

The React-Redux fix would be just remove the invariant?

[jamesreggio]

For those of you just joining —

A fix for this problem (and many other concerns related to React 16.4) is in progress in #1000.

Here's to hoping it lands soon. Many thanks to @cellog for taking this on.

[jamesreggio]

Also, I've published a script that will patch _just the invariant failure_, so that connect works on components wrapped by React.forwardRef.

https://gist.github.com/jamesreggio/caea042d7a9cd87065a187d5f1f97c62

Read the Usage section within the gist, and be sure to note that this patch requires that you install react-is as a package dependency.

[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
```

**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 { connect } from "react-redux";
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)