-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Install latest NPM for CI static checks #70844
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
base: trunk
Are you sure you want to change the base?
Conversation
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.
- 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 |
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.
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
.
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
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 At the time, we guarded against this with an upper limit defined in the |
To clarify, I am not saying this is the same situation. But it feels similar. Here are a few additional questions and thoughts:
In my opinion, if we are going to enforce that the lock file has no changes when run on npm |
@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 I worry about the practicality of assigning Some alternatives or extensions of this idea:
|
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 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 BackgroundThe 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 A previous effort spanning many months upgraded all branches still receiving security updates from Node versions as old as Today, 6.4 and newer all run version 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 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 SummaryWith all this in mind, I think that adjusting the Version 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. |
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.
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 |
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 |
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
afternpm 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.