Skip to content

Add Node/NPM engines, update Node across envs#1180

Draft
maureenlholland wants to merge 3 commits intomainfrom
update-npm-breaking-changes
Draft

Add Node/NPM engines, update Node across envs#1180
maureenlholland wants to merge 3 commits intomainfrom
update-npm-breaking-changes

Conversation

@maureenlholland
Copy link
Copy Markdown
Collaborator

@maureenlholland maureenlholland commented Mar 23, 2026

One-line summary

Significant changes and points to review

Issue / Bugzilla link

Testing

These package updates are related to Sass but they are appearing in
all dependabot PRs. Sass is currently pinned so we are going to ignore
these optional dependencies as well for now.
This should help avoid the package discrepancies we can get between
a developer npm install and dependabot updates. This won't enforce
specific engines for a developer, but will provide a warning. This
should enforce dependabot to use the specified versions, which will
help with debugging any unexpected changes in dependabot PRs.

Node 20 is also reaching EOL, so we are due for an update across envs
here.
stevejalim
stevejalim previously approved these changes Mar 23, 2026
@maureenlholland maureenlholland added the Do Not Merge Don't do it. label Mar 23, 2026
@maureenlholland
Copy link
Copy Markdown
Collaborator Author

following up on this node/npm upgrade PR: https://github.com/mozmeao/springfield/pull/808/changes

it seems worthwhile to add now, as the webpack and css minimizer deps are requiring a certain node version, and we continue to have issues with unexpected package-lock changes in dependabot updates

@maureenlholland maureenlholland changed the title Update dependencies Add Node/NPM engines, update Node across envs Mar 23, 2026
@maureenlholland maureenlholland dismissed stevejalim’s stale review March 23, 2026 15:37

scope of PR changed

@janbrasna
Copy link
Copy Markdown
Collaborator

I'd suggest trying without specific engines now. The reasons for the constraints proposed originally no longer apply (in CI; and has no effect locally).

@maureenlholland
Copy link
Copy Markdown
Collaborator Author

@janbrasna I think it would be helpful for debugging purposes and consistency. It would be nice to indicate that these are the versions used by CI & dependabot. If devs are not using those versions, that's the first place to look for bugs.

we have some PRs that remove dependencies & others that re-add them and I think this is based on differing npm versions: https://github.com/mozmeao/springfield/pull/1175/changes

some of our dependencies are also requiring certain node versions with their next updates (i.e. css minimizer & webpack)
Screenshot 2026-03-23 at 4 30 02 PM

@janbrasna
Copy link
Copy Markdown
Collaborator

https://docs.npmjs.com/cli/v11/using-npm/config#engine-strict That would only fail hard when npm install --engine-strict (or .nvmrc entry) — but then any incompatible engines on the actual deps would yield the same.

(Dependabot is a little weird in a way it parses that definition and reinstalls the runtime based on that — but that's not what the builder image or local dev env would do.)

I understand the utility of using some definition as a form of a reminder what the current stack expects. If the intent is enforcement though, the current project tooling has no say in the env it runs in (and either provides a correct container, or expects whatever developers have locally is compatible), and also does not enforce the engines mismatch to fail hard in make targets — mostly for its potential to stall builds, incl. production deploys.

It might be feasible to add the --engine-strict in Makefile for local operations to bark at developers, but keep the CI warn-only, to not introduce any new failure points, but I haven't looked into that split if feasible (e.g. if preflight only ever used locally, and never from automation etc.)

What might be interesting from maintenance perspective is https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file for the setup-node actions — to point it at the package engines, and never specify it manually ever again, if going that route.

@maureenlholland
Copy link
Copy Markdown
Collaborator Author

maureenlholland commented Mar 24, 2026

https://github.com/actions/setup-node/blob/main/docs/advanced-usage.md#node-version-file for the setup-node actions — to point it at the package engines, and never specify it manually ever again, if going that route.

^ This is interesting, thanks for sharing the link. Not to having to specify the same node version in multiple places sounds like a win.

My main intent is to have fewer WTF moments when reviewing dependabot PRs. We've had a few instances now where peer and optional deps are treated differently between developer PRs and dependabot PRs. I don't know if I want to enforce strict versions on developer env, but I would like an easy way to resolve those conflicts (i.e. if I'm running the package-specified engines and see the same dependabot changes, those are expected and fine to merge)

@janbrasna
Copy link
Copy Markdown
Collaborator

Yeah I remember where this comes from, luckily the npm bugs are long resolved now and dependabot also no longer runs these funky versions, so this should not be happening unless a developer is stuck on one of these few unfortunate versions. Updating npm if the cli says so is a good way. Both 10.x and 11.6/.7/.8 and newer branches are all alright if updated to latest patch version — and more importantly should be totally compatible with their dependency resolution between them. I was just hinting at the reality npm tooling has no mechanism to select runtimes, only a plethora of options to enumerate compatibility mostly for downstream consumers, and several ways to fail with different verbosity on these conditions unmet.

As this doesn't seem like something limiting the actual use of the project (engines), but more working on it (devEngines).

Basically something capturing "don't use npm 11.5 or 11.6 if you're upgrading deps, or you'd clutter the lock file" — luckily all the tools in CI (as proposed in this PR) are way past that, and npm itself is currently at 11.12 so my only question is — if this was to be a temporary guardrail, how long it should stay in; how relevant that is over time?

I was about to suggest devEngines.runtime instead, as that's now also supported: actions/setup-node@v6.3.0 — but given a suspect corepack bug dependabot fails on that currently if not pinned exactly to the last patch version — and nobody wants to maintain hard pins on that:( So that's a nice theory, but a little far away for now. But that has the goodies: specify version ranges for developers working on it, CI, tests etc. with a visible configuration how loud should the stack fail if not met etc. — but more importantly, it does not bark at just consumers, which e.g. the prod build pipeline is. (Wish it were this easy;D)

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

Labels

Do Not Merge Don't do it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants