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

semver: deprecate rangeMax and rangeMin #4365

Closed
kt3k opened this issue Feb 21, 2024 · 5 comments · Fixed by #4561
Closed

semver: deprecate rangeMax and rangeMin #4365

kt3k opened this issue Feb 21, 2024 · 5 comments · Fixed by #4561
Labels
feedback welcome We want community's feedback on this issue or PR

Comments

@kt3k
Copy link
Member

kt3k commented Feb 21, 2024

rangeMax and rangeMin were introduced in #3385, but these 2 APIs seem having critical design flaws. I suggest we should deprecate these 2 APIs.

  1. In many cases, rangeMax returns invalid semvers.

rangeMax(parseRange("<1.0.0")) returns 0.∞.∞ (0.Infinity.Infinity).
rangeMax(parseRange("<1.1.0")) returns 1.0.∞ (1.0.Infinity).
rangeMax(parseRange("<0.0.0")) returns -∞.∞.∞ (-Infinity.Infinity.Infinity).

These values are non semvers. Exposing them are very confusing.

  1. rangeMax ignores prerelease

rangeMax(parseRange("<1.2.3-a")) returns 1.2.2 ignoring prerelease version.

Because of this behavior, the example below returns false, which seems a bug, but there's no clear way to fix this.

greaterThan(rangeMax(parseRange("<1.2.3-beta.20")), rangeMin(parseRange(">1.2.3-beta.10")))
  1. rangeMin ignores prerelease version only when the given range doesn't include prerelease.

rangeMin(parseRange(">1.2.3")) return 1.2.4, but actually 1.2.4-0 is smaller than that (and bigger than 1.2.3). On the other hand, rangeMin(parseRange(">1.2.3-a")) returns the correct value 1.2.3-a.0, but the switching of the algorithm depending on the input value seems confusing and arbitrary.


Note: There's no concern about the compatibility to npm:semver because these APIs are invented by us in #3385.

@iuioiua iuioiua added the feedback welcome We want community's feedback on this issue or PR label Feb 21, 2024
@timreichen
Copy link
Contributor

I agree, this seems very confusing.

Just a thought about 1. for the record:
Maybe this could be fixed by using something like MAX_SAFE_INTEGER instead of Infinity. But this seems hacky.

@iuioiua
Copy link
Contributor

iuioiua commented Feb 22, 2024

+1 for this.

Maybe this could be fixed by using something like MAX_SAFE_INTEGER instead of Infinity. But this seems hacky.

Using infinity might be fine to use in the underlying logic.

@timreichen
Copy link
Contributor

In we deprecate these two functions, there is no way to check anymore if a version is bigger/less than a range, because #4273 relies on these two as an alternative. Any thoughts?

@kt3k
Copy link
Member Author

kt3k commented Feb 23, 2024

That sounds a valid point. I think if we deprecate these 2 APIs, we need to re-implement gtr ltr equivalents without using rangeMin and rangeMax.

@iuioiua
Copy link
Contributor

iuioiua commented Apr 9, 2024

Can this now be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback welcome We want community's feedback on this issue or PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants