feat(pm): show download progress when fetching a package manager - #2369
Conversation
`vp env`/shim installs of npm, pnpm, yarn, and bun silently downloaded in the background with only debug-level tracing, so users on a slow connection couldn't tell if the CLI had hung. Mirror the progress-bar pattern already used for Node.js runtime downloads (vp_js_runtime): show a byte progress bar with speed/ETA on a TTY outside CI. Closes voidzero-dev#2351
✅ Deploy Preview for viteplus-preview canceled.
|
|
could we add some test coverage for this? |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ 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". |
Extract the progress-bar visibility check into should_show_progress and unit test its truth table, plus add download_file / download_and_extract_tgz_with_hash regression tests that pass a message to confirm it doesn't affect download/extract results.
|
Good call — added coverage in fa7adc3:
|
|
thanks for adding these. I was expecting a snapshot test for the visible progress bar, but it looks difficult to test reliably because the bar is cleared on completion. On a closer look, these tests don't exercise the visible progress-bar path, so I don't think they provide meaningful coverage. Could we remove them for now? Also, could you drop the unrelated |
…sible path Per review feedback: these tests didn't exercise the actual visible progress-bar rendering (which is TTY-gated and cleared on completion, so hard to assert on reliably), and left an extraction with no meaningful test value. Also drop unrelated rolldown Cargo.lock churn from a local vendored-checkout sync.
|
Fair point on both — done in fc957c0:
|
jong-kyung
left a comment
jong-kyung
left a comment
There was a problem hiding this comment.
vp migrate has its own spinner. @fengmk2, Would it be okay if I triggered a preview-build build to check wherter the two spinners interfere with each other? Everything else looks good to me.
|
|
If we merge it, I'll check it in my local 🙇♂️ |
|
Sounds good, thanks both! Happy to have this merged first and verify the spinner interaction with |
fengmk2
changed the title
|
publish a preview build at #2373 (comment) |
Checked locally, no spinner conflicts. Works well 👍 |
3 participants
Closes #2351
Problem
When a project's workspace declares a package manager (
packageManager, lockfile,devEngines, etc.) that isn't installed yet,vpdownloads it silently. On a slow connection there's no way to tell whether the download is progressing or has hung — no log line, no progress indicator, nothing until it either succeeds or errors out.Root cause
The package-manager download path (
crates/vp_pm_cli/src/request.rs:HttpClient::download_file,download_and_extract_tgz_with_hash) only emitstracing::debug!calls, which aren't shown at the default log level. This is unlike the Node.js runtime download path (crates/vp_js_runtime/src/download.rs), which already renders anindicatifprogress bar.Fix
Mirror the existing
vp_js_runtimepattern invp_pm_cli:HttpClient::download_filetakes an optionalmessage, and shows a byte progress bar (speed, ETA) onceContent-Lengthis known, gated onstderrbeing a TTY and not running in CI — non-interactive/piped output stays unchanged.download_and_extract_tgz_with_hashthreads the message through."Downloading pnpm v10.0.0...".Testing
cargo check -p vp_pm_cli,cargo clippy -p vp_pm_cli --all-targets,cargo fmt -p vp_pm_cli --checkcargo test -p vp_pm_cli --lib: all download-path tests pass (request::tests::*,package_manager::tests::test_download_*)