Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Move react-is to react/is #20099

Closed
eps1lon opened this issue Oct 26, 2020 · 5 comments
Closed

feature: Move react-is to react/is #20099

eps1lon opened this issue Oct 26, 2020 · 5 comments

Comments

@eps1lon
Copy link
Collaborator

eps1lon commented Oct 26, 2020

Currently react and react-is are built using a shared/ package. That means that react-is (just like react-dom) has to have the same version number as react to avoid potential issues due to different code from shared/.

However, right now a lot of libraries are wrongly declaring react-is as a direct dependency1 while declaring react as a peer. Technically correct would be declaring react-is as a peer as well leading to more burden for app developers. This is slightly different to having react as a peer dependency because every app developer also needs react as a dependency. The same doesn't always hold true for react-is.

Since react-is has no dependencies it would be more ergonomically for libraries if react-is would be available from react/is since they already have react as a peer dependency. There are two possibilities for this:

  1. Move react-is to react/is
  2. Make react-is a dependency of react and re-export it from react/is

I guess marking react-is as a peer would be the approach that would be more consistent. It's really just annoying for libraries but if that's what you would recommend then I'm fine with it as well.

As far as I can tell this is really just a technicality right now i.e. I haven't seen any actual issues. I just stumbled over it while working on React 17 compat. So far it looks like we're in a position to declare 16.8 and 17 as the range for react in our peerDependencies which leaves us with a slightly awkward "react-is": "^16.8.0 || ^17.0.0" in the dependencies (I might be trying too hard to avoid additional peer deps).

1 Popular examples include prop-types, react-redux, react-router, @material-ui/core, next

@eps1lon eps1lon added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Oct 26, 2020
@bvaughn
Copy link
Contributor

bvaughn commented Oct 26, 2020

🤡

"dependencies": {
"object-assign": "^4.1.1",
"react-is": "^17.0.1",

@bvaughn bvaughn added Type: Discussion and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Oct 26, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 26, 2020

Now that I think about it I think re-exporting react-is from react is only technically correct if pinned to patch versions and if the version always matches. If pinned to major (caret range) the following scenario creates a mismatch of react-is and react:

  1. React 16.12 is released
  2. Install react: yarn add react-test-renderer@16.12.0 (installs react-is@16.12.0)
  3. React 16.13 is released
  4. Upgrade react: yarn add react@16.13.0 (keeps react-is@16.12.0 due to lockfiles)

@billyjanitsch
Copy link
Contributor

@eps1lon see #20063 (comment).

@gaearon
Copy link
Collaborator

gaearon commented Mar 24, 2021

We're probably going to recommend not using that package altogether. It doesn't work great if you consider different environments (e.g. Server Components). Given that it's not recommended, we won't make it a part of the core package.

@Andarist
Copy link
Contributor

We're probably going to recommend not using that package altogether

If react-is is not recommended then what is the recommended approach when having to test for element types etc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants