chore: declare the Node and npm toolchain the build already assumes - #272
Open
escooterclinic wants to merge 1 commit into
Open
chore: declare the Node and npm toolchain the build already assumes#272escooterclinic wants to merge 1 commit into
escooterclinic wants to merge 1 commit into
Conversation
The repo pins Node 22.22 in four places — `node:22.22-alpine` in both Dockerfile stages, and `node-version: 22.22` in pr-validation.yml and e2e-tests.yml — but nothing declares it to a contributor or to tooling. There is no `engines`, no `packageManager` and no `.nvmrc`, so a clone gives whatever Node happens to be on PATH and the mismatch only surfaces later, in a build. `engines.node` here is not a preference: it is the strictest range the dependency tree already demands (`@nuxt/nitro-server@4.5.2` requires `^22.19.0 || ^24.11.0 || >=26.0.0`; @nuxt/vite-builder and the cssnano/postcss family are looser subsets of it). `engines.npm` matches the npm bundled with the pinned Node, 10.9.8, which is the npm that has to be able to consume the committed lockfile. `.nvmrc` matches the Dockerfile and CI exactly. The lockfile change is npm recording the same `engines` block in the root entry; no dependency versions move. This declares intent — it does not enforce it. `engines` only warns unless `engine-strict=true` is set in .npmrc, which this commit deliberately does not do. Signed-off-by: Patryk Radek <patryk@escooterclinic.co.uk>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe project now specifies Node.js 22.22 and declares supported Node.js and npm engine ranges. ChangesRuntime alignment
Estimated code review effort: 1 (Trivial) | ~3 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What: adds
enginesand.nvmrcdeclaring the Node and npm the build already assumes.Why: the repo pins Node 22.22 in four places —
Dockerfile(builder stage)node:22.22-alpineDockerfile(runner stage)node:22.22-alpine.github/workflows/pr-validation.ymlnode-version: 22.22.github/workflows/e2e-tests.ymlnode-version: 22.22— but nothing declares it. There is no
engines, nopackageManager, and no.nvmrc, soa fresh clone builds against whatever Node is on
PATH, and a mismatch only surfaceslater, inside a build.
engines.nodehere is not a preference. It is the strictest range the dependency treealready demands:
@nuxt/nitro-server@4.5.2andnuxt@4.5.2require^22.19.0 || ^24.11.0 || >=26.0.0;@nuxt/vite-builder@4.5.2(^22.18.0 || …) and thecssnano/postcss-*family (^22.11.0 || …) are looser subsets. On Node 25 — acurrent release someone could easily be running —
npm installemits 33EBADENGINEwarnings against this tree today, and nothing in the repo says Node 25 is out of scope.
engines.npmis the npm bundled with the pinned Node (10.9.8). That is the npm that hasto be able to consume the committed lockfile, and it is the one part of the toolchain
nothing anywhere records.
.nvmrcis22.22, matching the Dockerfile and both workflows exactly.Scope, honestly
This declares intent; it does not enforce it.
enginesonly warns unlessengine-strict=trueis set in.npmrc, which this PR deliberately does not do — that isa behaviour change for every contributor and should be your call, not a side effect of a
declaration. It would also not, on its own, have prevented an out-of-sync lockfile
reaching
main.If you would rather move the toolchain forward than pin it back, the same two files are
the right place — change
engines.npmto^11and the base image and workflows to a Nodethat ships npm 11. The point of the PR is that the version should be written down
somewhere, not that it should be 22.22.
Type of change
Validation
npm cistill resolves, andnpm install --package-lock-onlyafter the change produces only the 4-line
enginesblock in the lockfile root entry(included here). No dependency version moves.
verbatim; happy to add a "use Node 22.22 /
nvm use" line if you want it.runtime code touched.
DCO
Signed-off-by) viagit commit -sSummary by CodeRabbit