node-semver icon indicating copy to clipboard operation
node-semver copied to clipboard

[BUG] dangerous check if semver is a valid semver in constructor

Open GiladShoham opened this issue 4 years ago • 2 comments

What / Why

in the semver class constructor, you are checking if the value is an instance of SemVer This check is dangerous since many times a package manager can create a tree of packages where you have 2 instances of SemVer in different places in the filesystem. This leads to a bug where you pass a semver from one package to another then call the constructor and get an error about Invalid Version

How

Current Behavior

An error when you pass a SemVer instance between packages and call the SemVer constructor.

Steps to Reproduce

create 2 packages with different locked semver package version (for example 7.3.1 and 7.3.2) (This sometimes happen also when the versions are the same, but it's easier to reproduce with different versions) pass a SemVer instance from one of them to the other and call the SemVer constructor with this version.

Expected Behavior

Test if the value is valid SemVer by it's content (or content + constructor name) instead of using instanceof

References

This happens to me when using yarn berry API even though the core and the npm-plugin uses the same versions of semver. When installing my tool as global with npm it creates 2 instances of the SemVer because it hoisted another version from another package.

GiladShoham avatar Dec 01 '20 09:12 GiladShoham

A concrete use case with a reproduce repo is described here https://github.com/yarnpkg/berry/issues/2224 the reproduce repo is here - https://github.com/GiladShoham/yarn-invalid-version-bug

GiladShoham avatar Dec 09 '20 11:12 GiladShoham

Old issue but imho, getting an error is essential here.

The implementation in another release might (probably will) have different behaviour. It would be a big assumption that you can pass an instance of one semver release to another and expect consistent results (there are no tests for such cross-release cases either).

You could stringify the version first and then create the instance in the other package.

mbtools avatar Feb 01 '24 09:02 mbtools