Skip to content

Commit 6f502c8

Browse files
committed
refactor(installer): address simplify review findings
- Move same-version check before platform package HTTP request: use resolve_version_string (1 HTTP call) first, then skip resolve_platform_package (2nd HTTP call) when version matches. Saves 1 HTTP request for tag matches, both for exact version matches. - Fix else { if let → else if let (clippy collapsible_else_if) - Consolidate VP_NODE_MANAGER handling: both "yes" and "no" now checked in should_enable_node_manager instead of split across cli.rs and main.rs - Make create_env_files return Result and report via print_warn, consistent with other best-effort post-activation steps
1 parent e90116f commit 6f502c8

2 files changed

Lines changed: 48 additions & 46 deletions

File tree

crates/vite_installer/src/cli.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,8 @@ pub fn parse() -> Options {
5454
if opts.registry.is_none() {
5555
opts.registry = std::env::var("NPM_CONFIG_REGISTRY").ok();
5656
}
57-
if !opts.no_node_manager {
58-
opts.no_node_manager =
59-
std::env::var("VP_NODE_MANAGER").ok().is_some_and(|v| v.eq_ignore_ascii_case("no"));
60-
}
57+
// VP_NODE_MANAGER env var is handled in should_enable_node_manager()
58+
// to keep both "yes" and "no" logic in one place.
6159

6260
// quiet implies yes
6361
if opts.quiet {

crates/vite_installer/src/main.rs

Lines changed: 46 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -94,41 +94,43 @@ async fn do_install(
9494
print_info(&format!("detected platform: {platform_suffix}"));
9595
}
9696

97+
// Check local version first to potentially skip HTTP requests
98+
tokio::fs::create_dir_all(install_dir).await?;
99+
let current_version = install::read_current_version(install_dir).await;
100+
97101
let version_or_tag = opts.version.as_deref().unwrap_or(&opts.tag);
102+
103+
// Resolve the target version — use resolve_version_string first so we can
104+
// skip the platform package fetch if the version is already installed
98105
if !opts.quiet {
99106
print_info(&format!("resolving version '{version_or_tag}'..."));
100107
}
101-
let resolved =
102-
registry::resolve_version(version_or_tag, &platform_suffix, opts.registry.as_deref())
103-
.await?;
104-
if !opts.quiet {
105-
print_info(&format!("found vite-plus@{}", resolved.version));
106-
}
108+
let target_version =
109+
registry::resolve_version_string(version_or_tag, opts.registry.as_deref()).await?;
107110

108-
tokio::fs::create_dir_all(install_dir).await?;
109-
110-
let current_version = install::read_current_version(install_dir).await;
111-
let same_version = current_version.as_deref() == Some(resolved.version.as_str());
111+
let same_version = current_version.as_deref() == Some(target_version.as_str());
112112

113113
if same_version {
114114
if !opts.quiet {
115-
print_info(&format!(
116-
"version {} already installed, verifying setup...",
117-
resolved.version
118-
));
115+
print_info(&format!("version {target_version} already installed, verifying setup..."));
119116
}
120-
} else {
121-
if let Some(ref current) = current_version {
122-
if !opts.quiet {
123-
print_info(&format!("upgrading from {current} to {}", resolved.version));
124-
}
117+
} else if let Some(ref current) = current_version {
118+
if !opts.quiet {
119+
print_info(&format!("upgrading from {current} to {target_version}"));
125120
}
121+
}
122+
123+
if !same_version {
124+
// Only fetch platform metadata + download when we actually need to install
125+
let resolved = registry::resolve_platform_package(
126+
&target_version,
127+
&platform_suffix,
128+
opts.registry.as_deref(),
129+
)
130+
.await?;
126131

127132
if !opts.quiet {
128-
print_info(&format!(
129-
"downloading vite-plus@{} for {}...",
130-
resolved.version, platform_suffix
131-
));
133+
print_info(&format!("downloading vite-plus@{target_version} for {platform_suffix}..."));
132134
}
133135
let client = HttpClient::new();
134136
let platform_data =
@@ -139,7 +141,7 @@ async fn do_install(
139141
}
140142
integrity::verify_integrity(&platform_data, &resolved.platform_integrity)?;
141143

142-
let version_dir = install_dir.join(&resolved.version);
144+
let version_dir = install_dir.join(&target_version);
143145
tokio::fs::create_dir_all(&version_dir).await?;
144146

145147
if !opts.quiet {
@@ -152,7 +154,7 @@ async fn do_install(
152154
return Err("Binary not found after extraction. The download may be corrupted.".into());
153155
}
154156

155-
install::generate_wrapper_package_json(&version_dir, &resolved.version).await?;
157+
install::generate_wrapper_package_json(&version_dir, &target_version).await?;
156158
install::write_release_age_overrides(&version_dir).await?;
157159

158160
if !opts.quiet {
@@ -165,24 +167,24 @@ async fn do_install(
165167
} else {
166168
None
167169
};
168-
install::swap_current_link(install_dir, &resolved.version).await?;
170+
install::swap_current_link(install_dir, &target_version).await?;
169171

170172
// Cleanup with both new and previous versions protected (matches vp upgrade)
171-
let mut protected = vec![resolved.version.as_str()];
173+
let mut protected = vec![target_version.as_str()];
172174
if let Some(ref prev) = previous_version {
173175
protected.push(prev.as_str());
174176
}
175177
if let Err(e) =
176178
install::cleanup_old_versions(install_dir, vite_setup::MAX_VERSIONS_KEEP, &protected)
177179
.await
178180
{
179-
tracing::warn!("Old version cleanup failed (non-fatal): {e}");
181+
print_warn(&format!("Old version cleanup failed (non-fatal): {e}"));
180182
}
181183
}
182184

183185
// --- Post-activation setup (always runs, even for same-version repair) ---
184-
// All steps below are best-effort after activation: the core install succeeded
185-
// once `current` points at the right version.
186+
// All steps below are best-effort: the core install succeeded once `current`
187+
// points at the right version.
186188

187189
if !opts.quiet {
188190
print_info("setting up shims...");
@@ -191,7 +193,6 @@ async fn do_install(
191193
print_warn(&format!("Shim setup failed (non-fatal): {e}"));
192194
}
193195

194-
// Node.js manager: match install.ps1/install.sh auto-detect logic
195196
let enable_node_manager = should_enable_node_manager(opts, install_dir);
196197
if enable_node_manager {
197198
if !opts.quiet {
@@ -201,8 +202,9 @@ async fn do_install(
201202
print_warn(&format!("Node.js manager setup failed (non-fatal): {e}"));
202203
}
203204
} else {
204-
// Still create shell env files even without Node.js manager
205-
create_env_files(install_dir).await;
205+
if let Err(e) = create_env_files(install_dir).await {
206+
print_warn(&format!("Env file creation failed (non-fatal): {e}"));
207+
}
206208
}
207209

208210
if !opts.no_modify_path {
@@ -226,12 +228,14 @@ async fn do_install(
226228
/// 6. Silent mode with system node → disable (don't silently take over)
227229
#[allow(clippy::print_stdout)]
228230
fn should_enable_node_manager(opts: &cli::Options, install_dir: &vite_path::AbsolutePath) -> bool {
231+
// --no-node-manager CLI flag
229232
if opts.no_node_manager {
230233
return false;
231234
}
232235

233-
if std::env::var("VP_NODE_MANAGER").ok().is_some_and(|v| v.eq_ignore_ascii_case("yes")) {
234-
return true;
236+
// VP_NODE_MANAGER env var: "yes" or "no" (both handled here)
237+
if let Ok(val) = std::env::var("VP_NODE_MANAGER") {
238+
return val.eq_ignore_ascii_case("yes");
235239
}
236240

237241
// Already managing Node (shims exist from a previous install)
@@ -358,21 +362,21 @@ async fn download_with_progress(
358362
Ok(data)
359363
}
360364

361-
async fn create_env_files(install_dir: &vite_path::AbsolutePath) {
365+
async fn create_env_files(
366+
install_dir: &vite_path::AbsolutePath,
367+
) -> Result<(), Box<dyn std::error::Error>> {
362368
let vp_binary = install_dir.join("current").join("bin").join(VP_BINARY_NAME);
363369

364370
if !tokio::fs::try_exists(&vp_binary).await.unwrap_or(false) {
365-
return;
371+
return Ok(());
366372
}
367373

368-
let output = tokio::process::Command::new(vp_binary.as_path())
374+
tokio::process::Command::new(vp_binary.as_path())
369375
.args(["env", "setup", "--env-only"])
370376
.output()
371-
.await;
377+
.await?;
372378

373-
if let Err(e) = output {
374-
tracing::warn!("Failed to create env files (non-fatal): {e}");
375-
}
379+
Ok(())
376380
}
377381

378382
fn resolve_install_dir(opts: &cli::Options) -> Result<AbsolutePathBuf, Box<dyn std::error::Error>> {

0 commit comments

Comments
 (0)