Skip to content

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 22, 2025

What?

Ensures CI static checks are run using the latest version of NPM.

Follow-up to #70839 (review)

Why?

Some of the static checks may behave differently in newer or older versions of NPM. Specifically, "local changes" checks for changes to package-lock.json after npm install, but changes may only be present on specific versions of NPM. We should default to running the latest version.

How?

Ideally, this could be configurable through the actions/setup-node action. Based on actions/setup-node#324, this does not appear to be possible, and must be a separate step.

Testing Instructions

Hopefully CI on this branch fails as a demonstration of the check, since I branched from 16302bd, intentionally one commit prior to #70839.

Some of the static checks may behave differently in newer or older versions of NPM. Specifically, "local changes" checks for changes to `package-lock.json` after `npm install`, but changes may only be present on specific versions of NPM. We should default to running the latest version.
@aduth aduth requested a review from sirreal July 22, 2025 14:55
@aduth aduth requested a review from desrosj as a code owner July 22, 2025 14:55
@aduth aduth added [Type] Enhancement A suggestion for improvement. Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jul 22, 2025
@aduth aduth removed the [Type] Enhancement A suggestion for improvement. label Jul 22, 2025
Comment on lines 46 to 48
- name: Npm install
# A "full" install is executed, since `npm ci` does not always exit
# with an error status code if the lock file is inaccurate. This also
Copy link
Member

Choose a reason for hiding this comment

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

Do you think we can also use npm ci here now? I think that would help catch issues. I'm looking at a recent trunk (7a1f29f) with the out-of-date lock file and npm ci does seem to exit with non-0 status, $? is 1 for me with npm 11.4.2.

Copy link

github-actions bot commented Jul 22, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: aduth <aduth@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>
Co-authored-by: desrosj <desrosj@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@desrosj
Copy link
Member

desrosj commented Jul 22, 2025

This is a tricky one. We dealt with a similar situation when the lock file format changed in npm v7. The change was both forward and backward compatible, however running npm install would result in the format flipping back and forth.

At the time, we guarded against this with an upper limit defined in the engines and being unable to use npm >= 7 was not supported. I couldn't find the related issues in this repository, bot Core-52951 and Core-56658 handled this on Trac.

@desrosj
Copy link
Member

desrosj commented Jul 22, 2025

To clarify, I am not saying this is the same situation. But it feels similar.

Here are a few additional questions and thoughts:

  • Do we know what changed in npm that is resulting in differences? That's not immediately clear here. Are we positive this is the culprit and not a dedupe, or something similar?
  • Are they forward/backward compatible? Will someone on npm 10 see the changes revert themselves?
  • It looks like 11.4.2 is currently bundled with Node.js 24.4.1. 24.x is the current release, but 22.x is the Active LTS, which is usually what we prefer contributors to use for consistency. The latest release of that version (currently 22.17.1) bundles npm 10.9.2.
  • Gutenberg and Core currently only require 20.x, which is a maintenance LTS until April 2026. It is ideal to always run an active LTS, but not always realistic because of the level of effort it sometimes takes to upgrade, and occasionally a lack of proper support for a new version for a required dependency.
  • Running a current release of Node.js has other challenges. Mainly, many dependencies wait for a version to be declared LTS before updating for compatibility. This could potentially result in some problems.

In my opinion, if we are going to enforce that the lock file has no changes when run on npm 11.4.x, then the engines and .nvmrc file should be updated to reflect this. Otherwise, we should add an upper boundary until it's properly supported to provide a consistent contributor experience (these nuances are not obvious or easily understood).

@aduth
Copy link
Member Author

aduth commented Jul 22, 2025

@desrosj Thanks for jumping into the conversation. These are all valid and insightful considerations.

I am fairly certain that this is the result of a version change and not a duplication, and this can be confirmed by checking out an older version and simply running npm install with the latest version of NPM. To your point, I checked now to confirm that older versions will not revert this (at least 10.x).

I worry about the practicality of assigning engines, both in terms of keeping it up-to-date, as well as the practical effects of intentionally requiring contributors to run outdated versions of NPM (conflicts between projects, missing improvements and bug fixes). On the other hand, I see the value in being explicit to a fixed version that we align around.

Some alternatives or extensions of this idea:

  • We could agree that the artifact produced by the latest version of NPM is the correct one
    • Pro: There's a definitive answer when inconsistencies arise across versions
    • Pro: We're always using the latest version, incorporating bug fixes and enhancements
    • Con: It would be impractical to codify this via engines, at least without the burden of constantly updating. And without engines, it's hard for a developer to ensure they're running the expected version.
  • We could align the expected version with what's included in the LTS release of Node.js
    • Pro: We'd have clear guidance around which version is expected, and the "correct" one in terms of expected output
    • Pro: More practical to specify this in engines, and we may be able to do this habitually as part of the existing routine of shifting to the latest LTS version
    • Con: We'd have to accept that we're using an older version which may behave inconsistently from the latest version

@desrosj
Copy link
Member

desrosj commented Aug 6, 2025

With the first alternative that you mentioned, would that result in code being present in the artifacts but not committed to version control?

For your second alternative, I'd argue that the Con is actually a Pro. Stability is a feature. Because the Current version can introduce new features and make implementation changes, it's likely not as stable as we need. Expecting relatively the same version of npm as what is bundled with the current preferred version of Node.js can also make things easier. By adding an upper limit, it ensures consistent behavior for all contributors. It's also only temporary until we can update.

When I was thinking this one through again this morning. I was considering the more obvious path: What if we all agreed that we should be running the Current version and just upgraded to 24? I don't know that it's realistic, but I wanted to brain dump to have all the information in one place so anyone that interested can weigh in (I'm sure both of you are aware of most of this already). This is stretching past the point of this pull request at this point, but it's important context.

Background

The project has historically used the Active LTS because updating the version of Node.js regularly has proven challenging. It requires:

The last item has a particularly high threshold to meet. In order to installing the new version, there needs to be a compelling argument to justify the added maintenance burden (all versions of Node.js installed on the build server must remain indefinitely because it's possible that older branches could still make use of them). This usually is a new feature that is blocking a current priority on the roadmap. I'm also not sure if there is a willingness to install non-LTS versions there. I would have to ask the systems team.

Another consideration of this is that older branches of the wordpress-develop and gutenberg repositories will likely be running different versions of Node.js. This can increase the maintenance burden and make releasing security updates in older versions more challenging. As much as possible, we should aim to use as few versions as possible across branches receiving security updates.

A previous effort spanning many months upgraded all branches still receiving security updates from Node versions as old as 0.10.48 (😱) to the same version. As a result, branches 5.6-3.7 (3.7-4.6 no longer receive security updates) all utilize version 14 (see Core-52341). To accomplish this, many of the dependencies in older branches needed to be updated. This resulted in massive code churn for most branches due to packages related to JavaScript bundling and minification changing all .js files.

Today, 6.4 and newer all run version 20, and 6.3 and earlier all run 14. 6.4 briefly used 16 (see Core-56658), but the update to 20 was backported when applied during the 6.5 cycle through Core-59663 (related dev notes).

So is using 24 possible?

To test whether using 24.x is even possible, I've created WordPress/wordpress-develop#9395 and #71091 to confirm the build processes work as expected on both versions 22 and 24. I've also confirmed that there are no unsupported engine warnings from dependencies in either repository.

So yes, it does seem that it's possible to use 24 without issue. However, we would still have the last bullet point above of getting 24.x installed on the build server. And I'm not sure if there are any compelling reasons why it should be at this time.

What I have not looked at is the level of effort, impact, and how practical updating the 6.4-6.8 branches to also use 24 is.

Summary

With all this in mind, I think that adjusting the engines fields for now to expect npm<11.0.0 and node<24.0.0 for the time being would be the best option.

Version 24 is set to enter Active LTS later this year on Octover 28th, but that's too late in the 6.9 cycle (beta 1 is due October 21st). I think we should begin to plan for upgrading to 24 during the 7.0 cycle, which would make the upper bound only required for a few months.

I also plan to find somewhere in the Core Handbook for the information above to live as a reference that's easier to find.

I'm going to request a review from few other people that have helped with managing Node.js versions and requirements in the past.

@aduth
Copy link
Member Author

aduth commented Aug 12, 2025

Thanks for the added context, @desrosj . I think I'm less fixated on this idea of using the "latest" version and instead pinning consistently based on what our current target version is (LTS), if that helps make things more predictable.

the added maintenance burden (all versions of Node.js installed on the build server must remain indefinitely because it's possible that older branches could still make use of them)

With what you're suggesting, if we were to update branches 6.4-6.8 to use v24, would we still need to have Node.js v20 installed indefinitely? I'm wondering if this is something where we can continue to bump previous branches as we target a new version. To your point about level of effort though, and based on how much went into Core-52341, it might be a big ask. Just trying to think through what's practical for a routine moving forward (every LTS, every other LTS, etc.).

Another potential problem is that there could be a mismatch between the version installed by .nvmrc as the major LTS release version compared to what's installed on the build server, where the version of NPM can change over the course of an LTS's lifetime. For example, in theory a future release of Node.js v20 could include a version of NPM >=11.0.0.

@desrosj
Copy link
Member

desrosj commented Aug 14, 2025

For example, in theory a future release of Node.js v20 could include a version of NPM >=11.0.0.

Has this ever happened before? I have been operating under the assumption that the major (and sometimes minor) version of npm included in an even-numbered version of Node.js is locked once it reaches the Active LTS state.

@aduth
Copy link
Member Author

aduth commented Aug 14, 2025

For example, in theory a future release of Node.js v20 could include a version of NPM >=11.0.0.

Has this ever happened before? I have been operating under the assumption that the major (and sometimes minor) version of npm included in an even-numbered version of Node.js is locked once it reaches the Active LTS state.

Yes, it happened in Node.js v20, which started with NPM v9.6.4, but then shifted to NPM v10 as of Node.js v20.7.0.

It also happened in Node.js v16, which started with NPM v7.10.0, and then upgraded to NPM v8 in Node.js v16.10.0

Source: https://nodejs.org/dist/index.json

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants