feat: support devEngines for Node.js runtime and package manager selection#1760
Draft
fengmk2 wants to merge 2 commits into
Draft
feat: support devEngines for Node.js runtime and package manager selection#1760fengmk2 wants to merge 2 commits into
fengmk2 wants to merge 2 commits into
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
…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
Contributor
|
✅ Staging deployment successful! Preview: https://viteplus-staging.void.app/ |
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)
Member
Author
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c13a831. Configure here.
| }); | ||
| let updated = vite_shared::edit_json_object(&content, |obj| { | ||
| if let Some(dev_engines) = obj.get_mut("devEngines").and_then(|v| v.as_object_mut()) { | ||
| dev_engines.insert("packageManager".into(), entry); |
There was a problem hiding this comment.
Auto-pin wipes PM array
Medium Severity
When auto-pin runs after lockfile or default detection, set_dev_engines_package_manager_field assigns a single packageManager object via insert, which replaces an existing devEngines.packageManager array and drops alternate entries the spec allows.
Reviewed by Cursor Bugbot for commit c13a831. Configure here.
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.
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 and package-manager resolution plus automatic package.json mutations on pin and lockfile detection; broad test coverage limits regressions but wrong resolution would affect every project command.
Overview
Implements the devEngines RFC so
devEngines.runtimeanddevEngines.packageManagerdrive development tooling alongside existing pins, without breaking.node-versionor the top-levelpackageManagerfield when they already exist.Node.js: Resolution now prefers
devEngines.runtimeoverengines.node(still below.node-version).vp env pin/unpinfollow a compatibility-first write target (existing.node-versionwins; otherwisedevEngines.runtime;--targetoverrides). Pinning can optionally syncdevEngines.runtimewhen it conflicts with a new.node-version, andengines.nodeis never modified.Package managers: Detection inserts
devEngines.packageManagerafterpackageManagerand before lockfiles; semver ranges resolve from cache then the registry and are not frozen into an exactpackageManagerpin. Lockfile/default auto-pin writesdevEngines.packageManagerinstead ofpackageManager. Mismatches betweenpackageManageranddevEnginesemit warnings (future hard error).Shared infrastructure: Lenient
devEnginesparsing (OnFail, array/single shapes),edit_json_objectfor indentation-preservingpackage.jsonedits, and a newUnsupportedDevEnginesPackageManagererror.Diagnostics & docs:
vp env doctoradds a semver-aware devEngines section; user guides and CLI snap tests reflect the new auto-pin shape.Reviewed by Cursor Bugbot for commit c13a831. Configure here.