-
Notifications
You must be signed in to change notification settings - Fork 955
Add engines #16906
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: main
Are you sure you want to change the base?
Add engines #16906
Conversation
This should make dependabot run similar versions so we don't end up with mismatched package.json updates
82482f9 to
0eac3cd
Compare
janbrasna
left a comment
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 all for moving the stack to Node 24 officially, but it may need harmonizing across more surfaces:
| "description": "Making mozilla.org awesome, one pebble at a time", | ||
| "private": true, | ||
| "engines": { | ||
| "node": "^24.0.0", |
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.
Bedrock currently runs on Node 20:
Line 25 in 4b47277
| FROM node:20.14.0-slim AS assets |
|
@maureenlholland Shall we move the entire farm to Node 24? I'm up for that if you and @stephaniehobson are OK with it. 24 is LTS so makes sense |
|
@stevejalim (ah, sorry, mixed up my pings!) yeah, I'm testing it out for now. Our last update was summer 2024, so it feels like a good maintenance upgrade |
Uses bookworm slim docker image
d99c9f2 to
dfa4fe9
Compare
janbrasna
left a comment
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.
In general this doesn't need any Node upgrade, and actually a Node 20 + npm 10 env would be just more stable. It's npm 11.5–11.6 bugs, combined with Dependabot adding support for it few weeks back.
"Peer fields were added in Dependabot package-lock update […] These fields were being removed in local npm install steps."
I believe that's a bug. Not sure if the keys should be there or not, but I'd err on the side of the past npm versions, and see the recent additions as a bug, and hopefully these would get removed over time with subsequent updates of Dependabot's npm like in a local setup.
They had a bug around 11.5 that was approached later not by reverting, but moving further around 11.6 looking like the resulting lockfiles are then wrapped differently between these…? These were added in 11.6.1 to fix a recent bug, but then 11.6.2 did a different fix and 11.6.3 reverted a lot of things back taking a different angle, with 11.6.4 still fixing some bugs from that exercise. So hopefully, the lockfiles for <11.5 and ≥11.6.4 should keep stable. It's just the spotty range that now Dependabot uses as a default, that yields these extra arborist values.
This was fine before npm 11.5 and before dependabot-core moved from npm 10.9.x to 11.6.x (incl. introducing a handful of bugs re. parsing semver constraints) — every patch since has a different behavior but unfortunately dependabot chose to preinstall a version right in the middle of this mess, at least a version before the peer key bug was fixed, so the dirty lockfile is a bug on their side — the local result should still be the right one.
More importantly, the results could differ between npm ci and npm install — or between a first and a second run of npm install in a row of the same project.
Normally engines won't be able to select any runtimes but Dependabot seems to take the values as a strict constraint.
So we could use the engines just temporarily, to avoid those buggy ranges for now, with possible removal at some point later?
| # assets builder and dev server | ||
| # | ||
| FROM node:20.14.0-slim AS assets | ||
| FROM node:24-slim AS assets |
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've written up some past decisions in #16910 for consideration.
(My personal take would be… try major only for now, and constrain more explicitly if any woes appear later — being expected more on developers' ARM machines, not the Linux build pipelines.)
| "private": true, | ||
| "engines": { | ||
| "node": "^24.0.0", | ||
| "npm": "^11.6.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.
Well, because JSON… as, who needs comments right?, JSON doesn't allow comments — is there a way to have some rationale about this somewhat/somewhere? I mean, the engines in this case is not deliberate, the stack works happily on versions years back — this is now here just a stopgap for time being, to hopefully try to trick dependabot-core into using non-buggy npm cli for their update PRs.
Because generally, this does not enforce any version of anything, either in CI or locally, right? (Only Dependabot decided to parse it, and use that as a hard semver constraint) — so folks still need to know the reasoning behind this and be running either npm 10.x or 11.5- … or 11.6.4+ containing the fixes already locally themselves actually — as this won't error out in any way if they use any earlier versions as their runtime, right?
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.
Also, given the last bugs were ironed out only in later versions, would:
| "npm": "^11.6.1" | |
| "npm": "^11.6.4" |
make Dependabot download and install this newer version in its runtime? Maybe worth trying.
It ends up being logged, e.g. good lockfiles here:
- /actions/runs/18989653611 10.9.3
Dirty lockfiles here:
- /actions/runs/19718667693 11.6.2
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.
Seems there's more affected by that general breakage, incl. Claas/MDN also having issues on their deps:
- [BC BREAK] version 11.6.2 breaks CI npm/cli#8669 (comment)
- [npm] Dependabot removes unrelated nested devDependency dependabot/dependabot-core#13652 (comment)
… so hopefully this is not the new norm now, and will return to what it used to be once bugs are squashed and bots use current versions.
|
Probably won't have time to get back to this today, will have to return to it next week. Skimming the comments, my gut reactions are:
|
|
I see it as a temporary measure to try to force Dependabot out of the bugged version range, so it doesn't matter too much. Ideally this could be removed in a few weeks (or months, based on how quickly they update the version they use by default) — at least let's see if the higher version than what they normally provide a) works and installs in the container; b) removes the peer mess back to what it used to look like. |
If this changeset needs to go into the FXC codebase, please add the
WMO and FXClabel.One-line summary
Pin npm and node versions to ensure consistent updates with dependabot
Significant changes and points to review
Major node version update from 20-24 (our last major update was here: https://github.com/mozilla/bedrock/pull/13939/files)
Issue / Bugzilla link
Peer fields were added in Dependabot package-lock update: #16897
These fields were being removed in local
npm installsteps. This is likely due to a mismatch in npm versions (although it is hard to diagnose as the peer field is not documented in npm, yet it is in npm GitHub PRs)Testing
Run
npm installlocally and confirm the lock file does not changeNode upgrade tests: confirm the following run without error or noise
make buildmake runIntegration test run 🟢 : https://github.com/mozilla/bedrock/actions/runs/19857946405