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

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

Open
Tracked by #501
GiladShoham opened this issue Dec 1, 2020 · 2 comments
Open
Tracked by #501
Labels
Bug thing that needs fixing semver:major backwards-incompatible breaking changes
Milestone

Comments

@GiladShoham
Copy link

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
Copy link
Author

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

@lukekarrys lukekarrys added the Bug thing that needs fixing label Feb 26, 2022
@lukekarrys lukekarrys added the semver:major backwards-incompatible breaking changes label Oct 27, 2022
@darcyclarke darcyclarke added this to the v8 milestone Oct 31, 2022
@darcyclarke darcyclarke mentioned this issue Oct 31, 2022
14 tasks
@mbtools
Copy link
Contributor

mbtools commented Feb 1, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug thing that needs fixing semver:major backwards-incompatible breaking changes
Projects
None yet
Development

No branches or pull requests

4 participants