process: add lots of version properties

[devsnek]

• process.release.{major,minor,patch,computed}Version
• process.release.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

[mscdex]

-1 because such checks (as suggested in the docs) reduce readability and it ignores release status.

[mscdex]

I'd rather prefer separate properties for each part of the version and a flag for release mode (if there isn't already one that I'm overlooking). This way versions checks are still just as easy/foolproof but are more understandable.

[devsnek]

@mscdex this is definitely not intended for readability. i have checks like:

const meetsVersion = (() => {
  if (process.versionMajor > 9)
    return true;
  return process.versionMajor === 9 && process.versionMinor === 8;
})();

and i might need that in three or four places for my tests, coverage, release scripts, etc and that doesn't fit very well into a node -e in a bash script. not everything needs to be verbose, just well designed.

[jasnell]

I'd prefer dropping this off process.release also to avoid adding anything new to the process object.

[jasnell]

'semver tagis not very descriptive. Perhaps aprerelease` property with a simple bool would work here?

[devsnek]

maybe prereleaseTag? i want to expose the actual tag itself, not just if node is pre-release

[jasnell]

Will the fact that the computed version does not incorporate the pre-release tag impact anything here? For instance, every version built on master will have the same computed version until the next major release.

[devsnek]

is process.computedVersion >= 591872 && process.prerelaseTag good enough?

[jasnell]

Yeah, as long as folks know to check both. Perhaps an additional note in the docs?

[devsnek]

#19438 (comment) hidden but maybe still unresolved

[mscdex]

I think this entire description is unnecessary as it should be pretty obvious how the version properties could be used. At the very least, 'this' should be more specific as there are multiple properties shown above.

[jdalton]

A couple questions:

• What's the use case for computedVersion?

• Is the mechanism for computing the computedVersion a common thing or
  done in other projects (python,php,etc.)?

[devsnek]

@jdalton

What's the use case for computedVersion?

if (process.release.computedVersion < 591616) {
  throw new Error('vm.Module not available before node 9.7')
}
// vs
if (process.release.majorVersion < 9 || (process.release.majorVersion === 9 && process.release.minorVersion  < 7)) {
  throw new Error('vm.Module not available before node 9.7')
}

node -e "process.release.computedVersion > 591872 || process.exit(1)"
if [ $? ]; then
  npm run coverage
fi
# vs
node -e "process.release.majorVersion > 9 || (process.release.majorVersion == 9 && process.release.minorVersion > 8) || process.exit(1)"
if [ $? ]; then
  npm run coverage
fi

etc...
you can even stick the number in your package.json and require it for consistency across a project. i was inspired with this while writing the source, tests, and coverage cases for my http request lib which conditionally uses http2

Is the mechanism for computing the computedVersion a common thing or
done in other projects (python,php,etc.)?

i have no idea, i don't use python, php, etc. however, this provides a similar mechanism to people building node addons using NODE_MODULE_VERSION and friends to support lots and lots of node versions

[jdalton]

The computed version isn't super readable. I use semver for these kind if things.

SemVer.satisfies(process.version, ">=8.5.0")

For the computedVersion have you done some research into if there are any collision cases?

[devsnek]

The computed version isn't super readable. I use semver` for these kind if things.

like i said before, it isn't designed for readability, and its absolutely bonkers that i would need to install an entire module just to perform a version check. if that's the path we're gonna take we should ship semver with node, and if shipping semver with node seems silly then there's your answer.

For the computedVersion have you done some research into if there are any collision cases?

what do you mean by that? if you mean when do the values overflow, we can't go higher than 255 for patch/minor and ~4398046511103 (with js's numbers) for the major

[jdalton]

like i said before, it isn't designed for readability, and its absolutely bonkers that i would need to install an entire module just to perform a version check.

Maybe make the the computedVersion as nice to use as semver. There could be a process method backed a computedVersion that does the comparison.

what do you mean by that?

I meant is there any combo of major + min + patch that could cause 2 computed values to be the same.

if you mean when do the values overflow

Overflow is another concern.

[devsnek]

I meant is there any combo of major + min + patch that could cause 2 computed values to be the same. Overflow is another concern.

> 1 << 16 // major version 1
65536
> 256 << 8 // minor version 256
65536

// ^^ will never happen, we're not going to release anywhere near 250 patch or minors

Maybe make the the computedVersion as nice to use as semver. There could be a process method backed by computedVersion that does the comparison.

i'm ok with something like process.release.satisfiesVersion() but i worried others would think it a bit much

[jdalton]

I think avoiding hard-to-reason-about numbers in source is a good thing. Hiding away the computed version behind a reasonable API also means the formula for computing it can change/adjust as needed.

[cjihrig]

I like the idea of process.release.satisfiesVersion(). I also use the semver module and hate adding a dependency just for that purpose. If you go down the process.release.satisfiesVersion() path, can I suggest returning -1, 0, or 1 instead of a boolean, and naming it compareVersion().

[devsnek]

i've added compareVersion, the only thing that worries me is that it involves calling a function which might not exist, but its a fairly easy check

[Trott]

If I'm not mistaken, technically this should not have landed because of @mscdex's -1.

@mscdex, in the future, you may wish to use Request changes to get the visible red X when objecting to a change.

[Trott]

@mscdex If you still feel strongly that this should not be in core, options are:

• PR to revert
• add all the dont-land-on labels

If @mscdex objects, option for anyone who feels strongly that this should remain on master and go into releases would be to add the tsc-agenda label and @-mention nodejs/tsc.

[vsemozhetbyt]

How can this be landed with linting error? This line exceeds length limit.

[vsemozhetbyt]

Last CI had a wrong PR number)

[Trott]

Last CI had a wrong PR number)

One more data point for "more automation, less manual entry"...

[tniessen]

I think avoiding hard-to-reason-about numbers in source is a good thing. Hiding away the computed version behind a reasonable API also means the formula for computing it can change/adjust as needed.

Wasn't the intention behind adding compareVersion that we don't need to expose computedVersion @jdalton? Seems like both were added.

[Trott]

Wasn't the intention behind adding compareVersion that we don't need to expose computedVersion @jdalton? Seems like both were added.

Since this hasn't gone out in a release yet, it's not too late to remove stuff if there's agreement on it.

[jasnell]

I agree this should not have landed yet. A revert would be appropriate

[tniessen]

@jasnell Partial revert of obviously problematic parts in #19574 and full revert in #19577 (alternatively).

[tniessen]

This change was reverted in eeb1b9d, partially due to problems with the exposed API as outlined in #19574.

@devsnek Feel free to open a new issue / PR to discuss your proposed changes, I'd be glad to take part in that discussion!