deps: Upgrade to Node v16, in setup docs and in CI #5356
Conversation
|
Should wait to merge this one until CI passes, because this is a rare PR that touches the CI workflow config directly. |
tools/test
Outdated
| @@ -192,12 +192,12 @@ check_node() { | |||
| local node_version | |||
| node_version="$(node --version)" | |||
| case "${node_version}" in | |||
| v1[0-9].*) ;; | |||
| v[1-9][0-9].* | v[1-9][0-9][0-9].*) ;; | |||
There was a problem hiding this comment.
v[1-9][0-9].* | v[1-9][0-9][0-9]* handles the v1000 problem.
But there’s a more standard solution: engines in package.json.
There was a problem hiding this comment.
Hmm interesting, thanks!
Taking the docs literally, it wouldn't actually do anything for us:
Unless the user has set the
engine-strictconfig flag, this field is advisory only and will only produce warnings when your package is installed as a dependency.
because we're not getting installed as a dependency. But perhaps these docs aren't quite solid enough to take literally. I'll experiment.
There was a problem hiding this comment.
Great, yeah, in reality it works. If I set it to >=20:
$ time yarn install
yarn install v1.22.17
[1/5] Validating package.json...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=20". Got "14.18.3"
error Found incompatible module.
(I guess these docs are only claiming to document npm anyway, not yarn. Trying npm install, it seems to hang -- or maybe just takes a long time without producing much output -- and I don't feel like digging into it.)
|
CI passed; and did indeed get Node v16: |
|
(BTW I did post a revision applying Anders's feedback. I see I hadn't posted a comment saying so.) |
| @@ -51,7 +51,7 @@ details to worry about. | |||
| Before starting, install these dependencies if you don't have them: | |||
| * [Git](https://git-scm.com/) | |||
| * [Node.js](https://nodejs.org/en/download/package-manager/): use the | |||
| **latest 12.x** version, not 14.x or later | |||
| **latest 16.x** version, or a later LTS version | |||
| * [Yarn](https://yarnpkg.com/en/docs/install), latest stable version | |||
There was a problem hiding this comment.
Does this mean the latest Yarn 1.x (classic) version, by the way?
There was a problem hiding this comment.
It does. I guess it'd be good to say that explicitly, though it looks like that URL continues to be about the classic versions.
Also I guess at some point we should look at migrating to the "modern" versions. For a while there it looked like a complex migration for something that wouldn't ultimately work for our form of deployment, and it seemed like the developers didn't even really understand why everything hadn't moved to their new model. But it looks like they now promise:
https://yarnpkg.com/getting-started/migration#step-by-step
that the new model isn't required, and that Yarn v2+ should be an improvement even while continuing to use node_modules.
|
It also LGTM. 🙂 |
I've been using this for local development for a few weeks now, and all seems well. (That's longer than it needs to be; I just hadn't come back to this to follow up.) Node v12 is going EOL in just over a week, 2022-04-30, so it's past time to make this upgrade official. Also delete an old troubleshooting entry for a problem with Node v11 that was avoided by sticking to v10; and fix two other references to Node v10 (where the problem was using an older version like v8) so that they remain accurate now and for further upgrades in the future. In particular, where we had been checking in our tests for Node v10+ (but in fact for >=10 and <20), just replace it with handy metadata in our package.json to make Yarn (or `npm`) complain at install time. Thanks to Anders Kaseorg for pointing out that package.json feature. Fixes: zulip#4263
|
Thanks for the reviews! Merged. |
A bit of background discussion here: #5356 (comment)
I've been using this for local development for a few weeks now,
and all seems well. (That's longer than it needs to be; I just
hadn't come back to this to follow up.)
Node v12 is going EOL in just over a week, 2022-04-30, so it's
past time to make this upgrade official.
Also delete an old troubleshooting entry for a problem with Node v11
that was avoided by sticking to v10; and fix two other references to
Node v10 (where the problem was using an older version like v8) so
that they remain accurate now and for further upgrades in the future.
(At least until Node v1000. On Node's current release cadence, that
will happen in a little under 500 years. I think we can tolerate
having to tweak this check then.)
Fixes: #4263