Skip to content

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

Rodrigo Muino Tomonari requested to merge github/fork/tniessen/process-remove-release-props into master

I apologize for voicing my opinion after https://github.com/nodejs/node/pull/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 https://github.com/nodejs/node/pull/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 https://github.com/nodejs/node/pull/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

Merge request reports