From 706b1dfcea8ab6bf7d155893ab795669107516a8 Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Mon, 4 Nov 2024 18:45:00 -0800 Subject: [PATCH] fix(add): better error message when adding package that only has pre-release versions (#26724) Fixes https://github.com/denoland/deno/issues/26597 A small refactor as well to reduce some code duplication --- cli/tools/registry/pm.rs | 205 +++++++++++++----- .../@denotest/unstable/1.0.0-beta.1/mod.ts | 3 + .../@denotest/unstable/1.0.0-beta.1_meta.json | 5 + .../@denotest/unstable/1.0.0-beta.2/mod.ts | 3 + .../@denotest/unstable/1.0.0-beta.2_meta.json | 5 + .../registry/jsr/@denotest/unstable/meta.json | 6 + .../add/only_unstable_versions/__test__.jsonc | 23 +- .../add/only_unstable_versions/add_jsr.out | 1 + 8 files changed, 187 insertions(+), 64 deletions(-) create mode 100644 tests/registry/jsr/@denotest/unstable/1.0.0-beta.1/mod.ts create mode 100644 tests/registry/jsr/@denotest/unstable/1.0.0-beta.1_meta.json create mode 100644 tests/registry/jsr/@denotest/unstable/1.0.0-beta.2/mod.ts create mode 100644 tests/registry/jsr/@denotest/unstable/1.0.0-beta.2_meta.json create mode 100644 tests/registry/jsr/@denotest/unstable/meta.json create mode 100644 tests/specs/add/only_unstable_versions/add_jsr.out diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 4e03879983..68913e2591 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -12,7 +12,9 @@ use deno_core::futures::StreamExt; use deno_path_util::url_to_file_path; use deno_semver::jsr::JsrPackageReqReference; use deno_semver::npm::NpmPackageReqReference; +use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; +use deno_semver::Version; use deno_semver::VersionReq; use jsonc_parser::cst::CstObject; use jsonc_parser::cst::CstObjectProp; @@ -455,15 +457,32 @@ pub async fn add( match package_and_version { PackageAndVersion::NotFound { package: package_name, - found_npm_package, + help, package_req, - } => { - if found_npm_package { - bail!("{} was not found, but a matching npm package exists. Did you mean `{}`?", crate::colors::red(package_name), crate::colors::yellow(format!("deno {cmd_name} npm:{package_req}"))); - } else { - bail!("{} was not found.", crate::colors::red(package_name)); + } => match help { + Some(NotFoundHelp::NpmPackage) => { + bail!( + "{} was not found, but a matching npm package exists. Did you mean `{}`?", + crate::colors::red(package_name), + crate::colors::yellow(format!("deno {cmd_name} npm:{package_req}")) + ); } - } + Some(NotFoundHelp::JsrPackage) => { + bail!( + "{} was not found, but a matching jsr package exists. Did you mean `{}`?", + crate::colors::red(package_name), + crate::colors::yellow(format!("deno {cmd_name} jsr:{package_req}")) + ) + } + Some(NotFoundHelp::PreReleaseVersion(version)) => { + bail!( + "{} has only pre-release versions available. Try specifying a version: `{}`", + crate::colors::red(&package_name), + crate::colors::yellow(format!("deno {cmd_name} {package_name}@^{version}")) + ) + } + None => bail!("{} was not found.", crate::colors::red(package_name)), + }, PackageAndVersion::Selected(selected) => { selected_packages.push(selected); } @@ -511,76 +530,144 @@ struct SelectedPackage { selected_version: String, } +enum NotFoundHelp { + NpmPackage, + JsrPackage, + PreReleaseVersion(Version), +} + enum PackageAndVersion { NotFound { package: String, - found_npm_package: bool, package_req: PackageReq, + help: Option, }, Selected(SelectedPackage), } +fn best_version<'a>( + versions: impl Iterator, +) -> Option<&'a Version> { + let mut maybe_best_version: Option<&Version> = None; + for version in versions { + let is_best_version = maybe_best_version + .as_ref() + .map(|best_version| (*best_version).cmp(version).is_lt()) + .unwrap_or(true); + if is_best_version { + maybe_best_version = Some(version); + } + } + maybe_best_version +} + +trait PackageInfoProvider { + const SPECIFIER_PREFIX: &str; + /// The help to return if a package is found by this provider + const HELP: NotFoundHelp; + async fn req_to_nv(&self, req: &PackageReq) -> Option; + async fn latest_version<'a>(&self, req: &PackageReq) -> Option; +} + +impl PackageInfoProvider for Arc { + const HELP: NotFoundHelp = NotFoundHelp::JsrPackage; + const SPECIFIER_PREFIX: &str = "jsr"; + async fn req_to_nv(&self, req: &PackageReq) -> Option { + (**self).req_to_nv(req).await + } + + async fn latest_version<'a>(&self, req: &PackageReq) -> Option { + let info = self.package_info(&req.name).await?; + best_version( + info + .versions + .iter() + .filter(|(_, version_info)| !version_info.yanked) + .map(|(version, _)| version), + ) + .cloned() + } +} + +impl PackageInfoProvider for Arc { + const HELP: NotFoundHelp = NotFoundHelp::NpmPackage; + const SPECIFIER_PREFIX: &str = "npm"; + async fn req_to_nv(&self, req: &PackageReq) -> Option { + (**self).req_to_nv(req).await + } + + async fn latest_version<'a>(&self, req: &PackageReq) -> Option { + let info = self.package_info(&req.name).await?; + best_version(info.versions.keys()).cloned() + } +} + async fn find_package_and_select_version_for_req( jsr_resolver: Arc, npm_resolver: Arc, add_package_req: AddRmPackageReq, ) -> Result { - match add_package_req.value { - AddRmPackageReqValue::Jsr(req) => { - let jsr_prefixed_name = format!("jsr:{}", &req.name); - let Some(nv) = jsr_resolver.req_to_nv(&req).await else { - if npm_resolver.req_to_nv(&req).await.is_some() { + async fn select( + main_resolver: T, + fallback_resolver: S, + add_package_req: AddRmPackageReq, + ) -> Result { + let req = match &add_package_req.value { + AddRmPackageReqValue::Jsr(req) => req, + AddRmPackageReqValue::Npm(req) => req, + }; + let prefixed_name = format!("{}:{}", T::SPECIFIER_PREFIX, req.name); + let help_if_found_in_fallback = S::HELP; + let Some(nv) = main_resolver.req_to_nv(req).await else { + if fallback_resolver.req_to_nv(req).await.is_some() { + // it's in the other registry + return Ok(PackageAndVersion::NotFound { + package: prefixed_name, + help: Some(help_if_found_in_fallback), + package_req: req.clone(), + }); + } + if req.version_req.version_text() == "*" { + if let Some(pre_release_version) = + main_resolver.latest_version(req).await + { return Ok(PackageAndVersion::NotFound { - package: jsr_prefixed_name, - found_npm_package: true, - package_req: req, + package: prefixed_name, + package_req: req.clone(), + help: Some(NotFoundHelp::PreReleaseVersion( + pre_release_version.clone(), + )), }); } + } - return Ok(PackageAndVersion::NotFound { - package: jsr_prefixed_name, - found_npm_package: false, - package_req: req, - }); - }; - let range_symbol = if req.version_req.version_text().starts_with('~') { - "~" - } else if req.version_req.version_text() == nv.version.to_string() { - "" - } else { - "^" - }; - Ok(PackageAndVersion::Selected(SelectedPackage { - import_name: add_package_req.alias, - package_name: jsr_prefixed_name, - version_req: format!("{}{}", range_symbol, &nv.version), - selected_version: nv.version.to_string(), - })) + return Ok(PackageAndVersion::NotFound { + package: prefixed_name, + help: None, + package_req: req.clone(), + }); + }; + let range_symbol = if req.version_req.version_text().starts_with('~') { + "~" + } else if req.version_req.version_text() == nv.version.to_string() { + "" + } else { + "^" + }; + Ok(PackageAndVersion::Selected(SelectedPackage { + import_name: add_package_req.alias, + package_name: prefixed_name, + version_req: format!("{}{}", range_symbol, &nv.version), + selected_version: nv.version.to_string(), + })) + } + + match &add_package_req.value { + AddRmPackageReqValue::Jsr(_) => { + select(jsr_resolver, npm_resolver, add_package_req).await } - AddRmPackageReqValue::Npm(req) => { - let npm_prefixed_name = format!("npm:{}", &req.name); - let Some(nv) = npm_resolver.req_to_nv(&req).await else { - return Ok(PackageAndVersion::NotFound { - package: npm_prefixed_name, - found_npm_package: false, - package_req: req, - }); - }; - - let range_symbol = if req.version_req.version_text().starts_with('~') { - "~" - } else if req.version_req.version_text() == nv.version.to_string() { - "" - } else { - "^" - }; - - Ok(PackageAndVersion::Selected(SelectedPackage { - import_name: add_package_req.alias, - package_name: npm_prefixed_name, - version_req: format!("{}{}", range_symbol, &nv.version), - selected_version: nv.version.to_string(), - })) + AddRmPackageReqValue::Npm(_) => { + select(npm_resolver, jsr_resolver, add_package_req).await } } } diff --git a/tests/registry/jsr/@denotest/unstable/1.0.0-beta.1/mod.ts b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.1/mod.ts new file mode 100644 index 0000000000..de63686189 --- /dev/null +++ b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.1/mod.ts @@ -0,0 +1,3 @@ +export function doThing() { + return "thing"; +} \ No newline at end of file diff --git a/tests/registry/jsr/@denotest/unstable/1.0.0-beta.1_meta.json b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.1_meta.json new file mode 100644 index 0000000000..631a18d0e5 --- /dev/null +++ b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.1_meta.json @@ -0,0 +1,5 @@ +{ + "exports": { + ".": "./mod.ts" + } +} diff --git a/tests/registry/jsr/@denotest/unstable/1.0.0-beta.2/mod.ts b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.2/mod.ts new file mode 100644 index 0000000000..4e599641ef --- /dev/null +++ b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.2/mod.ts @@ -0,0 +1,3 @@ +export function doThing() { + return "thing2"; +} \ No newline at end of file diff --git a/tests/registry/jsr/@denotest/unstable/1.0.0-beta.2_meta.json b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.2_meta.json new file mode 100644 index 0000000000..631a18d0e5 --- /dev/null +++ b/tests/registry/jsr/@denotest/unstable/1.0.0-beta.2_meta.json @@ -0,0 +1,5 @@ +{ + "exports": { + ".": "./mod.ts" + } +} diff --git a/tests/registry/jsr/@denotest/unstable/meta.json b/tests/registry/jsr/@denotest/unstable/meta.json new file mode 100644 index 0000000000..7c5c9971ef --- /dev/null +++ b/tests/registry/jsr/@denotest/unstable/meta.json @@ -0,0 +1,6 @@ +{ + "versions": { + "1.0.0-beta.1": {}, + "1.0.0-beta.2": {} + } +} diff --git a/tests/specs/add/only_unstable_versions/__test__.jsonc b/tests/specs/add/only_unstable_versions/__test__.jsonc index d05628b6f5..ea5a270b9c 100644 --- a/tests/specs/add/only_unstable_versions/__test__.jsonc +++ b/tests/specs/add/only_unstable_versions/__test__.jsonc @@ -1,9 +1,22 @@ { "tempDir": true, - "steps": [ - { - "args": "add npm:@denotest/unstable", - "output": "add.out" + "tests": { + "npm_package": { + "steps": [ + { + "args": "add npm:@denotest/unstable", + "output": "add.out" + } + ] + }, + "jsr_package": { + "steps": [ + { + "args": "add jsr:@denotest/unstable", + "output": "add_jsr.out", + "exitCode": 1 + } + ] } - ] + } } diff --git a/tests/specs/add/only_unstable_versions/add_jsr.out b/tests/specs/add/only_unstable_versions/add_jsr.out new file mode 100644 index 0000000000..95f0630bf3 --- /dev/null +++ b/tests/specs/add/only_unstable_versions/add_jsr.out @@ -0,0 +1 @@ +error: jsr:@denotest/unstable has only pre-release versions available. Try specifying a version: `deno add jsr:@denotest/unstable@^1.0.0-beta.2`