process: add version constants and compare #19587

devsnek · 2018-03-25T05:44:06Z

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

mscdex · 2018-03-25T09:00:18Z

lib/internal/process.js

+    minorVersion,
+    patchVersion,
+  } = process.release;
+  const prereleaseTag = process.release.prereleaseTag.replace(/^-/, '');


Isn't this replace() unnecessary since substr(1, ..) is already done in C++ land?

mscdex · 2018-03-25T09:06:39Z

lib/internal/process.js

+        return -1;
+      if (prerelease && patch === patchVersion)
+        return 1;
+      if (patch > patchVersion)


Perhaps move these last two conditionals to the top of this section (after the minor version checks) and then drop the patch === patchVersion check from each of the other three conditionals?

mscdex · 2018-03-25T09:08:17Z

lib/internal/process.js

+      if (prereleaseTag && prereleaseTag === prerelease &&
+          patch === patchVersion)
+        return 0;
+      if (prereleaseTag && patch === patchVersion)


I think the the above two checks could be grouped together under a if (prereleaseTag) conditional to avoid checking it twice.

mscdex · 2018-03-25T09:09:27Z

I'm not so sure about the usefulness of allowing a single version string argument. I think it's just complicating things and might slow things down if version checks are being done during runtime.

tniessen · 2018-03-25T09:14:46Z

doc/api/process.md

+    * `tag` {string} The pre-release tag to compare.
+    * Returns: {number} `-1` if the given version is lower than the release
+      version, `0` if the given version matches the process version, and `1`
+      if the given version is greater than the release version.


I already explained in #19574 why this design is problematic:

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 #19438 implements the opposite.

In one of your earlier revisions of your comment in that PR, you seemed to agree with me. Is there a reason to stick to the old design?

tniessen · 2018-03-25T09:20:16Z

lib/internal/process.js

+      if (typeof major === 'string')
+        [major, minor, patch] = major.split('.');
+      if (/-/.test(patch))
+        [patch, prerelease] = patch.split('-');


Both split() calls are problematic if you are trying to comply with semver, see section 9 of the spec. The prerelease label can contain dots and dashes.

tniessen · 2018-03-25T09:23:44Z

lib/internal/process.js

+      minor = +minor;
+      patch = +patch;
+      if (/^-/.test(prerelease))
+        prerelease = prerelease.slice(1);


I don't think this is correct. As far as I understand the specification, 1.1.0-- is a valid version, where { major: 1, minor: 1, patch: 0, prerelease: '-' }.

tniessen · 2018-03-25T09:25:10Z

doc/api/process.md

+* `majorVersion` {number} The major version of Node.js.
+* `minorVersion` {number} The minor version of Node.js.
+* `patchVersion` {number} The patch version of Node.js.
+* `prereleaseTag` {string} The SemVer pre-release tag for Node.js.


I guess the name of this property originates from how the variable is called in node.cc? Is "prerelease tag" a common term for this? The spec does not use it.

Edit: I just saw that https://github.com/npm/node-semver#prerelease-tags also uses this term, so it is probably not a bad choice.

tniessen · 2018-03-25T09:26:07Z

doc/api/process.md

+  majorVersion: 4,
+  minorVersion: 4,
+  patchVersion: 5,
+  prereleaseTag: '',


As long as compareVersion is enumerable, it should be listed here.

tniessen · 2018-03-25T09:29:58Z

To me, the most important questions are: Do we need / want an API like compareVersion? In how far should it be semver-compliant regarding prerelease labels? Should it accept a string or a list of integers or both? I would love to hear some opinions.

vsemozhetbyt · 2018-03-25T09:37:01Z

doc/api/process.md

+* `minorVersion` {number} The minor version of Node.js.
+* `patchVersion` {number} The patch version of Node.js.
+* `prereleaseTag` {string} The SemVer pre-release tag for Node.js.
+* `compareVersion` {function} Perform a SemVer comparison to the release


Nit: {function} -> {Function} (only primitives are lowercased).

We may need to add the function signature here, with all the optional parameters marked with [] (nested or not).

I am not sure if this is a common practice to not single out such API in its own section. Let us see what others think.

vsemozhetbyt · 2018-03-25T09:48:40Z

lib/internal/process.js

+  const prereleaseTag = process.release.prereleaseTag.replace(/^-/, '');
+
+  process.release.compareVersion =
+    (major, minor, patch, prerelease = false) => {


What should happen if all of or some of major, minor, patch are undefined? Should the function throw or should these arguments be 0? Currently, it seems they are NaN in this case.

mscdex · 2018-03-25T21:12:46Z

lib/internal/process.js

+        return 1;
+
+      if (prereleaseTag) {
+        if (prereleaseTag === prerelease && patch === patchVersion)


~~I would shorten this to something like:~~

~~if (prereleaseTag) { if (patch === patchVersion) return (prereleaseTag === prerelease ? 0 : 1); }~~

Actually scratch that. How about this for the entire bottom part?:

if (patch > patchVersion) return -1; if (patch < patchVersion) return 1; if (prereleaseTag) return (prereleaseTag === prerelease ? 0 : 1); if (prerelease) return -1; return 0;

and possibly shorten the last two returns even further to just return (prerelease ? -1 : 0);

mscdex · 2018-03-25T21:13:18Z

lib/internal/process.js

+        if (patch === patchVersion)
+          return 1;
+      }
+      if (prerelease && patch === patchVersion)


patch === patchVersion can be removed here if this check is moved to the bottom.

mscdex · 2018-03-29T18:46:14Z

lib/internal/process.js

+  } = process.release;
+
+  process.release.compareVersion =
+    (major, minor, patch, prerelease = false) => {


I think the default value for the last parameter here can be removed as it's not necessary and will not affect tag comparisons when it is undefined instead of false.

ah hehe, copied from older implementation :p

devsnek · 2018-03-29T19:08:25Z

@tniessen ptal

Comments have been addressed.

tniessen · 2018-03-29T22:17:32Z

#19587 (comment) should probably be addressed, and #19587 (comment) should be discussed.

cjihrig

Mostly LGTM, but left a few comments.

cjihrig · 2018-03-30T15:32:13Z

lib/internal/process.js

+    if (typeof minor !== 'number')
+      throw new ERR_INVALID_ARG_TYPE('minor', 'number', minor);
+    if (typeof patch !== 'number')
+      throw new ERR_INVALID_ARG_TYPE('patch', 'number', minor);


The last argument should be patch I think.

cjihrig · 2018-03-30T15:34:39Z

test/parallel/test-process-release.js

+  compareVersion,
+} = process.release;
+
+assert.strictEqual(0, compareVersion(major, minor, patch, tag));


Can you change the order of the arguments in these assertions to reflect the assert.strictEqual(actual, expected) signature.

cjihrig · 2018-03-30T15:36:39Z

test/parallel/test-process-release.js

+assert.strictEqual(0, compareVersion(major, minor, patch, tag));
+
+assert.strictEqual(-1, compareVersion(major, minor, patch + 1, tag));
+assert.strictEqual(1, compareVersion(major - 1, minor, patch, tag));


I think more tests are needed to achieve code coverage - increment/decrement each value and pass bad values for each.

devsnek · 2018-04-05T01:42:47Z

unless anyone objects i think i'll land this on friday

targos · 2018-04-05T06:41:03Z

doc/api/process.md

+
+Perform a SemVer comparison to the release version.
+
+* `major` {string | number} The major version to compare or a string containing


A string is not accepted by the function

apapirovski

A few nits and one relevant question / note that I think needs to be addressed?

apapirovski · 2018-04-06T10:53:48Z

test/parallel/test-process-release.js

+  [0, 0, 0, 0],
+]) {
+  common.expectsError(() => {
+    compareVersion(args[0], args[1], args[2], args[3]);


Nit: could just ...args here?

apapirovski · 2018-04-06T10:58:25Z

doc/api/process.md

@@ -1509,6 +1512,10 @@ tarball.
  - `'Argon'` for the 4.x LTS line beginning with 4.2.0.
  - `'Boron'` for the 6.x LTS line beginning with 6.9.0.
  - `'Carbon'` for the 8.x LTS line beginning with 8.9.1.
+* `majorVersion` {number} The major version of Node.js.


Nit: for consistency, this might be better as "of this release" or something?

apapirovski · 2018-04-06T10:59:46Z

doc/api/process.md

+
+* `major` {number} The major version to compare or a string containing the
+  entire version.
+* `minor` {number} The minor version number to compare.


Nit: can we stick to "version number" or just "version" consistently between the three descriptions for major, minor & patch?

apapirovski · 2018-04-06T11:03:33Z

doc/api/process.md

+
+Perform a SemVer comparison to the release version.
+
+* `major` {number} The major version to compare or a string containing the


It's totally possible I'm missing something here but I don't see the functionality for

or a string containing the entire version

implemented in this PR. Did I miss something?

devsnek · 2018-04-06T14:04:51Z

CI https://ci.nodejs.org/job/node-test-pull-request/14097/

devsnek · 2018-04-07T03:58:47Z

ci attempt 2 https://ci.nodejs.org/job/node-test-pull-request/14111/

devsnek · 2018-04-07T04:50:43Z

both times failures are unrelated, I'll land this soonish

devsnek · 2018-04-14T05:03:31Z

Landed in 91e0f8d

PR-URL: #19587 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

rvagg · 2018-04-16T07:08:07Z

tagged as dont-land-on-v9.x, I'm proposing reversion @ #20062 and would like that to be resolved before moving forward with a 9.x release

PR-URL: #19587 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

bcoe · 2018-08-15T00:12:23Z

@rvagg @devsnek could you bring me up to speed as to why this was ultimately closed. Feels like having a numeric major, minor, patch, prereleaseTag could simplify feature detection for upstream libraries.

I'm +1 that we don't want to pull in any additional semver functionality, unless we go all the way and merge in the library semver. But, having the major and minor broken out in a parameter could be nice.

rvagg · 2018-08-15T11:15:08Z

compareVersion() is the controversial piece here. As for the numeric options, they are less of a problem and may slide in if attempted by themselves. Although it's really not hard to do a quick slice and parse or do a proper job and pull in semver to do a real compare that takes into account all of the edge cases.

devsnek requested review from mscdex and tniessen March 25, 2018 05:44

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Mar 25, 2018

mscdex reviewed Mar 25, 2018

View reviewed changes

tniessen previously requested changes Mar 25, 2018

View reviewed changes

vsemozhetbyt reviewed Mar 25, 2018

View reviewed changes

mscdex reviewed Mar 25, 2018

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 26, 2018

devsnek force-pushed the feature/process-release-more-version branch from ba49805 to 526836c Compare March 29, 2018 16:24

mscdex reviewed Mar 29, 2018

View reviewed changes

devsnek force-pushed the feature/process-release-more-version branch from b7aef4a to bb27e9c Compare March 29, 2018 23:31

cjihrig approved these changes Mar 30, 2018

View reviewed changes

jasnell approved these changes Apr 1, 2018

View reviewed changes

targos reviewed Apr 5, 2018

View reviewed changes

devsnek force-pushed the feature/process-release-more-version branch from 3913cef to 4be45cd Compare April 5, 2018 16:53

apapirovski approved these changes Apr 6, 2018

View reviewed changes

devsnek added 3 commits April 6, 2018 08:42

process: add version constants and compare

5152f79

squash! more tests

251f6e9

squash! fix doc

5c59dd7

squash! more stuff

bd94813

devsnek force-pushed the feature/process-release-more-version branch from 4be45cd to bd94813 Compare April 6, 2018 13:44

devsnek closed this Apr 14, 2018

devsnek deleted the feature/process-release-more-version branch April 14, 2018 05:03

devsnek added a commit that referenced this pull request Apr 14, 2018

process: add version constants and compare

91e0f8d

PR-URL: #19587 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

BridgeAR mentioned this pull request Apr 16, 2018

process: add release.compareVersion(str) functionality #20055

Closed

4 tasks

This was referenced Apr 16, 2018

Confusing process.release.compareVersion API #20053

Closed

Revert "process: add version constants and compare" #19587 #20062

Closed

rvagg added the dont-land-on-v9.x label Apr 16, 2018

jasnell pushed a commit that referenced this pull request Apr 16, 2018

process: add version constants and compare

cf4e6fd

PR-URL: #19587 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

MylesBorins added the dont-land-on-v8.x label Aug 7, 2018

devsnek mentioned this pull request Aug 14, 2018

[WIP] fs: feature detection for recursive mkdir[Sync] #22302

Closed


		Perform a SemVer comparison to the release version.

		* `major` {string \| number} The major version to compare or a string containing


		Perform a SemVer comparison to the release version.

		* `major` {number} The major version to compare or a string containing the

process: add version constants and compare #19587

process: add version constants and compare #19587

Conversation

devsnek commented Mar 25, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Mar 25, 2018 • edited Loading

tniessen Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Mar 25, 2018

vsemozhetbyt Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

vsemozhetbyt Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

mscdex Mar 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Mar 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Mar 29, 2018

tniessen commented Mar 29, 2018

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Apr 5, 2018

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Apr 6, 2018

devsnek commented Apr 7, 2018

devsnek commented Apr 7, 2018

devsnek commented Apr 14, 2018 • edited Loading

rvagg commented Apr 16, 2018

bcoe commented Aug 15, 2018

rvagg commented Aug 15, 2018

mscdex commented Mar 25, 2018 •

edited

Loading

tniessen Mar 25, 2018 •

edited

Loading

tniessen Mar 25, 2018 •

edited

Loading

vsemozhetbyt Mar 25, 2018 •

edited

Loading

vsemozhetbyt Mar 25, 2018 •

edited

Loading

mscdex Mar 25, 2018 •

edited

Loading

mscdex Mar 26, 2018 •

edited

Loading

devsnek commented Apr 14, 2018 •

edited

Loading