Skip to content
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

Update to Node.js 20.x #56331

Merged
merged 15 commits into from
Dec 20, 2023
Merged

Update to Node.js 20.x #56331

merged 15 commits into from
Dec 20, 2023

Conversation

desrosj
Copy link
Contributor

@desrosj desrosj commented Nov 20, 2023

What?

This updates the version of Node.js used in the repository from 16.x to 18.x.

Why?

16.x is now EOL as of September 11, 2023.

While it's true that 20.x is the current active version of Node.js, there are external circumstances that prevent WordPress Core from using Node.js 20.x at this time. Mainly, the WordPress.org build server that is responsible for building, bundling, and packaging what is eventually shipped as the WordPress software needs to have the version of Node.js specified in the package.json file installed.

The newest version of Node.js present on that server is currently 18.18.0. Though 20.x is the current version, 18.x is still supported as a maintenance LTS through April 20, 2025. For the time being, let's update to use 18.x so that the project is using a version actively supported by the Node.js team.

Even though we're unable to officially use 20.x to build the finished packages, I think it's still reasonable to allow it's usage and to test against it in our GitHub Action workflows to ensure compatibility with both versions. This PR defines 18 as the preferred version within the .nvmrc file and the minimum requirement in the package.json file. But it removes the upper limit that was there previously for npm. In my testing, I did not find any problems with allowing both versions, and in general the code here appears to be unaffected by any of the problematic changes between the two versions (though there may be some not discovered through current testing).

This is a companion to WordPress/wordpress-develop#5515, which should be committed shortly after this is merged.

Copy link

github-actions bot commented Nov 20, 2023

Flaky tests detected in 2bfa462.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7278268036
📝 Reported issues:

@desrosj desrosj marked this pull request as ready for review November 20, 2023 18:50
@swissspidy swissspidy added the [Type] Build Tooling Issues or PRs related to build tooling label Nov 20, 2023
@flootr
Copy link
Contributor

flootr commented Nov 27, 2023

I'm trying to make myself familiar with what it takes to update the Node.js version for Gutenberg.

Mainly, the WordPress.org build server that is responsible for building, bundling, and packaging what is eventually shipped as the WordPress software needs to have the version of Node.js specified in the package.json file installed

What other blockers do you have in mind besides the build server for shipping?

Also cross-referencing #55561

@desrosj desrosj requested a review from gziolo November 27, 2023 16:40
@desrosj
Copy link
Contributor Author

desrosj commented Nov 27, 2023

What other blockers do you have in mind besides the build server for shipping?

I can't see any at the moment since this PR seems to work without issue. But it's possible there are considerations buried deeper in the Gutenberg code I'm not aware of. I've tagged @gziolo for a review since he's helped me review these changes in the past.

Also, I've made a request to the systems team to install 20.x on the build server. It's possible we can skip 18.x all together, though I'd advise we proceed as though we'll only have 18.x available for this current cycle (WP 6.5). We can always bump a second time should something change.

@gziolo
Copy link
Member

gziolo commented Nov 29, 2023

@desrosj, as simple as that? We have come a long way 😄

In the past, we were forcing a specific npm major version, or even always checking with the most recent version on CI. However, this PR makes me think that it's probably no longer a case and finally folks should be able any version they like locally. Let's observe whether it won't trigger some random lock file changes in PRs opened, and only consider imposing limitations if something goes wrong. That's said let me test this branch locally with Node 20 and a higher version of npm.

I'm sure there are a few instances of documentation where we force contributors to use Node 16 and npm 8, so we should update them, too.

@gziolo gziolo requested review from noahtallen and flootr November 29, 2023 08:29
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally with Node.js v20 and npm v10 and it works like a charm.

I found only two occurrences of documentation pages that need to be reworded:

Gutenberg is a JavaScript project and requires [Node.js](https://nodejs.org/). The project is built using Node.js v16, and npm v8. See the [LTS release schedule](https://github.com/nodejs/Release#release-schedule) for details.

- [Node.js](https://nodejs.org/en/download/) version 16.14 or above.

There might be more instances, but it looks like some of them were updated recently with the focus on using the Active LTS version 🥇

@flootr
Copy link
Contributor

flootr commented Nov 30, 2023

I found only two occurrences of documentation pages that need to be reworded […]

I added 87ee195 – let me know what you think. I tried to include a passage about the goal of using of active LTS version of Node.js but can be removed if it's not suitable.

Also did a rebase.

@desrosj desrosj changed the title Update to Node.js 18.x Update to Node.js 20.x Dec 5, 2023
@desrosj desrosj mentioned this pull request Dec 5, 2023
@desrosj
Copy link
Contributor Author

desrosj commented Dec 5, 2023

Looks like 20.x was just installed on the .org build server. 🎉

I've updated the PR to skip 18.x. There is a failure that seems to be related to the version of Node/npm available on the Action Runner image. I will circle back to look into that further tomorrow, but have to step away for the time being. It seems to be that the Windows and MacOS runners do not have Node 20.10.0 installed, rather 20.9.0.

I've also updated the corresponding Core PR to use 20.x.

@noahtallen
Copy link
Member

I think the compressed size failure is to be expected (it's comparing something with trunk, so the required versions will be different), but the React Native failure is something else. For some reason, the setup node version step has a version slightly too old:

image

First idea is to update actions/setup-node to version 4 to see if that helps

@desrosj
Copy link
Contributor Author

desrosj commented Dec 19, 2023

@noahtallen So I think in the instance you have included the screenshot for, that is the expected behavior. It seems that the GitHub runner images are not all updated at the same time, and the actual runner machines take time to update. I think I've fixed this by including check-latest in all areas that setup Node. This will always make a request to confirm that the latest version of Node.js for the version range specified is installed and ready to be used.

This fixes the problem, but does add an additional request. I think in about a month's time, we can circle back to see if we can remove this to save the additional request (once all GitHub's images are reliablyusing a newer version).

The failure I'm seeing now is:

  Installing using npm ci
  /opt/hostedtoolcache/node/20.10.0/x64/bin/npm ci
  npm ERR! code EBADENGINE
  npm ERR! engine Unsupported engine
  npm ERR! engine Not compatible with your version of node/npm: gutenberg@17.3.0-rc.1
  npm ERR! notsup Not compatible with your version of node/npm: gutenberg@17.3.0-rc.1
  npm ERR! notsup Required: {"node":">=16.0.0","npm":">=8 <9"}
  npm ERR! notsup Actual:   {"npm":"10.2.3","node":"v20.10.0"}

I think this is expected until a version of the plugin is released with the new engines version ranges. Since we previously had a specific upper limit for npm (9 in this case), it can't run with npm 10. I believe this should fix itself. once that happens.

With this in mind, it may make sense to merge this the same day of a release to minimize the failures that will occur for PRs.

I've made a few other small adjustments that are mostly job name improvements. Once this is merged, we'll need to check the PR merge rules to ensure those don't need to be updated. I seem to recall required checks are specified by name. Not sure if that is still the case. A repository admin will need to check this.

For the workflows with a matrix to test Node, I also included a job that will test 21.x that is allowed to fail. I don't think that's a requirement to merge this and will happily remove it if everyone feels it's overkill. But it is one potential benefit of removing the upper limit allowed in engines. When 22 is released, we can switch to testing against that. The hope is that this will make upgrading in the future much easier.

@desrosj desrosj requested a review from gziolo December 19, 2023 17:50
@noahtallen
Copy link
Member

I think this is expected until a version of the plugin is released with the new engines version ranges. Since we previously had a specific upper limit for npm (9 in this case), it can't run with npm 10. I believe this should fix itself. once that happens.

Yeah, I think the compressed size check compares the current PR with trunk. Since trunk currently has the older engines field, but the action runner will use Node 20, we get the conflict. So it should disappear after merging to trunk! (And once PRs incorporate these changes as well.) I don't think a plugin release is needed for that.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well for me!

@gziolo
Copy link
Member

gziolo commented Dec 20, 2023

Nice, the Compressed Size job always fails when upgrading the Node version, but it works after landing the PR. We can ignore it and hope for the best 😄

@tyxla
Copy link
Member

tyxla commented Dec 20, 2023

Great to see this at the finish line 🎉

@desrosj desrosj merged commit 415ecc9 into trunk Dec 20, 2023
55 of 56 checks passed
@desrosj desrosj deleted the update/node-to-18-x branch December 20, 2023 18:01
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 20, 2023
@afercia
Copy link
Contributor

afercia commented Dec 21, 2023

@desrosj is there any plan to update node / npm for Core as well?

@desrosj
Copy link
Contributor Author

desrosj commented Dec 21, 2023

@afercia the Core update was also coordinated and both code bases are now on Node.js 20. See the announcement post for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants