process: remove compareVersion and computedVersion (alternative to #19577)

[tniessen]

I apologize for voicing my opinion after #19438 has already landed, I simply didn't see the PR before. Whether there is agreement that there should be a built-in way to compare the node version against a known version or not, both compareVersion and computedVersion are flawed in my opinion. The PR received very little attention and I would like to propose removing two of the added properties until a well-designed solution is found, mostly to avoid having to maintain them because they make their way into a release.

There are some problems with the added compareVersion function:

1. The signum of the result of compareVersion seems counter-intuitive to me. In most (if not all) frameworks, a comparator returns a negative result if the first argument (or this) is considered to be "before" the second argument, and a positive result if the first argument (or this) is considered to be "after" the second argument (see e.g. java.lang.Comparable for a single-argument comparison). compareVersion contradicts this well-established norm. I would expect compareVersion(10, 0, 0) >= 0 to return whether this release is v10 or newer, which follows the norm and is intuitive, but process: add lots of version properties #19438 implements the opposite.

2. The documentation claims that this is a semver comparison, but the semver specification has specific rules for semver precedence, and this PR does not implement that behavior correctly, especially for prerelease tags. The documentation should either clearly state that this function does not conform to the semver specification or be adapted to do so.

3. Most users will likely expect an API that accepts a string instead of three distinct integer values.

On the other hand, computedVersion is something I don't like seeing exposed to users, and it seems like compareVersion was supposed to be a user-friendly alternative based on the discussion in #19438. People will need to construct the required constants for comparison with computedVersion, and process.release.computedVersion >= ((9 << 8) | 5) << 8 | 1 is not really pretty (and no, using the constant 591104 directly or storing the value in a named constant does not improve that all that much), so people will likely still use a proper semver implementation or manually compare process.release.xxxVersion. (process.release.computedVersion also ignores the prerelease tag.)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

FYI @jasnell @BridgeAR @devsnek @Trott

[mscdex]

I think it would be better to still test these other properties.

[mscdex]

One suggestion, otherwise LGTM.

[devsnek]

@tniessen as long as you're removing it maybe you can try to come up with an alternative. compareVersion will only fail a semver check if the patch or minor happen to be greater than 255 which is unlikely to ever happen. perhaps this pr could add a pre-release parameter, or separate out the minor, major, patch, and manually compare them in order (which would do the same thing but i think people would get less confused about whether this is a safe semver check)

I would expect compareVersion(10, 0, 0) >= 0 to return whether this release is v10 or newer, which follows the norm and is intuitive, but #19438 implements the opposite.

this seems reasonable, you could just propose that behavior change

Most users will likely expect an API that accepts a string instead of three distinct integer values.

we can pretty easily add this (split on . and -)

i'm not sure why the following isn't what this pr does, which would address all your issues (and feel free to use this code, i didn't copy it from anywhere)

function compareVersion(major, minor, patch, preRelease = false) {
  if (typeof major === 'string')
    [major, minor, patch] = major.split('.');
  if (/-/.test(patch))
    [patch, preRelease] = patch.split('-');

  if (+major > release.major)
    return 1;
  else if (+major < release.major)
    return -1;
    
  if (+minor > release.minor)
    return 1;
  else if (+minor < release.minor)
    return -1;
    
  if (+patch === release.patch && !preRelease) {
    return 0;
  } else if (+patch > release.patch) {
    return 1;
  } else {
    return -1;
  }
}

[tniessen]

@devsnek To clarify, the sole purpose of this PR is to make sure that the existing API does not land on any release line. The problem is not the implementation but rather the design, and I would like any new design to be discussed extensively.

We need to maintain any added APIs, especially when they are included in an LTS release, so my main priority is to make sure that these properties are not included in node 10 as they are. This PR is simply the easiest and fastest way to do so.

Please don't be offended by this change, if anything, it shows that our review process did not prevent this API from landing in the first place.

---

I would expect compareVersion(10, 0, 0) >= 0 to return whether this release is v10 or newer, which follows the norm and is intuitive, but #19438 implements the opposite.

i would also expect this, if there is a flaw in how i implemented this how about it is fixed instead of ripping it out?

I had no idea you considered this an implementation flaw as the implementation matches your documentation and tests. According to your documentation, implementation and tests, this expectation is false, so I assumed this was the intended behavior.

Most users will likely expect an API that accepts a string instead of three distinct integer values.

again this is a change we can pretty easily add (split on . and -) instead of removing the entire api

I will happily discuss any such changes extensively and I would love to hear other collaborators' opinions about this API in both its current and your proposed form, but as that process takes time, the safest approach (for now) is to remove the current API.

[devsnek]

Please don't be offended by this change, if anything, it shows that our review process did not prevent this API from landing in the first place.

i'm not offended by wanting the api to be changed, i'm just opposed to removing it instead of changing it. the time right now could have been spent in a pr which changed the implementation, discussing proper api design.

[jdalton]

i'm not offended by wanting the api to be changed, i'm just opposed to removing it instead of changing it.

The original PR landed prematurely, without following proper process, so it should probably be reverted so this feature can be shaped/solidified a bit more before landing.

[tniessen]

i'm opposed to removing it instead of fixing it. the time right now could have been spent in a pr which changed the implementation. discussing proper api design.

@devsnek I already explained why I decided to propose removing the API in my last comment: it is "the easiest and fastest way" to minimize the potential harm of the current API by making sure that nobody uses it and that it does not land on release lines, and I decided to spend my time doing that (and personally, I don't need much time to partially revert a commit).

Let me put it this way: It takes more time to design, discuss, implement and test a new API than to remove a currently unused, problematic API. (And yes, in such a big open-source project, undoing changes is time-critical.)

Your PR, #19438, is a great example: The new API was not discussed properly and if I wouldn't have spoken up, we could have ended up having to maintain a flawed API. I do not wish to repeat that mistake, and thus I will try to do everything possible to make sure that any new API is discussed properly before landing, even if that takes time.

---

@mscdex PTAL

---

@jdalton Would you prefer a complete revert? I kind of like having majorVersion etc.

[jdalton]

Would you prefer a complete revert? I kind of like having majorVersion etc.

Yes. It doesn't really matter what other things landed with the PR. It was just a false start and should be reverted so it can be given time to bake. I believe a revert is imminent.

[tniessen]

@jdalton I did not notice it was landed incorrectly, full revert in #19577.

[tniessen]

Closing in favor of #19577.