React: `typeof React.forwardRef(...)` is not a 'function', breaking third-party libs

[jamesreggio]

Do you want to request a feature or report a bug?

Bug, possibly

What is the current behavior?

The new React.forwardRef feature returns an object, not a function.

To this point, the typeof both stateless components and class components was 'function'.

React does not expose a utility to check whether a given value is valid to be passed to React.createElement. As a result, third-party library authors who wish to pre-validate a component type will commonly use typeof Component === 'function' to test whether the type is valid. The return value of forwardRef, despite being a valid component type, will fail this naive test. Library authors do this in order to provide better errors than would otherwise be provided by React if an invalid component type was passed in.

In a perfect world, the fault would fall squarely on the third-party library developers for using an imprecise check, but those developers could rightfully point out that React has given them little ability to be precise in these pre-emptive checks.

~This is one of those "don't break the internet" bugs.~ This is a bug where third-party developers have taken reasonable-yet-illegal dependencies on React internals in order to provide a better experience for their users. If it's not too difficult, it may be a good idea to chane forwardRef to return a 'function', instead of an object.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16.3.0-alpha.3

[jamesreggio]

cc: @bvaughn, who implemented the forwardRef API in #12346.

The forwardRef API is described in this RFC: https://github.com/reactjs/rfcs/pull/30

Finally, I encountered this issue when using React 16.3 with Next.js 5. See https://github.com/zeit/next.js/issues/4055.

[gaearon]

React does not expose a utility to check whether a given value is valid to be passed to React.createElement

I don’t think we’re opposed to exposing a utility like this. We’re already releasing a package called react-is with some brand checking utilities along with 16.3, and I think we could add something like isValidElementType to it.

it may be a good idea to change forwardRef to return a 'function', instead of an object.

Then we’d have to look for a “special” field on every single function/class pass to React. I don’t think we’d want to go with that strategy since it introduces an extra cost for the majority of cases.

Ultimately I don’t think this problem is a blocker. It’s unfortunate, but React widening supported types is not a semver major change. If as a library author you choose to depend on the fact that React only supports defining components as classes or functions (and not, e.g., with objects), you are kind of committing to closely tracking what exactly it is that React supports. Of course a better solution would be to have a way to check it that’s maintained by us—and as I said, I think we’re open to providing that.

[gaearon]

This is one of those "don't break the internet" bugs.

As an aside, I’d prefer if we reserved this terminology for its original meaning: “ship something that breaks existing websites without any direct action on their behalf”. For example, Chrome’s “interventions”, Array.prototype.contains, etc.

Using this phrase to refer to a change where you opt in (by bumping the version of React, and later bumping some library to a new major—after all, it is now 16.3+—relying on forwardRef in public API) doesn’t seem entirely fair to me. It makes “break the internet” sound cheaper and more commonplace than it is.

[bvaughn]

We will be releasing the package Dan mentioned (react-is) along with 16.3. It exposes a function called isForwardRef, which could be used here. Although this function expects React elements (the return type of React.createElement) rather than a naked Component type, as the next.js validation check seems to be expecting.

Put another way:

const { isForwardRef } = require("react-is");
isForwardRef(React.createElement(Component)); // true
isForwardRef(<Component />); // true
isForwardRef(Component); // false

[jamesreggio]

We’re already releasing a package called react-is with some brand checking utilities along with 16.3, and I think we could add something like isValidElementType to it.

That sounds like the perfect solution. I wasn't aware of the creation of react-is, but it's a great idea!

As an aside, I’d prefer if we reserved this terminology for its original meaning.

Sure thing — sorry for the hyperbole. What I meant to express is that React is fully within its rights to add the forwardRef behavior in a minor change, but it may cause consternation amongst those who have taken 'illegal', yet reasonable, dependencies upon React internals.

---

Personally, I don't have a horse in this race. I just wanted to raise a problem I witnessed in the wild when using a new API whose design I considered during the RFC process, and suggest a solution or two.

[jamesreggio]

@bvaughn

Although this function expects React elements (the return type of React.createElement) rather than a naked Component type, as the next.js validation check seems to be expecting.

Checking the type _after_ element instantiation seems like a fairly awkward developer experience. It's unintuitive to me because I would expect React to throw in the createElement() call if it received an invalid element type (even though, as of today, it appears that that check is deferred until the renderer begins mounting the element).

I realize that HOCs are going out of style right now, but there's compelling value to being able to check at HOC instantiation time that the component you're wrapping in the HOC is a valid component type. In fact, you'll see these checks for typeof Component === 'function' in some very popular libraries today, including Redux, React Native, and styled-components.

It's fine if you don't want to change how forwardRef is implemented, but I think it would be a good idea to have an isValidElementType in react-is before these breakages start rolling in.

[gaearon]

It's unintuitive to me because I would expect React to throw in the createElement() call if it received an invalid element type (even though, as of today, it appears that that check is deferred until the renderer begins mounting the element).

It intentionally doesn’t throw so that we can easily inline this function or replace it with a compiler output.

there's compelling value to being able to check at HOC instantiation time that the component you're wrapping in the HOC is a valid component type

I agree this is useful to HOCs.

[bvaughn]

Fair enough. I was just pointing out a difference between what Next is doing and how react-is works.

[jamesreggio]

Cool — I'll open a PR to add a element type validity check to react-is.

[jamesreggio]

I've opened a PR here: #12483

[bvaughn]

This new method has been added to react-is and will go out with the upcoming release. Thanks!

[jedwards1211]

@gaearon

Ultimately I don’t think this problem is a blocker. It’s unfortunate, but React widening supported types is not a semver major change.

I don't like this reasoning; you have the freedom to release any change with a major version bump, for any reason whatsoever, technical breaking changes or not. Only breaking changes released as a patch or minor change are against the spirit of semver. If you think there's a lot of risk that related packages using React will break, even if none of the existing narrower type support in React was broken, I think it's perfectly reasonable (and conscientious) to release it with a major version bump.

[gaearon]

@jedwards1211

I think you're getting a little bit confused. There was no breaking change on the React side.

Technically, the breaking change here is for component package authors to wrap their exported components in forwardRef. It's still debatable whether the type of the export (as long as it's renderable by React) is truly a breaking change, but it is in those packages, not in React itself.

But then depending on a new method that doesn't exist in previous React 16.x minor versions is a breaking change by itself. So we imagine that anybody who adds forwardRef to the topmost component already makes a breaking change in their package.

[jedwards1211]

@gaearon I know there was no breaking change on the React side. What I'm saying is that doesn't prohibit you from releasing a new major version.

[gaearon]

What's the point of releasing a new major version for React without a breaking change? How would that help the ecosystem? It wouldn't solve the problem you're experiencing, and would only add churn for everyone.

[gaearon]

Let's imagine forwardRef came out in React 17 instead. If my-component-library started using it in its own minor version, it would still break your app. So the problem here isn't in React versioning, it's in my-component-library using forwardRef with only a minor release bump in its own versioning.

[jedwards1211]

If my-component-library depends on a new major version of React, shouldn't my-component-library receive a major version bump as well?

[jedwards1211]

In any case, I have an idea for a fix:

• if component passed to forwardRef is a function, wrap it in a function that just forwards the arguments, attach the magic symbol to the wrapper (and perhaps add the wrapped function as a prop too so it can be extracted by React), and return it.
• if component passed to forwardRef is a React.Component class, create a new class that extends it, attach the magic symbol to the new class' constructor, and return it

In this way the return value of forwardRef should work with all existing HoC libraries that expect a React.Component class or function.

This could be released as patch, and AFAIK it would solve everyone's problems.

[gaearon]

If my-component-library depends on a new major version of React, shouldn't my-component-library receive a major version bump as well?

Sure, but you can make the same argument about my-component-library starting to depend on a new feature that was introduced in a minor version of React, results in an observably different behavior, and didn't exist before. Which is exactly my point: relying on a new feature is a breaking change whereas adding a new feature is not.

I don't understand what you're suggesting. forwardRef() doesn't accept a component at all (it accepts a rendering function). There are good reasons why the returned type is not a function (see last paragraph of https://github.com/reduxjs/react-redux/issues/914#issuecomment-396410016). We don't plan to cut a patch to change this.

Again, a library adopting forwardRef should constitute a major bump for that library. Even if React did something magical to ensure forwardRef return value is a function it still has a different observable behavior (namely, it changes what a ref points to, and depends on a method that does not exist in older versions), so a library would have to bump its major version anyway.

[jedwards1211]

relying on a new feature is a breaking change whereas adding a new feature is not

Yeah, I totally agree. Here's the rub: the new code in react-jss checks if React.forwardRef is defined, and if so they use it. By doing this they thought they wouldn't need to release it as a breaking change...

[bvaughn]

I think you may both be talking about different aspects of this.

Again, a library adopting forwardRef should constitute a major bump for that library. Even if React did something magical to ensure forwardRef return value is a function it still has a different observable behavior (namely, it changes what a ref points to, and depends on a method that dpes not exist in older versions), so a library would have to bump its version anyway.

I believe Dan is saying that if my-component-library uses React.forwardRef() - that's a breaking change (regardless of how React implements forwardRef) - because any applications using my-component-library with an older version of React (e.g. < 16.3) would break.

But I believe Andy might be referring to third party library that _accepts_ a React component. It may have a hard-coded assumption that a React component will always be a function (class or functional component), where as newer versions of React have more exotic component types (e.g. fragments, modes, forward refs, etc).

This sort of hard-coded assumption has caused problems in a handful of places that I've seen since the 16.3 release.

[gaearon]

Here's the rub: the new code in react-jss checks if React.forwardRef is defined, and if so they use it.

That seems pretty fragile. I wouldn't recommend anyone to do that because it leads to subtly different behavior between versions.

[jedwards1211]

@gaearon Yup... I think you have a good point about the observable behavior in particular; merely using a new feature of a dependency is not necessarily a breaking change. For instance if a library starts using a new feature in lodash, but its observable behavior remains the same, it can be released as a patch or minor change.

[sanfilippopablo]

There's another use case that's broken with this. I know inheriting from Component is discouraged by the React team, but the react-leaflet project uses it extensively (pun not intended) and quite elegantly if you ask me, but that's not the point. The point is that inheriting a component was a possible thing back then, but now you can't extend a component that uses forwardRef. This is the check babel is making but it's failing
Issue in react-leaflet: https://github.com/PaulLeCam/react-leaflet/issues/488

[noahgrant]

Sorry to post here if this is not totally relevant, but another change this causes is the inability to use scryRenderedComponentsWithType and similar methods that search the tree for composite components. What would you recommend instead?

[gaearon]

@sanfilippopablo I don’t see why this issue is specific to forwardRef. We’ve always been very explicit about inheritance not being the best method for code reuse in React. It already doesn’t work with functional components. You can think of forwardRef as something being close in spirit to a functional component, and in that sense it makes sense that it doesn’t work.

another change this causes is the inability to use scryRenderedComponentsWithType and similar methods that search the tree for composite components

Mind filing a separate issue so I can take a look? That sounds accidental to me.