Repository navigation
Confusing process.release.compareVersion API #20053
Description
It might only be me but the current implementation is not intuitive for me because I expect the output to be exactly the opposite of what it is. Even though it is not right or wrong either way.
Let's give an example.
The current master version is 10.0.0-pre.
That way I expect 9.0.0 to return -1 because the input value is lower than the current version. Instead, it is 1, because the current version is bigger than the input value. So it depends on what viewpoint the user is in.
For me, it is more intuitive to have the input value being the main important part and not the current version (I hope it is clear what I want to express...).
So I would like to hear from others @nodejs/collaborators how they feel about this (it did not yet land on any release, so there would theoretically still be time to change this without being semver-major).
Activity
tniessen commentedon Apr 15, 2018
@devsnek originally landed the API with the signum as you described in #19438, which was reverted after I expressed my concern that the signum might be counterintuitive (see #19574 (comment)):
The following discussion was a bit difficult to follow as @devsnek edited his response about ten times, but in one of the first revisions he agreed with me.
devsnek commentedon Apr 15, 2018
I suggest you change it, as the way I see it isn't apparently very popular. I'll approve whatever pr is opened 👍
process: fix release.compareVersion logic
jdalton commentedon Apr 16, 2018
\cc @isaacs as author of the semver package for insight into API design.
rvagg commentedon Apr 16, 2018
Why is this even in core? This is the definition of feature creep, users should just be using the semver package to do this themselves rather than duplicating all that logic and prerelease rules.
The whole of #19587 is a smell, we're duplicating existing functionality (the numbers themselves! what's wrong with a
split()to get the major??) and functionality available via npm that includes some non-trivial rules that we need to make ensure is in sync (having dabbled in semver parsing and those rules I can say this is not straightforward).I'd +1 removing it entirely as a "fix", would I be alone in that?
rvagg commentedon Apr 16, 2018
proposing reversion @ #20062 along with some reasons and some notes on how the API is currently broken
BridgeAR commentedon Apr 22, 2018
The whole API got reverted. Closing therefore