From 99d5c6e423fe67f7f062296ffd38b38f92b7ab70 Mon Sep 17 00:00:00 2001 From: Miguel Rodrigues <64497525+mbrdg@users.noreply.github.com> Date: Sat, 16 Nov 2024 13:57:14 +0000 Subject: [PATCH] fix(cli): show prefix hint when installing a package globally (#26629) Closes #26545 Shows a hint when a package is installed globally, otherwise fallbacks to the existing implementation. --- cli/tools/installer.rs | 45 ++++++++++++++++++++++- cli/tools/registry/mod.rs | 1 + cli/tools/registry/pm.rs | 2 +- tests/integration/install_tests.rs | 57 ++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 2 deletions(-) diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index 1655f0a322..fe477a8e6c 100644 --- a/cli/tools/installer.rs +++ b/cli/tools/installer.rs @@ -3,6 +3,7 @@ use crate::args::resolve_no_prompt; use crate::args::AddFlags; use crate::args::CaData; +use crate::args::CacheSetting; use crate::args::ConfigFlag; use crate::args::Flags; use crate::args::InstallFlags; @@ -13,8 +14,11 @@ use crate::args::TypeCheckMode; use crate::args::UninstallFlags; use crate::args::UninstallKind; use crate::factory::CliFactory; +use crate::file_fetcher::FileFetcher; use crate::graph_container::ModuleGraphContainer; use crate::http_util::HttpClientProvider; +use crate::jsr::JsrFetchResolver; +use crate::npm::NpmFetchResolver; use crate::util::fs::canonicalize_path_maybe_not_exists; use deno_core::anyhow::bail; @@ -354,12 +358,51 @@ async fn install_global( ) -> Result<(), AnyError> { // ensure the module is cached let factory = CliFactory::from_flags(flags.clone()); + + let http_client = factory.http_client_provider(); + let deps_http_cache = factory.global_http_cache()?; + let mut deps_file_fetcher = FileFetcher::new( + deps_http_cache.clone(), + CacheSetting::ReloadAll, + true, + http_client.clone(), + Default::default(), + None, + ); + + let npmrc = factory.cli_options().unwrap().npmrc(); + + deps_file_fetcher.set_download_log_level(log::Level::Trace); + let deps_file_fetcher = Arc::new(deps_file_fetcher); + let jsr_resolver = Arc::new(JsrFetchResolver::new(deps_file_fetcher.clone())); + let npm_resolver = Arc::new(NpmFetchResolver::new( + deps_file_fetcher.clone(), + npmrc.clone(), + )); + + let entry_text = install_flags_global.module_url.as_str(); + let req = super::registry::AddRmPackageReq::parse(entry_text); + + // found a package requirement but missing the prefix + if let Ok(Err(package_req)) = req { + if jsr_resolver.req_to_nv(&package_req).await.is_some() { + bail!( + "{entry_text} is missing a prefix. Did you mean `{}`?", + crate::colors::yellow(format!("deno install -g jsr:{package_req}")) + ); + } else if npm_resolver.req_to_nv(&package_req).await.is_some() { + bail!( + "{entry_text} is missing a prefix. Did you mean `{}`?", + crate::colors::yellow(format!("deno install -g npm:{package_req}")) + ); + } + } + factory .main_module_graph_container() .await? .load_and_type_check_files(&[install_flags_global.module_url.clone()]) .await?; - let http_client = factory.http_client_provider(); // create the install shim create_install_shim(http_client, &flags, install_flags_global).await diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index 4098d62e37..12289c581b 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -69,6 +69,7 @@ pub use pm::add; pub use pm::cache_top_level_deps; pub use pm::remove; pub use pm::AddCommandName; +pub use pm::AddRmPackageReq; use publish_order::PublishOrderGraph; use unfurl::SpecifierUnfurler; diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 68913e2591..c1ea2c75ea 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -679,7 +679,7 @@ enum AddRmPackageReqValue { } #[derive(Debug, PartialEq, Eq)] -struct AddRmPackageReq { +pub struct AddRmPackageReq { alias: String, value: AddRmPackageReqValue, } diff --git a/tests/integration/install_tests.rs b/tests/integration/install_tests.rs index 4dfd00146d..b0c1e44778 100644 --- a/tests/integration/install_tests.rs +++ b/tests/integration/install_tests.rs @@ -329,3 +329,60 @@ fn check_local_by_default2() { .skip_output_check() .assert_exit_code(0); } + +#[test] +fn show_prefix_hint_on_global_install() { + let context = TestContextBuilder::new() + .add_npm_env_vars() + .add_jsr_env_vars() + .use_http_server() + .use_temp_cwd() + .build(); + let temp_dir = context.temp_dir(); + let temp_dir_str = temp_dir.path().to_string(); + + let env_vars = [ + ("HOME", temp_dir_str.as_str()), + ("USERPROFILE", temp_dir_str.as_str()), + ("DENO_INSTALL_ROOT", ""), + ]; + + for pkg_req in ["npm:@denotest/bin", "jsr:@denotest/add"] { + let name = pkg_req.split_once('/').unwrap().1; + let pkg = pkg_req.split_once(':').unwrap().1; + + // try with prefix and ensure that the installation succeeds + context + .new_command() + .args_vec(["install", "-g", "--name", name, pkg_req]) + .envs(env_vars) + .run() + .skip_output_check() + .assert_exit_code(0); + + // try without the prefix and ensure that the installation fails with the appropriate error + // message + let output = context + .new_command() + .args_vec(["install", "-g", "--name", name, pkg]) + .envs(env_vars) + .run(); + output.assert_exit_code(1); + + let output_text = output.combined_output(); + let expected_text = + format!("error: {pkg} is missing a prefix. Did you mean `deno install -g {pkg_req}`?"); + assert_contains!(output_text, &expected_text); + } + + // try a pckage not in npm and jsr to make sure the appropriate error message still appears + let output = context + .new_command() + .args_vec(["install", "-g", "package-that-does-not-exist"]) + .envs(env_vars) + .run(); + output.assert_exit_code(1); + + let output_text = output.combined_output(); + assert_contains!(output_text, "error: Module not found"); +}