feat: passthrough fallback for vpr/vp i on low Node (#1909 #1916)#1921
feat: passthrough fallback for vpr/vp i on low Node (#1909 #1916)#1921l246804 wants to merge 19 commits into
Conversation
…ch in passthrough Task 1 follow-up: the initial commit (9e7ddfd) omitted the node-semver workspace dependency in vite_shared/Cargo.toml and used `let Some(...)` to match node_semver::Version::parse, which returns Result (not Option). - Add node-semver = { workspace = true } to vite_shared dependencies - Use let Ok(...) for both Version::parse calls (actual + min) - Plan doc updated to match (node_semver returns Result, not Option) Verified: RUSTC_BOOTSTRAP=1 cargo test -p vite_shared --lib passthrough -> 4 passed
…isting install.rs lint
- 12 run_* functions each get a run_*_with_pm variant that accepts an external &PackageManager and calls pm.run_*_command directly (no build). - Original run_* becomes a thin wrapper: build + delegate to *_with_pm. - dispatch_with_pm copies dispatch's match body but calls *_with_pm variants. - build_for_dispatch picks the right build function per command. - dispatch now delegates: build_for_dispatch + dispatch_with_pm. - Add tempfile dev-dep for the new unit test. Verified: RUSTC_BOOTSTRAP=1 cargo test -p vite_pm_cli --lib -> 12 passed.
…+ dispatch_with_pm
- execute() resolves the project's package manager via detect_only (no
devEngines pin), then dispatches:
- Commands::PackageManager: delegates to vite_pm_cli::dispatch_with_pm,
reusing existing resolve_* parameter generation (zero drift).
- Commands::Run: builds <pm> run <script> args manually and runs via
run_command (nr-style), since resolve_install_command auto-prepends
'install'.
- map_pm_error maps UnrecognizedPackageManager to a user-friendly message.
- build_passthrough_envs prepends the node runtime bin to PATH.
- Uses pm.bin_name directly (not resolve_install_command which prepends
'install') to correctly handle run <script>.
Verified: RUSTC_BOOTSTRAP=1 cargo test -p vite_global_cli --bin vp passthrough
-> 4 passed.
Task 4a (Critical): Fix Dlx npx fallback regression on original dispatch path. When vp dlx runs without a package.json, the original code fell back to npx. The refactored dispatch propagated PackageJsonNotFound through build_for_dispatch, making the fallback dead code. Fix: special-case Dlx in dispatch to call handlers::run_dlx directly (preserving the fallback). Task 4b (Medium): Add missing tests for build_passthrough_envs and map_pm_error (plan requires them). Remove dead code merge_envs (never called — resolve_install_command doesn't exist, so Run arm uses pm.bin_name directly). Add #[allow(dead_code)] on execute (was lost during placeholder replacement). Add AbsolutePathBuf import in test module. Verified: - RUSTC_BOOTSTRAP=1 cargo test -p vite_pm_cli --lib -> 12 passed - RUSTC_BOOTSTRAP=1 cargo test -p vite_global_cli --bin vp passthrough -> 6 passed - RUSTC_BOOTSTRAP=1 cargo clippy -p vite_pm_cli -- -D warnings -> clean - RUSTC_BOOTSTRAP=1 cargo clippy -p vite_global_cli --bin vp -- -D warnings -A clippy::result_large_err -> clean
✅ Deploy Preview for viteplus-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Thanks!
plans/2026-06-23-low-node-passthrough.md and specs/2026-06-23-low-node-passthrough-design.md seem left by mistake?
Could you please also clean up outdated comments and dead code like Wired in Task X?
| #[allow(dead_code)] // wired up in Task 4 | ||
| fn print_passthrough_notice(node_version: &str, min: &str) { | ||
| // Reuse shared output style; keep to a single concise line. | ||
| vite_shared::output::info(&format!( |
There was a problem hiding this comment.
| vite_shared::output::info(&format!( | |
| vite_shared::output::warn(&format!( |
Is it better to show a warning?
There was a problem hiding this comment.
Sorry, I haven't tested this PR locally yet; it was mistakenly submitted by AI yesterday.
|
I tested locally, and I found it only works for |
|
To use Codex here, create an environment for this repo. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d290e8af8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| } else { | ||
| // npm, no explicit version: use Node-bundled npm. | ||
| ( | ||
| node_bin_prefix.to_absolute_path_buf(), |
There was a problem hiding this comment.
When low-Node passthrough resolves to npm without an explicit packageManager, node_bin_prefix is already the directory containing npm, but it is stored as install_dir. Package-manager commands later call PackageManager::get_bin_prefix(), which appends another /bin, so vp i in this advertised npm fallback sets PATH to <node>/bin/bin and cannot find the bundled npm. Store the runtime install directory here or avoid appending bin for this case.
Useful? React with 👍 / 👎.
| // name, PATH = node runtime bin (for the pm shim) + pm's bin dir. | ||
| let mut full_args = vec!["run".to_string()]; | ||
| full_args.extend(args.iter().cloned()); | ||
| let envs = build_passthrough_envs(&runtime_bin); |
There was a problem hiding this comment.
For low-Node projects with an explicit packageManager such as pnpm@... or yarn@..., detect_only downloads the package manager into the Vite+ cache, but this env only prepends the Node runtime bin. run_command resolves bin_name using this PATH, so vp run <script>/vpr <script> fails with CannotFindBinaryPath unless that package manager also happens to be globally installed. Include pm.get_bin_prefix() in the passthrough env as well.
Useful? React with 👍 / 👎.
| Ok(vite_pm_cli::dispatch::dispatch_with_pm(cwd, pm_command.clone(), &pm) | ||
| .await?) |
There was a problem hiding this comment.
For explicit pnpm/yarn/npm package managers in low-Node passthrough, dispatch_with_pm runs generated JS shims with a PATH that starts at the package-manager bin dir, but ensure_project_runtime only returns a runtime and does not mutate the process PATH. In a standalone vp install with no system node on PATH (or with a different system node), vp i/vp add cannot run the shim with the resolved project runtime. Prepend runtime_bin before dispatching or pass an env that contains both dirs.
Useful? React with 👍 / 👎.
| let resolution = resolve_node_version(cwd, true).await?; | ||
| Ok(resolution.map(|r| r.version.to_string())) |
There was a problem hiding this comment.
This returns the raw version source, so supported inputs like .node-version containing 18/18.20 or engines.node: "<20" are passed directly to is_node_below_min, fail semver parsing, and skip passthrough. Those projects then continue to the old JS delegation path even though the runtime resolver would choose a Node version below 20.19.0. Compare against the resolved exact Node version instead of the raw source string.
Useful? React with 👍 / 👎.
The vpr shim goes through shim::dispatch → commands::vpr::execute_vpr → delegate::execute, which bypasses run_command_with_options where the passthrough precheck lives. This meant vpr on low-Node projects still crashed with the styleText ESM error. Add the same passthrough precheck logic to execute_vpr: resolve the project's Node version, check should_passthrough, and if eligible call passthrough::execute before delegating to the JS CLI. Fixes the issue reported in PR voidzero-dev#1921 review where vpr didn't work with passthrough. Verified: RUSTC_BOOTSTRAP=1 cargo test -p vite_global_cli --bin vp passthrough -> 6 passed.
These files were used during development but shouldn't be committed to the repo. They contain internal implementation notes and planning docs that aren't part of the public API or documentation.
Replace 'wired up in Task X' comments with descriptive text explaining where each function is called from (run_command_with_options, execute_vpr, execute). Makes the code self-documenting without referencing internal task numbers.
2 participants
Summary
vpr/vp runand the package-manager family bypass the Vite+ JS CLI and run the project's own package manager directly.styleTextcrash on Node < 20.12 — never loads Vite+ JS) and the low-Node case of bug: vp i pins npm@11 into devEngines on low Node, breaking manual npm with "Cannot find module node:path" #1916 (no longer pins/downloads an incompatible npm, no longer writesdevEngines).devEngines"fail leaves side effects" general fix (deferred pin) is split into a follow-up PR.Closes #1909 (low-Node passthrough). Refs #1916.
Changes
vite_sharedMIN_SUPPORTED_NODE = "20.19.0"constant +is_node_below_min()helpervite_installPackageManagerBuilder::detect_only()— builds PM without writing devEnginesvite_pm_clidispatch_with_pm()+ 12*_with_pmhandler variants for external PM injectionvite_global_cliis_eligible,should_passthrough,execute) + precheck inrun_command_with_optionsHow it works
run_command_with_options, before the command match, check if the command is eligible (Run / PackageManager) and the project's resolved Node < 20.19.0.detect_only(no devEngines pin), and dispatch viadispatch_with_pm(PM commands) orrun_command(Run commands).Test plan
cargo test -p vite_shared --lib→ 92 passedcargo test -p vite_install --lib→ 552 passedcargo test -p vite_pm_cli --lib→ 12 passedcargo test -p vite_global_cli --bin vp passthrough→ 6 passedcargo clippyon all 4 crates → clean.node-version=14.15.0+vpr/vp ipassthrough, nodevEngineswrittenpnpm bootstrap-cli && pnpm test && git status(requires cmake)