facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

Allow react-is ^17 as dependency #354

Open alippai opened 3 years ago

alippai commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

^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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

This would be a hell of a dependency.

ashidaharo commented 2 years ago

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

ljharb commented 2 years ago

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 commented 2 years ago

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

ashidaharo commented 2 years ago

... 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 commented 2 years ago

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

ljharb commented 2 years ago

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

ashidaharo commented 2 years ago

@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 commented 2 years ago

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

ashidaharo commented 2 years ago

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.

https://github.com/facebook/react/commit/9102719baacb64738e9235bc80d3d7d9918cc74c#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 commented 2 years ago

...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 commented 2 years ago

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. https://github.com/facebook/react/commit/e2c6702fcaaee35ca08957be7645820192d0daf3#diff-f0b61e170bdf20fa2c4810fb5f1bd6ddc849132d8051f1105a6a97e29cd0633e https://github.com/facebook/react/commit/b61174fb7b09580c1ec2a8f55e73204b706d2935#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 commented 2 years ago

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

alippai commented 2 years ago

@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 commented 2 years ago

@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 commented 2 years ago

It is unfortunate this package does not get updated with a major version to change the dependency to a newer version of React (17+). A project I am working on is having issues with multiple versions of React being used due to this package having become outdated. For my case, it was decided to no longer use prop-types.

ljharb commented 2 years ago

@nbkhope this package in no way causes problems with multiple versions of react. it depends on the package react-is at v16, which is entirely distinct from react 17, and duplicating it in the tree is perfectly fine.

vtaits commented 1 year ago

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