[rush] Add support for "Current" Node version (12.x)

[mikeharder]

Please add support for the "Current" Node version (12.x), in addition to the "Active LTS" (10.x) and "Maintenance LTS" versions (8.x).

https://github.com/microsoft/web-build-tools/blob/b31c2e73ce549ce37531ef39034c860cf5782cd4/apps/rush/src/start.ts#L22-L36

[iclanton]

We've generally only supported versions of Node once they enter LTS. 12.x.x will enter LTS on October 22, 2019 (https://nodejs.org/en/about/releases/). Node's docs say:

After six months, odd-numbered releases (9, 11, etc.) become unsupported, and even-numbered releases (10, 12, etc.) move to Active LTS status and are ready for general use.

So we haven't supported versions of of Node until Node's developers deem them "ready for general use."

We could consider revising that to also include Current versions of Node. Are there specific features in Node 12 that you need?

[iclanton]

After some offline discussion, I'm convinced that we should support Current node versions in addition to LTS versions.

Many teams use Rush to build tools and libraries that are published publicly, and from the Node docs:

Major Node.js versions enter Current release status for six months, which gives library authors time to add support for them.

We should help developers who use Rush make sure their projects are ready for the LTS release during the six-month transition period.

[octogonz]

A few questions:

• Is this request really about /support/? Or is it merely asking for a way to suppress the warning notice?

• Are "Current" versions highly unlikely to contain bugs? (In the past we could waste several hours investigating someone's issue, only to realize that it was a stupid NodeJS bug. Rejecting non-LTS versions was an effective way to eliminate these headaches.)

• Is there a programmatic way to distinguish version numbers that are "Current" versus "Active"?

[bterlson]

My thoughts on above:

• This is about support. Node developers have a strong inclination to use latest stable Node, especially during development. As the Azure SDK must support latest stable Node versions, it would be nice if Rush would support this as well.
• Current is incrementally more likely to contain a bug, but in my experience, not appreciably so. But based on above, it shouldn't matter much - it's where users are, so we should be there too. There is also value in finding bugs/mis-features in Node early as we can drive fixes before it becomes broadly problematic.
• I don't think you can detect LTS based on version numbers unfortunately :(

[mikeharder]

I will submit a PR for this today.

[octogonz]

Current is incrementally more likely to contain a bug, but in my experience, not appreciably so.

That seems reasonable.

I don't think you can detect LTS based on version numbers unfortunately

If it's not a simple formula, then there should really be an NPM package that maintains a table of NodeJS version numbers, with an API that allows you to query whether a given version number is:

• Unstable
• Current
• LTS-Active
• LTS-Maintenance

Does that exist? If not, it should be pretty easy to create. We could just stick it in node-core-library, but I'd imagine other community people would want to use it.

When we tried to offer support for "Unstable" builds (and by consequence, encouraged users to go down that road), it was a dumpster fire.

[mikeharder]

Node does expose an API to detect whether a release is LTS:

lts <string> a string label identifying the LTS label for this release. This property only exists for LTS releases and is undefined for all other release types, including Current releases. Currently the valid values are:

https://nodejs.org/api/process.html#process_process_release

[octogonz]

Hrmm... I was confused before. I had wrongly assumed that "Current" was applied after a certain amount of stabilization releases, just like "LTS"

From PR #1381 I now understand that "Current" simply means the latest major version. In other words, 11.0.0 was considered "Current" when it was released.

The full story is here: https://github.com/nodejs/Release#release-plan

They say:

New semver-major releases of Node.js are cut from master every six months.
New even-numbered versions (e.g. v6, v8, v10, etc) are cut in April.
New odd-numbered versions (e.g. v5, v7, v9) are cut in October.

This seems to imply that pre-LTS 12.0.0 is forked directly from the master branch that, on the previous day, was the bleeding edge of the UNSTABLE bar in this diagram:

Is that correct?

Their CHANGELOG.md seems to support this. For example 10.16.0 latest LTS is fairly tame. Whereas 12.6.0 latest Current and 12.5.0 Current have comparably more churn.

It might be useful to revisit the point of this PR. The original error message said:

Your version of Node.js (${nodeVersion}) has not been tested with 
this release of Rush.  The Rush team will not accept issue reports for it.
Please consider upgrading Rush or downgrading Node.js.

The motivation behind that message was:

1. To educate people that it's unwise to use non-LTS releases in a large monorepo. For example, the SPFx repo services thousands of rush install invocations per week, and developers are free to choose their NodeJS version. If some flaky NodeJS version has a regression, it's fairly likely that someone will use it. A support ticket will get raised, and it will waste people's time. Of course, these considerations don't apply to small projects, small teams, or people who are just experimenting with new stuff.
2. To protect the tools team from wasting time investigating NodeJS bugs. As I mentioned earlier, this was happening a lot, and the warning message solved that problem.

Thinking about this though, "The Rush team will not accept issue reports" is a fairly heavy-handed statement that arguably goes way beyond the actual goals.

Lemme chat with @iclanton and see if we can come up with a clearer statement. It may seem like nitpicking, but we put a lot of energy into supporting people, so it's important to accurately communicate our position to the community.

[octogonz]

Here's what we came up with:

1. We will remove the "not accept issue reports" language from the warnings.

   For the "not LTS" message, change this (old) text:

   Your version of Node.js (${nodeVersion}) is not a Long-Term Support (LTS) release.
   These versions frequently contain bugs, and the Rush team will not accept issue reports
   for them.  Please consider installing a stable release.
   

   ...to say this instead:

   Your version of Node.js (${nodeVersion}) is not a Long-Term Support (LTS) release.
   These versions frequently have bugs.  Please consider installing a stable release.
   

   For the "too new" message, change this (old) text:

   Your version of Node.js (${nodeVersion}) has not been tested with this release of Rush.
   The Rush team will not accept issue reports for it.  Please consider upgrading Rush
   or downgrading Node.js.
   

   ...to say this instead:

   Your version of Node.js (${nodeVersion}) has not been tested with this release 
   of Rush.  Please consider installing a newer version of the "@microsoft/rush"
   package, or else downgrading Node.js.
   

   For the "known broken" message, it will stay the same as before:

   Your version of Node.js (${nodeVersion}) is very old and incompatible with Rush.
   Please upgrade to the latest Long-Term Support (LTS) version.
   

   (No changes for this message.)

2. We realized that the "too new" message applies separately for @microsoft/rush
   (the locally installed CLI package) versus @microsoft/rush-lib (the engine that gets
   automatically installed by the Rush version selector).

   Thus we need to add a separate check in rush-lib, with this text:

   Your version of Node.js (${nodeVersion}) has not been tested with this release 
   of the Rush engine.  Please consider upgrading the "rushVersion" setting in rush.json,
   or else downgrading Node.js.
   

   If we do this, we need to consider some edge cases:

   • This warning probably shouldn't be printed when rush-lib is invoked via the API.
   • This check probably shouldn't occur if the rush front end already printed an equivalent warning.
   • That includes the "unmanaged" case, e.g. when running rush init without any rush.json file

3. We should add a new setting in rush.json that allows @mikeharder to suppress the "not LTS"
   message for the purpose of testing pre-LTS versions. The idea is that we can use the
   documentation for this setting to recommend against using a pre-LTS version in a production repo
   while providing a way to disable the warning itself

   I'm proposing to add a suppressNodeLtsWarning setting like this:

   rush-init/rush.json

     /**
      * Older releases of the Node.js engine may be missing features required by your system.
      * Certain releases may have bugs.  In particular, the most recent version will not be a
      * Long Term Support (LTS) version and is likely to have regressions.
      *
      * Specify a SemVer range to ensure developers use a Node.js version that is appropriate
      * for your repo.
      */
   "nodeSupportedVersionRange": ">=10.13.0 <11.0.0",
   
   /**
    * Odd-numbered major versions of Node.js are experimental.  Even-numbered releases
    * spend six months in a stabilization period before the first Long Term Support (LTS) version.
    * For example, 8.9.0 was the first LTS version of Node.js 8.  Pre-LTS versions are not recommended
    * for production usage because they frequently have bugs.  They may cause Rush itself
    * to malfunction.
    * 
    * Rush normally prints a warning if it detects a non-LTS Node.js version.  If you are testing
    * pre-LTS versions in preparation for supporting the first LTS version, you can use this setting
    * to disable Rush's warning.
    */
   /*[LINE "HYPOTHETICAL"]*/ "suppressNodeLtsWarning": true,

   For odd-numbered versions, we decided that Rush should still print a warning. It can be like this:

   Your version of Node.js (${nodeVersion}) has an odd major version number.
   These releases frequently have bugs.  Please consider installing a Long Term Support (LTS)
   version instead.
   

4. For diagnostic purposes, Rush should log the Node.js version number and its status. The log would look like this:

   Node.js version is 8.15.0 (LTS)
   

   Node.js version is 8.1.0 (pre-LTS)
   

   Node.js version is 7.7.3 (unstable)
   

   We can use the process.release.lts API for this.

   It might be noisy to log this for every command. Maybe just for these ones:

   • rush init
   • rush install
   • rush update

5. Our GitHub issue template (Update issue templates #1376) should ask people to check this field. Something like this:

   Node.js version: (paste your "node -v" output here)

   Repros with an LTS version of Node.js? yes/no

   See https://nodejs.org/en/download/releases/ for a list of LTS version numbers.

6. Lastly, as a group we should test the pre-LTS versions in web-build-tools. Officially we will wait 4 minor releases before we start testing (e.g. we start with 12.4.0 not 12.0.0). But from a support perspective, we'll now happily accept PRs and issues that improve support for any pre-LTS version. (Note that odd-numbered major versions are not pre-LTS by my definition.)

How's that sound?

[iclanton]

Here's a PR that makes those changes to Rush: #1388

[octogonz]

In #1388 (comment) @mikeharder said:

Should suppressNodeLtsWarning apply to this warning as well? My preference would be for it to suppress both warnings.

Per our discussion above, the odd-numbered releases are not on any path to Long Term Support. Presumably they do stabilize over time, as compared to the v10.0.0-nightly20171114803cacd228 versions which are maximally unstable. However, the release plan's triage criteria states Changes in an LTS-covered major version are limited to. It does not say Changes in a "Current" major version are limited to. Thus, I would not expect the stabilization of an odd-numbered release to be particularly reliable.

If you just want to make the Rush warnings go away in your master branch, we can provide a setting for that. 👍

But if you're asking the Rush maintainers to invest resources in supporting non-LTS release branches, I'm curious about the rationale. Node.js isn't just a library, it's the runtime that executes every file from your toolchain. In the past we've seen serious Node.js regressions in bread-and-butter APIs like writing a file to disk. I'm somewhat uncomfortable about leading people to use half-baked releases for serious work.

[bterlson]

This plan seems fine to me!

One minor suggestion:

Your version of Node.js (${nodeVersion}) has an odd major version number.

Could be read in two ways: that the major version is not even, or that the major version is unusual 🤣. Also maybe people not familiar with Node's versioning tick-tock won't understand the import of an odd version number. Suggestion:

Your version of Node.js (${nodeVersion}) has an odd-numbered major version indicating that this is a pre-release version of Node.

"odd-numbered" is seemingly the official nomenclature. I'm not sure they also call them "pre-release" so I'm happy with some other term.

[octogonz]

BTW I didn't mean to sound dismissive about allowing odd-numbered builds. I interpreted this issue as calling attention to a usage scenario that we haven't considered in the past. Thus my wall of text is about understanding exactly how you guys use Rush, to make sure we can document it and support it properly.

I'm usually reachable on Gitter chat if anyone wants to talk through more details offline.

[octogonz]

I chatted about this offline with @mikeharder and he agreed that odd-numbered versions aren't important to support and the warning shouldn't be disabled for them. Thus I think we have a consensus about the design. Mike also pointed out a couple logic issues in the current PR that made the intent less obvious. We'll add PR comments about that.