-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade Node.js and npm to v18 and v9 #48950
Conversation
e515207
to
85939a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some workflows are expected to fail in this PR:
- Performance tests
- Compressed size
- Pull request automation
The first two check out to a certain point at trunk which doesn't support later versions of Node.js and npm in the engines
settings yet. I don't know how to solve it for this PR, but it should work after merging to trunk though.
Pull request automation also check out to trunk to run the workflow, which doesn't support npm workspaces yet. This issue should also go away after merging to trunk.
@@ -1,5 +1,5 @@ | |||
exports.local = { | |||
host: 'localhost', | |||
host: '127.0.0.1', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be needed after node 17 for some reason.
@@ -39,7 +39,7 @@ module.exports = { | |||
// and add it first so https://github.com/facebook/react-native/blob/v0.60.0/Libraries/react-native/react-native-implementation.js#L324-L326 doesn't pick up the Platform npm module. | |||
moduleDirectories: [ | |||
'../../node_modules/react-native/Libraries/Utilities', | |||
'../../node_modules', | |||
'node_modules', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason the original ../../node_modules
doesn't work anymore after opting-in to npm workspaces.
@@ -10,20 +10,21 @@ For each user feature we should also add a importance categorization label to i | |||
--> | |||
|
|||
## Unreleased | |||
- [*] Increase the minimum Node.js version to 18 [#48950] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually not sure if this should be [*]
or [***]
. Maybe some RN folks could answer that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's [*]
because it's a change that initially won't impact users.
@@ -1,2 +1,3 @@ | |||
save-exact = true | |||
engine-strict = true | |||
legacy-peer-deps = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to be a temporary solution until we fix all the peer dependency issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevin940726 I created a draft PR that solves the peer dependency issue related to React Native and removes this NPM configuration option. As far as I tested, the dependency installation works, although warnings are displayed. Let me know if you'd like to incorporate it into this PR, thanks 🙇 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I tried the same thing too, but I think it would be better to be a separate follow-up PR. Warnings can be intimidating for some, I would like to solve all warnings before we roll them out to all contributors.
Ah right, sorry about that. We can land next week on Wednesday. I think we should also draft a short update this week to publish on Make Core that we plan to migrate Node and npm next week starting with Gutenberg, and we will follow up with WP core. |
Just wanted to note that on the Core side, we are blocked by this request to the systems team.. I'll reach out to try and get an update on this today. My preference is to merge this change here, and then merge the accompanying changes to Core right after (same day if possible). But it's not clear if that would require any
Is there any reason why we shouldn't make the minimum v16.x? 14.x is EOL starting April 30th. A secondary goal, IMO, should be to use this as an opportunity to trim all unsupported versions. Is the concern that anyone previously using 12 or 14 pulls updates to Gutenberg or Core and then are unable to run Node scripts? If so, maybe we leave it as 14 in this PR, and include in the post a specific date that we will change the minimum to 16 to allow contributors to update their local environment. |
I would avoid putting immediate pressure on all projects to upgrade to Node 16 because we recommend in the official documents and learning materials using Node 14. Once we update those resources to go with any LTS version, we can switch to Node 16 as the minimum version, or completely remove it from the config of WordPress packages and only leave a note in the corresponding README files. The biggest challenge is the different format for the lock file and more importantly the fact that we still didn't update packages to work correctly with the new behavior for peer dependencies. |
We will have to stick with Node.js 16 in WordPress core for now. See https://make.wordpress.org/systems/2023/02/09/upgrade-nodejs-npm-on-the-build-server/#comment-2077 for the context. There shouldn't be a big difference between v16 and v18. @desrosj, do you anticipate any challenges with using two different Node.js versions between both projects? We would still run tests against v16 and v18 on CI in Gutenberg. |
.github/workflows/unit-test.yml
Outdated
@@ -28,7 +28,7 @@ jobs: | |||
strategy: | |||
fail-fast: false | |||
matrix: | |||
node: ['14'] | |||
node: ['18'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have the same tests running for Node.js v16 and v18. We can change that as a follow-up if that isn't as simple as updating this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only problem is with the engines
setting in the root package.json
. It pins both Node and npm to the latest version so that we can ensure developers have the same local environment. However, CI will fail when running Node v16 because we have engine-strict
set to true. Perhaps we can ignore this setting on CI with the --engine-strict=false
CLI arg? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great to have it possible to run with different Node.js versions whatever the solution is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out it might be worth being in a different PR for clarity. Adding cli args to every command is too verbose, we might want to do it another way, like calling npm config set
before the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the GitHub action supports it, see how the registry URL gets passed here:
Follow-up sounds great 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any useful option in the GitHub action for this tweak:
https://github.com/actions/setup-node/blob/main/action.yml
We will figure out a different approach.
This should be ready now! I'll be away for a couple of days, so if anyone can help address any further feedback and merge this will be greatly appreciated! |
@kevin940726 @desrosj @gziolo What are the remaining steps for getting this across the line? I notice that April 30th date is approaching 😬 Do we need to update this PR to use node 16 as per this comment? Maybe we can schedule a date to merge this and commit the core patch. |
I thought that's for core, not gutenberg? The only remaining task I see is #48950 (comment), which can be solved in a different PR. LMK if there's something I missed though! Once someone approves the PR I'll rebase it and merge it :) |
It would be great to agree on the date and announce it ahead of time so contributors know when they will have to switch to a different Node.js version when developing for Gutenberg and WordPress core. I think that communication is key here to avoid any disruption, as there are different lock file formats between npm 6, npm 7/8, and npm 9. It's also why we need to enforce a certain pair of Node.js and npm in both projects so people don't get the lock file regenerated with a different lock file version all the time when using different Node.js major versions. I don't know whether we need to use Node.js 16 everywhere, but it definitely might be annoying to have to switch between Node.js 16 and 18 when contributing to both the Gutenberg plugin and WordPress core. Let's keep in mind that the maintenance for Node 16 ends on 2023-09-11 – in 5 months. Regarding the possible date, the next Gutenberg plugin RC1 release will probably happen today, and it will trigger npm publishing. It means that the next tentative windows for upgrading Node.js and npm are:
Technically, we don't need to do everything at once, but it could be easier to schedule all steps for a single week. |
Do we have an open PR on core for upgrading Node.js and npm, or is the work ongoing somewhere? Also, curious about the context of why we have to target Node 16 for core for now? |
I think there's one here - WordPress/wordpress-develop#4028 |
The WordPress.org build server that updates https://build.trac.wordpress.org/ when a commit to WordPress is made in SVN does not have Node 18 installed. 16.x is the newest version there. If the If I remember correctly, version requirements defined within As far as I can tell, the differences between 16 and 18 should be minimal. I don't think there is a reason we can't recommend 18 but still support (and test against) 16. We also need to adjust the |
Fair point! I can update it to make the
If I'm not mistaken, |
Playwright just dropped support for Node 14 - https://github.com/microsoft/playwright/releases/tag/v1.34.0. |
Hey @kevin940726 👋, I noticed it's been a couple of months since the last commit. I'm wondering if we found any blockers that are preventing the PR to continue or if we are just waiting for reviews. Regarding the blockers, and based on the above comments:
NOTE: Regarding React Native, I'd like to share that we're actively working on upgrading it to a version that uses React |
What?
Upgrade Node.js and npm to their latest (LTS) versions.
Related issues and prior art:
node
andnpm
to latest LTS versions #48588npm install
with npm 7 and higher #46652Notable Changes
Start using npm workspaces
Since npm v7, it introduces native support for workspaces to manage monorepo. It works great with
lerna
, which is also updated to the latest version in this PR (v6.5.1). We can now run scripts inpackage/
folder using the-w
command. For instance:See the official documentation for more info.
Upgrade lockfile to v3
This is done by an automatic update from the v1 lockfile. I believe that most packages are not changed, which can happen if we delete and regenerate the lockfile from scratch. This should be mostly backward-compatible, but it's still a breaking change, do file an issue if there's any bug!
Deprecate running binaries using
npm bin
andnode_modules/.bin
We should use
npx
instead.npm bin
is also deprecated in newer npm versions.Enable
legacy-peer-deps
This is mostly a temporary change until we fix all the underlying dependencies issues in the project. See #48588 for more info and the tracking issue.
Update the minimum required version of Node.js and npm in packages.
This is a breaking change. Prior art: #43141.
Why?
Node.js v14 will be EOL at the end of April. Node.js v16 is also going to be EOL in September. See the release schedule. It's a good idea to stay up-to-date and be long-term supported.
How?
Testing Instructions
nvm
to upgrade your Node.js version to v18. (nvm install 18
)npm i -g npm@latest
).npm ci
(this will delete thenode_modules
folder and regenerate it)npm run build
tsc --build
to work, you might have to delete the old cache (rm -rf packages/*/tsconfig.tsbuildinfo
).Testing Instructions for Keyboard
Same as above.