Skip to content

Conversation

@maureenlholland
Copy link
Collaborator

@maureenlholland maureenlholland commented Dec 1, 2025

If this changeset needs to go into the FXC codebase, please add the WMO and FXC label.

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 install steps. 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 install locally and confirm the lock file does not change

Node upgrade tests: confirm the following run without error or noise

  • make build
  • make run

Integration test run 🟢 : https://github.com/mozilla/bedrock/actions/runs/19857946405

This should make dependabot run similar versions so we don't end up
with mismatched package.json updates
Copy link
Collaborator

@janbrasna janbrasna left a 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",
Copy link
Collaborator

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:

FROM node:20.14.0-slim AS assets

@stevejalim
Copy link
Contributor

@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

@maureenlholland
Copy link
Collaborator Author

maureenlholland commented Dec 2, 2025

@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
Copy link
Collaborator

@janbrasna janbrasna left a 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
Copy link
Collaborator

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"
Copy link
Collaborator

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?

Copy link
Collaborator

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:

Suggested change
"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:

Dirty lockfiles here:

Copy link
Collaborator

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:

… 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.

@maureenlholland
Copy link
Collaborator Author

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:

  • we can drop node out of engines field, since only npm version seems related to dependabot, can update to suggested ^11.6.4 npm version
  • we're due for a node upgrade but it doesn't have to be in this PR

@janbrasna
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants