feat(cli): support devEngines for Node.js runtime and package manager selection#1760
Merged
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
Contributor
|
✅ Staging deployment successful! Preview: https://viteplus-staging.void.app/ |
This comment was marked as outdated.
This comment was marked as outdated.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
fengmk2
added
test: e2e
This comment was marked as outdated.
This comment was marked as outdated.
…ction Implement rfcs/dev-engines.md with a compatibility-first rule: existing .node-version and packageManager sources keep winning for writes, while devEngines.runtime and devEngines.packageManager become the recommended default for new projects. Conflicts are surfaced by vp env doctor (semver-aware), never silently resolved. - Parse devEngines per the OpenJS spec (single/array shapes, optional version, onFail with positional defaults, lenient on malformed entries) - Detect devEngines.packageManager between the packageManager field and lockfiles; resolve version ranges (cached-first, then registry); never freeze a range into an exact pin - Auto-pin lockfile-detected package managers into devEngines.packageManager instead of the packageManager field (vp migrate included) - Move devEngines.runtime above engines.node in Node.js version resolution - vp env pin/unpin write devEngines.runtime when no .node-version exists, with a --target override; existing engines.node is never modified - vp env doctor gains a devEngines section with semver-aware conflict checks Closes #864
Mirror the test matrix from npm/npm-install-checks test/check-dev-engines.js, mapped to Vite+ semantics: npm validates the current environment and throws on malformed input, while Vite+ provisions the environment and reads devEngines leniently (rfcs/dev-engines.md), so npm's throw cases pin down our skip/treat-as-default behavior for the same inputs. - vite_shared: empty arrays, non-object devEngines/fields, non-string name/version/onFail values, extra entry properties, unrecognized sub-fields - vite_install: empty-array fallthrough, all-unsupported array onFail handling, first-supported-entry selection; extract dev_engines_package_manager_conflict_message() for direct unit tests - vite_js_runtime: runtime array form, no-node-entry and name-only fallthrough to engines.node - vite_global_cli: extract collect_dev_engines_findings() and cover all doctor findings (conflicts, spec violations, notes, all-satisfied)
Auto-pin replaced the whole devEngines.packageManager value, dropping alternate entries the spec allows (e.g. other package managers declared with onFail: ignore) when detection fell through to a lockfile. Preserve them instead: append to an existing array, convert an existing single entry to array form, and only write a single entry when the field is absent or malformed. Also regenerate the create-flow snapshots (new-vite-monorepo, new-vite-monorepo-bun, create-org-bundled) that were left stale after setPackageManager() was retargeted to devEngines.packageManager, which broke the CLI snap test and E2E jobs in CI.
- Guard against spreading a malformed non-object devEngines value in setPackageManager, which corrupted package.json with numeric index keys - Make vp env doctor packageManager checks read the workspace root package.json (the file vp install actually uses) instead of the nearest one, fixing wrong or missing diagnostics in monorepos - Fetch the npm abbreviated metadata document (KBs) instead of the full packument (multi-MB) when resolving devEngines version ranges - Allow prerelease versions when a range explicitly requests them (e.g. ^12.0.0-0) and no stable version satisfies it - Widen the snap normalizer regex to cover prerelease identifiers with hyphens and build metadata - Extract shared confirm-overwrite and success-message helpers in vp env pin; pin --force with an identical version now no-ops as Already pinned - Render uncached devEngines ranges without the v prefix in vp --version
- The doctor .node-version vs devEngines.runtime conflict check now follows the resolution walk on both sides: it fires only when a .node-version actually wins resolution, and finds the devEngines.runtime declaration in ancestor manifests too. This also removes a false positive where a parent .node-version was flagged against a nearer devEngines.runtime that wins resolution - vp env pin (show) now reports inherited devEngines.runtime pins from parent directories, matching the existing .node-version inheritance
fengmk2
changed the title
fengmk2
changed the title
Member
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e3f4ba0. Configure here.
- Replace the planned 1-hour TTL range-resolution cache with the actual behavior: cached-satisfying-version short-circuits, registry consulted only while nothing satisfying is downloaded (abbreviated metadata) - Document the prerelease rule: excluded unless the range itself contains a prerelease marker and no stable version satisfies it - Document that auto-pin preserves existing devEngines.packageManager entries (append to array, convert single entry to array form) - Document doctor scoping: runtime checks are resolution-aware and walk ancestor manifests; package-manager checks use the workspace root - Document inherited devEngines.runtime pins in vp env pin output and the already-pinned no-op
Quality-only cleanup from /simplify, no behavior change:
- Add vite_shared::PackageJson::dev_engines_runtime() accessor and use it
in the four readers (runtime.rs, config.rs, js_executor.rs, pin.rs,
doctor.rs) that re-implemented the same dev_engines.runtime[node] chain
- Add vite_shared::dev_engine_entry() factory for the canonical
{name, version, onFail: download} shape, used by pin and auto-pin
- Add PackageManagerType::from_name() and use it for the doctor
supported-package-manager check instead of a parallel SUPPORTED const
- Remove the unused OnFail and PackageManagerSource Display impls
- Extract read_workspace_root_doc() to flatten doctor's nested match
- Collapse the duplicated unsupported-package-manager warn/note message
- Drop the node_version_exists param threaded into pin_dev_engines;
emit the shadowing warning at the call site
find_cached_package_manager_version considered an install complete when only the plain bin existed, while download_package_manager (and the bun path) require the plain bin plus the .cmd and .ps1 shims. Range resolution could therefore select a partially written cache entry that the download path would re-do, which is most likely to bite on Windows where the .cmd/.ps1 wrappers are the ones actually invoked. Extract is_package_manager_install_complete() as the single source of truth and use it in all three places. Add cross-platform tests covering a partial install being skipped, a partial-only cache resolving to None, and the completeness predicate itself.
The .cmd/.ps1 wrappers are only invoked on Windows, so checking them on other platforms wastes two stat calls per cache entry (and per download fast-path). Gate those two checks behind cfg(windows); the plain bin is still required everywhere. Because the check is shared, the cache and download fast-path still agree on every platform, now with a platform-accurate notion of complete: off Windows a bin-only install is usable, on Windows the wrappers are required. Tests updated to assert the per-platform behavior.
Quality-only cleanup from /simplify, no behavior change:
- find_cached_package_manager_version: compare the in-memory version
against the running best before stat'ing the install, so the filesystem
check is skipped for versions that cannot win (saves stats on the shim
hot path, more when read_dir yields the highest version early)
- Route both download fast-paths' post-lock re-checks through
is_package_manager_install_complete instead of a bare plain-bin stat, so
the function honors its own single-source-of-truth contract. Equivalent
in practice: create_shim_files runs under the lock and writes all shims
together, and off Windows the helper checks only the plain bin
- Drop the now-dead bin_file locals and inline install_dir.join("bin")
at the create_shim_files call sites
onFail is parsed, preserved, and validated, but its full behavioral matrix is not implemented in this PR. Correct the docs to say so: - RFC 2.3: mark the runtime onFail matrix as not-yet-acted-on (managed mode always downloads); keep it as documented future behavior - RFC 3.2: packageManager onFail is acted on only when no array entry names a supported package manager; a selected entry's onFail does not yet drive fallback on resolve/download failure - Spec compliance matrix + Phase 3 plan: mark onFail as partial/deferred - Add a Deferred / Future Work section laying out the per-entry packageManager fallback and the runtime onFail matrix - package-manager-detection.md and docs/guide/install.md: same narrowing No code change; onFail behavior is unchanged.
/simplify on the docs changes: - env.md Project Setup: drop the parenthetical that duplicated the Manage-section vp env pin description - install.md: drop the obscure array-of-all-unsupported onFail edge case (RFC-level detail); keep the user-facing 'only download is differentiated' caveat
Member
Author
|
@cursor review |
Member
Author
|
@codex review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8ce926e. Configure here.
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
resolve_latest_satisfying_version used version_req.contains('-') to decide
whether to allow prerelease versions when no stable version satisfies the
range. An npm hyphen range like "1.0.0 - 2.0.0" also contains '-', so it
would wrongly enable the prerelease fallback and could select a prerelease
the user never asked for.
Replace the substring check with requirement_requests_prerelease, which
splits on whitespace and only counts a hyphen embedded in a comparator
token (e.g. ^1.0.0-rc), not the lone ' - ' hyphen-range separator.
cpojer
approved these changes
Jun 8, 2026
vp env doctor had no snap coverage (only unit tests on collect_dev_engines_findings, which bypass the live command). doctor's output is mostly environment-dependent, so this follows the package-manager-diagnostics pattern: it sets up a project whose .node-version conflicts with devEngines.runtime and whose packageManager conflicts with devEngines.packageManager, runs vp env doctor, and asserts the two conflict warnings render (via a node script, not a raw snapshot of the volatile sections). Covers the render + resolution-gating path the unit tests skip.
The previous step redirected doctor output to a file and only printed a confirmation string, so the snapshot never captured the actual command output and a wording/format change would be invisible in the snap diff. Now the node script strips ANSI, extracts just the deterministic devEngines section (the other doctor sections are environment-dependent), and prints it, so snap.txt is a real golden file of the rendered findings.
…gines.runtime coexistence Clarify in the RFC that compatible coexistence of the two Node-version sources is intentionally not flagged (not even as a note): they serve different tool ecosystems (.node-version for fnm/nvm/CI, devEngines.runtime for npm/pnpm), so keeping both is a legitimate interop pattern. Doctor warns only when they actually diverge, which also catches later drift.
…urce The Source line preferred source_path (the package.json path) and dropped the descriptor, so a version resolved from engines.node and one from devEngines.runtime both rendered as <cwd>/package.json, indistinguishable. Append the field name when the source is a package.json field, matching vp env pin's output: 'package.json (devEngines.runtime)' vs 'package.json (engines.node)'. .node-version and pathless sources are unchanged. Extracted format_version_source as a tested helper.
vp env --help renders a hand-written HelpDoc in help.rs, separate from clap's derived help, so the cli.rs doc-comment updates only fixed vp env pin --help. The env subcommand list still described pin as 'creates .node-version' and unpin as 'Remove the .node-version file', which no longer matches the compatibility-first write target (.node-version or devEngines.runtime). Update both summaries and regenerate the cli-helper-message snapshot.
The test hardcoded Unix-absolute paths (/proj/package.json), which AbsolutePathBuf::new rejects on Windows (no drive letter), panicking on unwrap. Build the paths from a TempDir and assert against each path's own display string so the test holds on every platform.
fengmk2
commented
Jun 8, 2026
The user runs 'vp env pin <version>' explicitly, so pressing Enter at the overwrite confirmation should accept. Change the prompt to (Y/n) and cancel only on an explicit no instead of requiring an explicit y.
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
2 participants
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.
Implements rfcs/dev-engines.md (included in this PR), with a compatibility-first rule: existing
.node-versionandpackageManagersources keep winning,devEnginesbecomes the default for new projects, and conflicts are surfaced byvp env doctorinstead of silently resolved.Changes
devEngines.packageManagerbetween thepackageManagerfield and lockfiles; version ranges resolve against downloaded versions first, then the registry, and are never frozen into an exact pinvp migrate) writesdevEngines.packageManagerinstead of thepackageManagerfielddevEngines.runtimenow ranks aboveengines.nodefor Node.js version resolutionvp env pin/unpintargetdevEngines.runtimewhen no.node-versionexists, with a--targetoverride; an existingengines.nodeis never modifiedvp env doctorgains a devEngines section with semver-aware conflict checksValidation
just check,just test, crate-scopedcargo clippy --deny warnings,vp check, andpnpm test:unitall pass. Snap diffs reviewed: thepackageManagertodevEngines.packageManagerswap plus deduplicated invalid-engines warnings.Follow-ups
devEngines.runtimealongside the keptengines.nodevp migrate:.nvmrc/ Volta pins todevEngines.runtimewith alias-to-semver conversionCloses #864
Note
Medium Risk
Changes core Node/PM resolution order and mutates package.json on pin and auto-detect; behavior shifts for projects that relied on engines.node beating devEngines or on packageManager auto-writes.
Overview
Implements devEngines per
rfcs/dev-engines.mdwith a compatibility-first rule: existing.node-versionand top-levelpackageManagerstill win when present; new or inferred pins favordevEngines.Node.js resolution now prefers
devEngines.runtimeoverengines.node(after.node-version).vp env pin/unpincan write or remove pins inpackage.json#devEngines.runtimewhen there is no.node-version(or via--target), with optional sync prompts when.node-versiondiverges from a declared runtime range.engines.nodeis never modified by pin.Package manager detection reads
devEngines.packageManagerafterpackageManagerand before lockfiles, resolves semver ranges via cached installs then npm abbreviated metadata, and auto-pin from lockfile/default detection writesdevEngines.packageManager(notpackageManager). Conflicts betweenpackageManageranddevEngineswarn today (future hard error).vp env doctoradds a devEngines section (semver-aware runtime/PM/spec checks, monorepo workspace root for PM). Shared format-preservingpackage.jsonedits and a newUnsupportedDevEnginesPackageManagererror support these flows.Reviewed by Cursor Bugbot for commit 8ce926e. Configure here.