Go back to using explicit node verisons in CI

[jakebailey]

Fixes #51943

We were using "*" and "lts" assuming that one would be the most recent node release, and the other be LTS. This turns out to be wrong because "*" means "whatever is newest in GHA's cache", not the actual latest.

We could switch to say "current" or "latest", which would ensure we always get the actual current Node version. However, the current setup has another problem; it assumes that there are only two LTS versions out at one time. This isn't accurate; right now, Node 14, 16, and 18 are all LTS, and we are now suddenly not testing Node 14, which is scary.

I don't see a way to solve this, because at some points of the year, there will be two LTS releases, and sometimes, there will be three.

Just go back to explicitly listing the versions we test with. This will make us definitely run on all of our intended targets, but also mean old release branches won't suddenly test on the wrong versions, which is good.

We'll have to remember to update these, but I am personally confident that is fine. I also imagine that we don't want to drop Node 14 anytime soon, because it's the last version with stack restarting in debugging, a feature that many on the team use (including me).

[jakebailey]

Well, that's disconcerting; our tests fail on Node 19. I guess it's good we didn't get upgraded...?

[jakebailey]

Okay, so, Node 19 (probably v8) changed the formatting of Dates; instead of just 12:00:00 AM with a space character between them, toLocaleTimeString in en-US now returns a string where the space is \u202F, the narrow non-breaking space. Wow.

[rubiesonthesky]

Okay, so, Node 19 (probably v8) changed the formatting of Dates; instead of just 12:00:00 AM with a space character between them, toLocaleTimeString in en-US now returns a string where the space is \u202F, the narrow non-breaking space. Wow.

You probably already found this issue but linking it in case you did not spot it, nodejs/node#45171

[jakebailey]

Yikes, I guess it's an icu change, not even a node change. This is just a testing bug, so I'll just change the helper to fix the character. This sort of thing could happen to any version of Node, I'm guessing.

Cross linking:

• Error Date.toLocaleString() in node 19.0.0 nodejs/node#45171
• deps: update ICU to 72.1 nodejs/node#45068
• Date.toLocaleString contains invalid delimiter character nodejs/node#45753

[ddzz]

node-version: "*" is still used in other parts of .github/workflows/ci.yml. We may want to update/remove all uses of "*" in this PR.

[jakebailey]

node-version: "*" is still used in other parts of .github/workflows/ci.yml. We may want to update/remove all uses of "*" in this PR.

There's no major need to do this; all of the tasks which don't pin a version of Node don't care about the particular version other than that it's new enough.

[ddzz]

I think we can take out L27 here?

[jakebailey]

No node-version means "use whatever random version is installed on the runner on $PATH", and is used if you want to set up other Node stuff like proxies or auth, but keep the version active on the runner. I don't think that's what we want.

[ddzz]

L27 re-introduces the duplication problem from the original issue. There are now Node * and Node 18 jobs, which are duplicates.

[jakebailey]

This is an entirely different build with --bundle=false. It's not a duplicate.

[ddzz]

Ah, I missed that, never mind! Thanks for jumping on my bug report so quickly.