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

Allow react-is ^17 as dependency #354

Open
alippai opened this issue Jul 12, 2021 · 22 comments
Open

Allow react-is ^17 as dependency #354

alippai opened this issue Jul 12, 2021 · 22 comments

Comments

@alippai
Copy link

alippai commented Jul 12, 2021

Now it depends on react-is ^16 which may introduce extra library for react 17 projects.
https://github.com/facebook/prop-types/blob/master/package.json#L31

@ljharb
Copy link
Collaborator

ljharb commented Jul 13, 2021

The problem there is that might break React 16 users - there's no way to allow v17 without auto-installing it, except by converting it to a peer dep, which would be a breaking change.

@alippai
Copy link
Author

alippai commented Jul 13, 2021

^16|^17 in the worst case auto installs v17, which is still better than auto installing v16. I'm not sure this is true, what do you think?

@ljharb
Copy link
Collaborator

ljharb commented Jul 13, 2021

In every case it will install 17, except when 16 is already present.

Using react 16 with react-is 17 is the worst possible outcome.

@ashidaharo
Copy link

ashidaharo commented Oct 26, 2021

It seems to me that there is one solution for now that correctly specifies the dependencies for this.
That is OptionalPeerDependencies(PeerDependencies)(Optional)
This feature allows package A to be associated with a specific version of package B, and returns an error if it tries to use package A at the same time as an unspecified version of package B is included in the dependency tree... I think. I think... if I remember correctly. If I'm not mistaken.

By bumping up the major version and specifying react ^17 as an OptionalPeerDependencies (and preferably react-is ^17 as a PeerDependencies as well), we can prevent people trying to use prop-types by specifying it as a dependency (and preferably also react-is^17 in the PeerDependencies), we can prevent people trying to use prop-types from inadvertently upgrading their version while using react ^16... should be.
If we want to continue to support react ^16, we will need to separate branches for each major version and pull the necessary bug fixes there as needed. There is currently no dependency solution to switch react-is according to the installed version of react.

... As a matter of fact, we don't know if this is really something that npm or yarn can solve correctly. Also, even if it is resolved correctly, this will confuse people who casually try to npm i prop-types@latest.
However, react has undergone a major version upgrade and breaking changes. If so, then all packages that depend on react should preferably work that way, as opposed to react-is unknowingly mixing multiple versions , or react and react-is versions not meshing.

My conclusion is that it is preferable to use PeerDependencies, because the versions of react and react-is should be the same when resolving dependencies, and there should be one of each in the whole dependency tree.
(By the way, the good old package hoist-non-react-statics still doesn't seem to support react-is ^17. Based on this theory, any package that depends on it should not be able to use it with react ^17... Funny enough, react-redux and emotion are included)

@ashidaharo
Copy link

This would be a hell of a dependency.

@ashidaharo
Copy link

...Why isn't one of react and react-is in the other's peerdependencies in the first place?

@ljharb
Copy link
Collaborator

ljharb commented Oct 26, 2021

They should have been (in react-is). But since they’re not, it’s too late, there’s not much to be done here. Optional peer deps would be a breaking change, because older npm versions don’t understand them.

@alippai
Copy link
Author

alippai commented Oct 26, 2021

Based on your answer I assume no new major version can be created. What's the reason for that?

@ashidaharo
Copy link

... History tells us that many cases similar to this have been solved by the passage of time and fashion.
Well, that is, by creating another, better solution while the old solution is out of control, tied up in history and backward compatibility.

@ashidaharo
Copy link

Well or? What if the old npm support is about to expire and the OptionalPeerDependencies become reliable?

@ljharb
Copy link
Collaborator

ljharb commented Oct 26, 2021

@alippai new major versions of any package should be avoided unless there's a very good reason for a breaking change.

@ashidaharo
Copy link

@alippai I agree with you that a major version release of react should be reason enough to release a major version of this package, which is downstream from react...
Well, React has an ecosystem around Typescript and Flow these days... Also, this package has a lot of commits, but no minor releases or patch releases since 2019, so you can imagine the level of attention from Facebook...

@alippai
Copy link
Author

alippai commented Oct 26, 2021

I don't see any issues with the reasoning above, I was just curious ¯_(ツ)_/¯

@ashidaharo
Copy link

However... as a matter of practice, it is extremely unpleasant to have mixed versions of react-is, so I have been investigating it.

If it is confirmed that the implementation of this package's dependency on react-is has not changed destructively since the current dependency v16.13.1, then assuming React is released as defined by SemVer, this package's dependency on react-is ^17 should not affect the behavior of this package, even if later versions are inadvertently used downstream with react ^16.
...it should still be a major release, but it is enough of a guarantee that breaking changes to this package will not have a negative impact downstream.

Regarding the results of the survey.... NO, not completely, ... However, it is stable except for the experimental APIs.

facebook/react@9102719#diff-f0b61e170bdf20fa2c4810fb5f1bd6ddc849132d8051f1105a6a97e29cd0633e
The function isValidElementType has been destructively changed in the file "packages/shared/isValidElementType.js" in this commit.
Apparently, React is working on something called ReactScope API (I couldn't find any documentation on how it works due to my lack of ability), but in React ^17, it seems that the functionality has been changed to be controlled by flags.
And I found no other changes in the functionality that this package depends on in react-is.

My argument is that if prop-types makes a major release relying on react-is ^17, it should not affect the behavior of anyone except the users who used this experimental feature called ReactScope API.
Therefore, is it okay for prop-types to make a major release?

@ashidaharo
Copy link

...However, since I checked it visually, there may be something I overlooked that I didn't notice... It would be a very unreliable basis for judgment...

@ashidaharo
Copy link

Oh... While I was talking about it, I was reviewing it and found that I really missed something...
It seems that two other experimental APIs were also deprecated at the same time.
facebook/react@e2c6702#diff-f0b61e170bdf20fa2c4810fb5f1bd6ddc849132d8051f1105a6a97e29cd0633e
facebook/react@b61174f#diff-f0b61e170bdf20fa2c4810fb5f1bd6ddc849132d8051f1105a6a97e29cd0633e

All three of these are experimental APIs, so I'm not going to change my assertion that the major release of prop-types will not affect many users operationally.
(However... I was surprised that ConcurrentMode was not released after all, despite all the noise when it was announced. I think this feature attracted a lot of attention and many users were touching it, whether they were using it or not.)

@ashidaharo
Copy link

I forgot to tell you guys the answer, which I received when I threw an issue on react, because the answer was so shocking.
facebook/react#22640 (comment)
facebook/react#20099 (comment)
The react developers are telling us that we shouldn't use react-is. What the hell are we supposed to do?

@alippai
Copy link
Author

alippai commented Apr 2, 2022

@ljharb does almost 5 years passing by since the release of react v16 and the recent release of react v18 warrant a new major version of prop-types? Maybe the EoL of Node.js 12.x is an interesting event in April as well.

@ljharb
Copy link
Collaborator

ljharb commented Apr 2, 2022

@alippai i'm not sure why either would have an impact. node going EOL in no way means a package must, or even should, drop support for it. I maintain hundreds of packages that support down to node 0.6, for example, and have no intention of ever dropping that support.

@nbkhope

This comment was marked as off-topic.

@ljharb

This comment was marked as resolved.

@vtaits
Copy link

vtaits commented Mar 22, 2023

Any progress here? There is 18.x version of react-is.

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

No branches or pull requests

5 participants