fix: correct engines.node to match vite/rolldown's actual requirement - #1103
Merged
Conversation
…irement (#1102) vite@8/rolldown require Node ^20.19.0 || >=22.12.0 (they use node:util's styleText, unavailable on earlier Node 20.x/21.x/22.x releases), but our engines field only said ">=20". That let unsupported versions like Node 21.0.0 pass the check and fail later with a cryptic rolldown SyntaxError instead of a clear engine mismatch. Also enable engine-strict in .npmrc so npm install fails fast with a clear message on an unsupported Node version.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the Node engine requirement to align with vite/rolldown’s actual minimum versions and enforces engine checks at install time, with the lockfile regenerated accordingly. Sequence diagram for npm install and build with strict Node engine enforcementsequenceDiagram
actor Developer
participant npm
participant Node
participant Project
Developer->>npm: npm install
npm->>Project: read package.json engines.node
npm->>Node: check version satisfies ^20.19.0 || >=22.12.0
alt compatible_version
npm-->>Developer: install succeeds
Developer->>npm: npm run build --workspace @svgedit/svgcanvas
npm->>Project: invoke vite/rolldown build
Project->>Node: use util.styleText
Node-->>Project: styleText available
else incompatible_version
npm-->>Developer: fail install with Unsupported engine (engine-strict)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
.npmrcwithengine-strict=trueonly affects npm, consider mirroring this constraint in other package managers’ configs (e.g., pnpm/Yarn) if they’re used in the project to avoid inconsistent behavior across tooling. - If any workspace packages are published or used independently, you may want to align their individual
engines.nodefields with the root setting so consumers see the same minimum Node requirements everywhere.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `.npmrc` with `engine-strict=true` only affects npm, consider mirroring this constraint in other package managers’ configs (e.g., pnpm/Yarn) if they’re used in the project to avoid inconsistent behavior across tooling.
- If any workspace packages are published or used independently, you may want to align their individual `engines.node` fields with the root setting so consumers see the same minimum Node requirements everywhere.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
engine-strict=true made npm ci enforce engines for every installed package, not just vite/rolldown. That surfaced a pre-existing, unrelated mismatch: open-cli@9.0.0 (used only by the open-docs-no-start script) requires Node >=22, while CI runs on node-version 20.x. Fixing that is out of scope for #1102, so drop engine-strict; the corrected engines.node range still fixes the original build failure on its own.
@svgedit/svgcanvas and @svgedit/react-test are both published independently (publishConfig.access: public), but neither declared its own engines field, so consumers installing them directly wouldn't see the Node version requirement. Add the same engines.node range as the root package.json to both. No yarn/pnpm config to mirror engine-strict in: the repo has no yarn.lock, pnpm-lock.yaml, or packageManager field, and no workflow references those tools — npm is the only package manager in use here.
Collaborator
Author
|
Addressed both points:
|
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
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
SyntaxError: The requested module 'node:util' does not provide an export named 'styleText'vite@8/rolldownrequire Node^20.19.0 || >=22.12.0(confirmed in their ownpackage.jsonenginesfields — they usenode:util'sstyleText, which isn't available on earlier Node 20.x, 21.x, or early 22.x releases). Our rootpackage.jsononly required>=20, so unsupported versions (e.g. Node 21.0.0, as in the reporter's Docker repro) passed the engine check and only failed later with a confusing low-level rolldown crash.engines.nodeto^20.19.0 || >=22.12.0and regenerated the lockfile to match..npmrcwithengine-strict=truesonpm installnow fails fast with a clear "Unsupported engine" message on an incompatible Node version, instead of letting the build fail later with a cryptic error.Test plan
npm installsucceeds on Node v24.18.0 (satisfies the new range)npm run build --workspace @svgedit/svgcanvasbuilds successfullyvite/rolldown's ownenginesfields innode_modulesto confirm the correct minimum version range🤖 Generated with Claude Code
Summary by Sourcery
Tighten Node.js engine requirements to align with vite/rolldown and enforce early failure on unsupported Node versions.
Build: