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] 7.3 breaks lower bound calculation for v0 versions #323

Closed
dstaley opened this issue Apr 16, 2020 · 5 comments
Closed

[BUG] 7.3 breaks lower bound calculation for v0 versions #323

dstaley opened this issue Apr 16, 2020 · 5 comments

Comments

@dstaley
Copy link

dstaley commented Apr 16, 2020

The new v7.3 releases modify the lower bound calculation for version identifiers beginning with v0. Previously, the v7.2 release would report 0.0.0 as the lower bound. v7.3 now returns Symbol('SemVer ANY') in place of the expected SemVer result object.

This behavior seems like it was intentional, but I do admit it's a bit ironic to see such a breaking change in a minor version of the package that's literally about semantic versioning :)

If this change is intentional, and there won't be a new minor/patch version to correct the behavior, please let me know so that I can send in a PR to a library that's experiencing a crash due to this change.

$ npm i semver@7.2.3

+ semver@7.2.3
added 1 package and audited 1 package in 0.49s
found 0 vulnerabilities

$ node
> const semver = require("semver");
undefined
> const r = new semver.Range("v0.x.x")
undefined
> r.set[0][0].semver.version
'0.0.0'


$ npm i semver@latest

+ semver@7.3.2
updated 1 package and audited 1 package in 0.524s
found 0 vulnerabilities

$ node
> const semver = require("semver");
undefined
> const r = new semver.Range("v0.x.x")
undefined
> r.set[0][0].semver.version
undefined
@isaacs
Copy link
Contributor

isaacs commented Apr 18, 2020

Ah, this is because it's converting >=0.0.0 to * (since they're identical). So, the version range 0.x gets converted to >=0.0.0 <1.0.0-0, and then the >=0.0.0 is ANY (ie, like * or '').

If I remember correctly, this fixed a bug that was preventing subset and intersects from working properly in some cases.

I think the best answer here is to just throw out any ANY comparators that are found in a simple range set. After all, * ${other comparators} is identical to just ${other comparators}, and only include ANY in the set list when it's the sole member of a simple range set.

That being said, relying on getting exactly two comparators in a simple range set is not reliable, and never was! That does not tell you the lower bound, and never could have, even for relatively simple ranges that are in use in production. It only worked by accident, and only in very restricted use cases. The specific comparators generated from a given range should be considered an internal implementation detail. That 0.x is functionally equivalent to >=0.0.0 <1.0.0-0 should not be taken as a guarantee that it will be exactly equal to that set of comparators.

@isaacs isaacs closed this as completed Apr 18, 2020
@isaacs isaacs reopened this Apr 18, 2020
@isaacs
Copy link
Contributor

isaacs commented Apr 21, 2020

Comparator sets cleaned up a bit further in #324

Since it's not (and has never been) guaranteed that the first comparator in a range set will reflect the lower bound, maybe it'd be worthwhile to add a method to give you the lower bound of a range? What are you using this for?

@isaacs isaacs closed this as completed Apr 21, 2020
@dstaley
Copy link
Author

dstaley commented Apr 21, 2020

I'm not the developer, but I believe the library is relying on the lower bound to see if the lower bound is greater than all of the possible versions for a given semver range. This is the relevant line of code.

Essentially they're taking a range, getting the lower bound, and then checking that against another range. The library was relying on the lower bound of the range v0.x.x being a valid version (in this case, 0.0.0).

@gantoine would you mind clarifying if this isn't accurate?

@isaacs
Copy link
Contributor

isaacs commented Apr 21, 2020

Yeah, so if you have a range like <1.2.3 >=1.0.0 or 1.x >=1.2.3 then that approach won't work at all, and never would have.

So that line in flow-typed is fundamentally broken. I'll add upper/lower bound methods and send a PR once that's landed.

@gantoine
Copy link

That code is well before my time, so your assessment of it is as good as mine. 🤷‍♂️ From the looks of it that chunk of code is indeed trying to compare the lower bound of a range to another range. Since 0.0.0 is now no longer valid, I suppose the comparison should/would always hold true, since all valid semver versions would be above that lower bound.

vgrichina added a commit to near/near-cli that referenced this issue Mar 3, 2021
This should be tested on create-near-app CI anyway
and currently was failing CLI because of semver bullshit npm/node-semver#323
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

3 participants