fix(bundle): declare Node >=20 and install the shipped lockfile with npm ci - #191
Conversation
|
There was a problem hiding this comment.
🟡 Changes recommended
The consumer-facing behavior change is not recorded in the empty ## [Unreleased] section of CHANGELOG.md as required by CONTRIBUTING.md.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR corrects two inconsistencies in the shipped release bundle manifest (resolving #162). It raises the declared Node engine floor in the bundled template/package.json from >=16 to >=20 to match the repo's actual runtime requirements and CONTRIBUTING.md, and changes the consumer install command in template.yml from npm install to npm ci so installs from the release asset (which ships a package-lock.json) are reproducible and don't mutate the cached lockfile. A comment documents why npm ci is safe despite failing without a lockfile.
Changes:
- Bump
engines.nodein the bundledtemplate/package.jsonto>=20. - Switch
build.installintemplate.ymltonpm ciwith an explanatory comment.
File summaries
| File | Description |
|---|---|
| template/package.json | Raises declared Node floor to >=20, aligning the shipped bundle with the root package, .nvmrc, and CONTRIBUTING. |
| template.yml | Uses npm ci for reproducible consumer installs against the shipped lockfile, with a rationale comment. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
🎭 Visual regression results
Details
Skipped testsmobile-chrome › theme.spec.ts › QuantEcon theme — visual regression › without JavaScript › drawer-opens-without-javascript |
Copilot review on #191: CONTRIBUTING.md requires every consumer-facing change under Unreleased. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…npm ci The release bundle's package.json declared engines.node ">=16" while CONTRIBUTING.md cites that field as evidence for a Node 20 floor; 20 is the honest number (.nvmrc pins 24, the root package.json requires 20, the rendering engine's floor is above 16). And template.yml ran `npm install` although the release asset ships a lockfile (release.yml generates it and `files` lists it), so consumer installs were neither reproducible nor lockfile-preserving. `npm ci` fails without a lockfile, but the only lockfile-less path is a source checkout, which is not a usable template until `make build-theme` has run, and that target installs on its own (#162). The report theme carries the same two lines; its copy of the engines floor was corrected in QuantEcon/quantecon-theme-report.mystmd#12 and the install decision is QuantEcon/quantecon-theme-report.mystmd#13. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Copilot review on #191: CONTRIBUTING.md requires every consumer-facing change under Unreleased. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
a6f04b8 to
3382c4c
Compare
Copilot review on #191: CONTRIBUTING.md requires every consumer-facing change under Unreleased. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3382c4c to
872b7fc
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The three-file, config-only change is fully verified against the release workflow, Makefile, and repo Node floors, with all inline-comment claims confirmed and the prior changelog feedback addressed.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Both halves of #162, one PR since both lines are part of the same bundle manifest.
Engines floor.
template/package.jsondeclaredengines.nodeas>=16whileCONTRIBUTING.mdcites that field as the evidence for a Node 20 floor. Twenty is the honest number:.nvmrcpins 24, the rootpackage.jsonrequires 20 or newer, and the rendering engine's floor is above 16 anyway. The report theme's copy was already corrected to>=20in QuantEcon/quantecon-theme-report.mystmd#12, so this brings the two back in step.Install command.
template.ymlrannpm installalthough the release asset ships a lockfile (release.ymlgenerates it withnpm install --package-lock-onlyandfileslists it). It now runsnpm ci, which makes consumer installs reproducible and stops the lockfile being mutated in a consumer's template cache. On the caveat #162 raises:npm cifails outright without a lockfile, but the only lockfile-less path is a source checkout of this repo, which is not a usable template untilmake build-themehas produced the bundle, and that target runs its ownnpm installin.deploy/. The reasoning is recorded as a comment next to the line so nobody re-litigates it from the manifest alone. The report theme's identical decision is QuantEcon/quantecon-theme-report.mystmd#13 and should move the same way.Not tested end to end against a consumer install from a release asset; that happens on the next tag. The CI visual jobs exercise
.deploy/quantecon-themevia the Makefile path, which this change does not touch.Closes #162.
🤖 Generated with Claude Code