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

[rvagg]

This reverts commit 91e0f8d, PR #19587

This is not a good API addition and should not make it in to Node 10. Here's some reasons:

• It's unnecessary feature bloat
• Major version can be fetched by a single-line of JavaScript, there is no good case for the other xVersion properties and this is just more for us to maintain
• It turns process.release into a complex object with a method, rather than a simple object that contains simple classes (JSONifiable)
• It forces us maintain the complex semver rules, these rules are already encapsulated in node-semver which has been battle tested and gone through so many revisions to get the rules just right, now we need to duplicate all of that and maintain that compatibility. Users should just install node-semver and use it if they need to do anything complex with the Node version.

Aside from being unnecessary feature bloat (my main objection), that last point is critical to consider. Here's how this is already a broken feature, let's take a 10.0.0-rc.2 (not available yet but it will be soon)

$ ./node -v
v10.0.0-rc.2
$ ./node -p 'process.release.compareVersion(9,0,0)'
1
$ ./node -p 'process.release.compareVersion(11,0,0)'
-1
$ ./node -p 'process.release.compareVersion(10,0,0,"foo")'
1
$ ./node -p 'process.release.compareVersion(10,0,0,"rc.1")'
1
$ ./node -p 'process.release.compareVersion(10,0,0)'
1 # WRONG
$ ./node -p 'process.release.compareVersion(10,0,0,"rc.9")'
1 # WRONG
$ ./node -p 'process.release.compareVersion(10,0,0,"zzz")'
1 # WRONG

Yes we could fix these, but that's not the point, the fact that these even need fixing shows what a mess we're wading in to here. Review semver.org and look particularly at the pre-release rules. This is not a simple specification and it's taken a long time for node-semver to get it just right. We should leave this kind of thing to userland. If we include a proper semver parser and comparator then we'll next be getting calls to expose that from core .. this is how our stdlib bloats and it's not how Node was supposed to work.

/cc @devsnek @BridgeAR @tniessen @cjihrig @jasnell @apapirovski

[rvagg]

Sorry, I could be more tactful than "This is not a good API". I should have said simply that I don't think this meets our standards or our historical consideration for the addition of features. The rest of my points still stand though.
Another area this breaks right now is nightlies, consider today's nightly build compared to a nightly produced when we first incremented the major version to 10. These two are clearly different but this API would return the same result whichever way you did the compare.

[mafintosh]

Agreed that does not belong in core and should be left to userland, where good implementations already exists

[BridgeAR]

I do not have a strong opinion either way if #20055 is applied (that would fix all broken examples and fixes the non intuitive API). I read the spec very closely and the only difference with #20055 is not checking numbers properly in pre-release tags and accept some non-semver versions for it to compare too (0.x.x). That should not be important for that functionality though, as I expect it to be used only with other valid Node.js versions and in this case we are safe (I guess we do not have more than 9 pre-releases of the same type). And I would also be fine to properly implement the tag validation (it is not really that difficult).

I definitely do not like that it adds a function to process.release, but I believe it is handy for a lot of people. So 🤷‍♂️

[tniessen]

I did not approve the change being discussed, nor did I approve the first PR (which was reverted as well), mainly because I was not sure about the API design (the first one was undoubtedly flawed). I personally don't like having a half-baked SemVer implementation in core, and seeing the discussion in #20053, it seems to even cause confusion (maybe compareVersionTo would have been a better name to make the signum of the result obvious).

[devsnek]

its cool that this is getting reverted and all but i'm floored that people seem unable to bring these issues up on the original PR. a total of TEN DAYS pass without anyone having any issues with this, and the PR was open for 22 days!

and as a sidenote of "userland" i think its preposterous that i have to install the semver package to make sure my package is in node version X, if the team feels that way please merge semver into core

[devsnek]

also if possible can you just do a partial revert of the function, those other props are kinda useful

[apapirovski]

IMO @nodejs/tsc should discuss and determine whether this is something that needs to be in core or not. I don't see any other way at this point. It's a bit tiresome that we've gone back and forth with reverts on this functionality.

[tniessen]

@devsnek I would have requested changes on the first PR if I had seen it before it landed, but I didn't feel strong enough about the second one to block it and I didn't want to cause further frustration after urging to (at least partially) revert the first PR. Maybe we should have some stricter rules for semver-minor PRs as well, I think API additions should be handled especially careful. We got a very stable set of rules for semver-major PRs, but semver-minor PRs can cause similar and sometimes even more trouble.

[jasnell]

I'm fine with a revert on this but I have to share @devsnek's frustration. I've been generally seeing a growing lack of engagement from folks while PRs are open only to find folks raising objections and complaints after the fact. It's rather concerning.

[bnoordhuis]

There's simply a lot going on. I try to keep an eye on new PRs but I still miss one from time to time. It must be much worse for anyone not working on Node.js full-time.

[jdalton]

I've been similarly frustrated too, but after just trying to keep up with ES module related things I can see how stuff can easily be missed. I know Node has its parts split into groups with folks involved in each group (modules, napi, etc). Is there such a group for process? The idea being getting stakeholders (more eyeballs) involved sooner than later can help to avoid things like this.

[Trott]

@nodejs/process

[Trott]

I don't want to hijack this issue for a larger discussion around how to manage project velocity and the frustrations around both stuff landing without seeing it and stuff getting reverted because of that. So I opened a discussion here: https://github.com/orgs/nodejs/teams/collaborators/discussions/10

[joyeecheung]

I am sorry to say that I only noticed this PR after it landed and I was not very comfortable with the API when I saw it (especially about the choice of prereleaseTag since the tag could be pretty flexible), but I did not feel strong enough to revert it myself.

[devsnek]

@joyeecheung just to be clear, semver spec calls it pre-release

[joyeecheung]

@devsnek Yes but we do not strictly obey the semver spec. For instance, the pre-release tag is used to indicates that the version is unstable and might not satisfy the intended compatibility requirements as denoted by its associated normal version., but we have the -node.x suffix for v8 versions.

[joyeecheung]

@devsnek Hm, wait, V8 versions are not semver, so that's a bad example, sorry. A probably better example: we bump patch versions for security releases, even when they could be breaking.

[jasnell]

If this is going to make it in time for 10.0.0 it needs to be landed today.

[Trott]

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

[tniessen]

Is there anything preventing this from landing?

[Trott]

Needs a green CI on OS X, but that's it, I think. Here's a re-run: https://ci.nodejs.org/job/node-test-commit-osx/17975/

[Trott]

It's green now. Landing...

[Trott]

Landed in 287ec1e

[rvagg]

@devsnek (and others), if I did not approach this in a sensitive-enough way then I'd like to hear it (feel free to email me privately if you prefer). I probably fired this off quicker than I should have and didn't think through my language enough. Reverting someone's code is not a trivial exercise and there is an understandable emotional burden and I don't want to downplay that.
Guidance for how to do this in a sensitive way is probably something we can discuss in https://github.com/orgs/nodejs/teams/collaborators/discussions/10, feedback on that would be appreciated as we don't want to make it feel harder to contribute to core, just make things more flexible and this kind of thing less painful.