From ec86089c4e95eabbf9edc48491031eb6711a410f Mon Sep 17 00:00:00 2001 From: Leo Kettmeir Date: Wed, 18 Sep 2024 11:38:22 -0700 Subject: [PATCH] feat: require jsr prefix for `deno install` and `deno add` (#25698) --- cli/tools/registry/pm.rs | 93 +++++++++++-------- tests/integration/pm_tests.rs | 12 +-- .../specs/add/add_with_subpath/__test__.jsonc | 4 +- .../add_with_subpath/wrong_constraint_jsr.out | 2 +- .../add/missing_npm_specifier/__test__.jsonc | 8 +- tests/specs/add/missing_prefix/__test__.jsonc | 15 +++ tests/specs/add/missing_prefix/deno.json | 0 tests/specs/add/no_root_export/__test__.jsonc | 2 +- .../package_json_and_deno_json/__test__.jsonc | 2 +- tests/specs/remove/basic/__test__.jsonc | 2 +- 10 files changed, 85 insertions(+), 55 deletions(-) create mode 100644 tests/specs/add/missing_prefix/__test__.jsonc create mode 100644 tests/specs/add/missing_prefix/deno.json diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 1f8463b00c..05785ec743 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -437,18 +437,6 @@ pub async fn add( } let http_client = cli_factory.http_client_provider(); - - let mut selected_packages = Vec::with_capacity(add_flags.packages.len()); - let mut package_reqs = Vec::with_capacity(add_flags.packages.len()); - - for entry_text in add_flags.packages.iter() { - let req = AddPackageReq::parse(entry_text).with_context(|| { - format!("Failed to parse package required: {}", entry_text) - })?; - - package_reqs.push(req); - } - let deps_http_cache = cli_factory.global_http_cache()?; let mut deps_file_fetcher = FileFetcher::new( deps_http_cache.clone(), @@ -463,6 +451,37 @@ pub async fn add( let jsr_resolver = Arc::new(JsrFetchResolver::new(deps_file_fetcher.clone())); let npm_resolver = Arc::new(NpmFetchResolver::new(deps_file_fetcher)); + let mut selected_packages = Vec::with_capacity(add_flags.packages.len()); + let mut package_reqs = Vec::with_capacity(add_flags.packages.len()); + + for entry_text in add_flags.packages.iter() { + let req = AddPackageReq::parse(entry_text).with_context(|| { + format!("Failed to parse package required: {}", entry_text) + })?; + + match req { + Ok(add_req) => package_reqs.push(add_req), + Err(package_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 {cmd_name} 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 {cmd_name} npm:{package_req}")) + ) + } else { + bail!( + "{} was not found in either jsr or npm.", + crate::colors::red(entry_text) + ); + } + } + } + } + let package_futures = package_reqs .into_iter() .map({ @@ -636,7 +655,7 @@ struct AddPackageReq { } impl AddPackageReq { - pub fn parse(entry_text: &str) -> Result { + pub fn parse(entry_text: &str) -> Result, AnyError> { enum Prefix { Jsr, Npm, @@ -675,13 +694,13 @@ impl AddPackageReq { None => match parse_alias(entry_text) { Some((alias, text)) => { let (maybe_prefix, entry_text) = parse_prefix(text); - ( - maybe_prefix.unwrap_or(Prefix::Jsr), - Some(alias.to_string()), - entry_text, - ) + if maybe_prefix.is_none() { + return Ok(Err(PackageReq::from_str(entry_text)?)); + } + + (maybe_prefix.unwrap(), Some(alias.to_string()), entry_text) } - None => (Prefix::Jsr, None, entry_text), + None => return Ok(Err(PackageReq::from_str(entry_text)?)), }, }; @@ -690,19 +709,19 @@ impl AddPackageReq { let req_ref = JsrPackageReqReference::from_str(&format!("jsr:{}", entry_text))?; let package_req = req_ref.into_inner().req; - Ok(AddPackageReq { + Ok(Ok(AddPackageReq { alias: maybe_alias.unwrap_or_else(|| package_req.name.to_string()), value: AddPackageReqValue::Jsr(package_req), - }) + })) } Prefix::Npm => { let req_ref = NpmPackageReqReference::from_str(&format!("npm:{}", entry_text))?; let package_req = req_ref.into_inner().req; - Ok(AddPackageReq { + Ok(Ok(AddPackageReq { alias: maybe_alias.unwrap_or_else(|| package_req.name.to_string()), value: AddPackageReqValue::Npm(package_req), - }) + })) } } } @@ -847,42 +866,42 @@ fn update_config_file_content< #[cfg(test)] mod test { - use deno_semver::VersionReq; - use super::*; #[test] fn test_parse_add_package_req() { assert_eq!( - AddPackageReq::parse("jsr:foo").unwrap(), + AddPackageReq::parse("jsr:foo").unwrap().unwrap(), AddPackageReq { alias: "foo".to_string(), value: AddPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("alias@jsr:foo").unwrap(), + AddPackageReq::parse("alias@jsr:foo").unwrap().unwrap(), AddPackageReq { alias: "alias".to_string(), value: AddPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("@alias/pkg@npm:foo").unwrap(), + AddPackageReq::parse("@alias/pkg@npm:foo").unwrap().unwrap(), AddPackageReq { alias: "@alias/pkg".to_string(), value: AddPackageReqValue::Npm(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("@alias/pkg@jsr:foo").unwrap(), + AddPackageReq::parse("@alias/pkg@jsr:foo").unwrap().unwrap(), AddPackageReq { alias: "@alias/pkg".to_string(), value: AddPackageReqValue::Jsr(PackageReq::from_str("foo").unwrap()) } ); assert_eq!( - AddPackageReq::parse("alias@jsr:foo@^1.5.0").unwrap(), + AddPackageReq::parse("alias@jsr:foo@^1.5.0") + .unwrap() + .unwrap(), AddPackageReq { alias: "alias".to_string(), value: AddPackageReqValue::Jsr( @@ -891,15 +910,11 @@ mod test { } ); assert_eq!( - AddPackageReq::parse("@scope/pkg@tag").unwrap(), - AddPackageReq { - alias: "@scope/pkg".to_string(), - value: AddPackageReqValue::Jsr(PackageReq { - name: "@scope/pkg".to_string(), - // this is a tag - version_req: VersionReq::parse_from_specifier("tag").unwrap(), - }), - } + AddPackageReq::parse("@scope/pkg@tag") + .unwrap() + .unwrap_err() + .to_string(), + "@scope/pkg@tag", ); } } diff --git a/tests/integration/pm_tests.rs b/tests/integration/pm_tests.rs index bd136f1ce2..e3db9006fa 100644 --- a/tests/integration/pm_tests.rs +++ b/tests/integration/pm_tests.rs @@ -16,7 +16,7 @@ fn add_basic() { let temp_dir = context.temp_dir().path(); temp_dir.join("deno.json").write_json(&starting_deno_json); - let output = context.new_command().args("add @denotest/add").run(); + let output = context.new_command().args("add jsr:@denotest/add").run(); output.assert_exit_code(0); let output = output.combined_output(); assert_contains!(output, "Add jsr:@denotest/add"); @@ -35,7 +35,7 @@ fn add_basic_no_deno_json() { let context = pm_context_builder().build(); let temp_dir = context.temp_dir().path(); - let output = context.new_command().args("add @denotest/add").run(); + let output = context.new_command().args("add jsr:@denotest/add").run(); output.assert_exit_code(0); let output = output.combined_output(); assert_contains!(output, "Add jsr:@denotest/add"); @@ -55,7 +55,7 @@ fn add_basic_with_empty_deno_json() { let temp_dir = context.temp_dir(); temp_dir.write("deno.json", ""); - let output = context.new_command().args("add @denotest/add").run(); + let output = context.new_command().args("add jsr:@denotest/add").run(); output.assert_exit_code(0); let output = output.combined_output(); assert_contains!(output, "Add jsr:@denotest/add"); @@ -74,7 +74,7 @@ fn add_version_contraint() { let context = pm_context_builder().build(); let temp_dir = context.temp_dir().path(); - let output = context.new_command().args("add @denotest/add@1").run(); + let output = context.new_command().args("add jsr:@denotest/add@1").run(); output.assert_exit_code(0); let output = output.combined_output(); assert_contains!(output, "Add jsr:@denotest/add"); @@ -90,7 +90,7 @@ fn add_tilde() { let context = pm_context_builder().build(); let temp_dir = context.temp_dir().path(); - let output = context.new_command().args("add @denotest/add@~1").run(); + let output = context.new_command().args("add jsr:@denotest/add@~1").run(); output.assert_exit_code(0); let output = output.combined_output(); assert_contains!(output, "Add jsr:@denotest/add"); @@ -114,7 +114,7 @@ fn add_multiple() { let output = context .new_command() - .args("add @denotest/add @denotest/subset-type-graph") + .args("add jsr:@denotest/add jsr:@denotest/subset-type-graph") .run(); output.assert_exit_code(0); let output = output.combined_output(); diff --git a/tests/specs/add/add_with_subpath/__test__.jsonc b/tests/specs/add/add_with_subpath/__test__.jsonc index b051bd2654..edd45299dd 100644 --- a/tests/specs/add/add_with_subpath/__test__.jsonc +++ b/tests/specs/add/add_with_subpath/__test__.jsonc @@ -2,11 +2,11 @@ "tempDir": true, "steps": [ { - "args": "add @std/testing/bdd npm:preact/hooks", + "args": "add jsr:@std/testing/bdd npm:preact/hooks", "output": "add.out" }, { - "args": "add @std/testing/bdd@1 npm:preact/hooks@10", + "args": "add jsr:@std/testing/bdd@1 npm:preact/hooks@10", "output": "wrong_constraint_jsr.out", "exitCode": 1 }, diff --git a/tests/specs/add/add_with_subpath/wrong_constraint_jsr.out b/tests/specs/add/add_with_subpath/wrong_constraint_jsr.out index 2b218407d2..7de7008e93 100644 --- a/tests/specs/add/add_with_subpath/wrong_constraint_jsr.out +++ b/tests/specs/add/add_with_subpath/wrong_constraint_jsr.out @@ -1,4 +1,4 @@ -error: Failed to parse package required: @std/testing/bdd@1 +error: Failed to parse package required: jsr:@std/testing/bdd@1 Caused by: Invalid package specifier 'jsr:@std/testing/bdd@1'. Did you mean to write 'jsr:@std/testing@1/bdd'? diff --git a/tests/specs/add/missing_npm_specifier/__test__.jsonc b/tests/specs/add/missing_npm_specifier/__test__.jsonc index 10e109c95e..3e3f8df966 100644 --- a/tests/specs/add/missing_npm_specifier/__test__.jsonc +++ b/tests/specs/add/missing_npm_specifier/__test__.jsonc @@ -2,22 +2,22 @@ "tempDir": true, "steps": [ { - "args": "add ajv@latest", + "args": "add jsr:ajv@latest", "output": "error: jsr:ajv was not found, but a matching npm package exists. Did you mean `deno add npm:ajv@latest`?\n", "exitCode": 1 }, { - "args": "add ajv", + "args": "add jsr:ajv", "output": "error: jsr:ajv was not found, but a matching npm package exists. Did you mean `deno add npm:ajv`?\n", "exitCode": 1 }, { - "args": "add ajv@8.11.0", + "args": "add jsr:ajv@8.11.0", "output": "error: jsr:ajv was not found, but a matching npm package exists. Did you mean `deno add npm:ajv@8.11.0`?\n", "exitCode": 1 }, { - "args": "install ajv@latest", + "args": "install jsr:ajv@latest", "output": "error: jsr:ajv was not found, but a matching npm package exists. Did you mean `deno install npm:ajv@latest`?\n", "exitCode": 1 } diff --git a/tests/specs/add/missing_prefix/__test__.jsonc b/tests/specs/add/missing_prefix/__test__.jsonc new file mode 100644 index 0000000000..1c97b77372 --- /dev/null +++ b/tests/specs/add/missing_prefix/__test__.jsonc @@ -0,0 +1,15 @@ +{ + "tempDir": true, + "steps": [ + { + "args": "add ajv", + "output": "error: ajv is missing a prefix. Did you mean `deno add npm:ajv`?\n", + "exitCode": 1 + }, + { + "args": "add @std/testing", + "output": "error: @std/testing is missing a prefix. Did you mean `deno add jsr:@std/testing`?\n", + "exitCode": 1 + } + ] +} diff --git a/tests/specs/add/missing_prefix/deno.json b/tests/specs/add/missing_prefix/deno.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/specs/add/no_root_export/__test__.jsonc b/tests/specs/add/no_root_export/__test__.jsonc index 2adfbd8de2..c0fa3b7fd4 100644 --- a/tests/specs/add/no_root_export/__test__.jsonc +++ b/tests/specs/add/no_root_export/__test__.jsonc @@ -2,7 +2,7 @@ "tempDir": true, "steps": [ { - "args": "add @std/testing", + "args": "add jsr:@std/testing", "output": "add.out" } ] diff --git a/tests/specs/add/package_json_and_deno_json/__test__.jsonc b/tests/specs/add/package_json_and_deno_json/__test__.jsonc index 4d886d98b3..0beee02d15 100644 --- a/tests/specs/add/package_json_and_deno_json/__test__.jsonc +++ b/tests/specs/add/package_json_and_deno_json/__test__.jsonc @@ -4,7 +4,7 @@ "npm_prefers_package_json": { "steps": [ { - "args": "add npm:@denotest/esm-basic @denotest/add npm:@denotest/say-hello", + "args": "add npm:@denotest/esm-basic jsr:@denotest/add npm:@denotest/say-hello", "output": "add.out" }, { diff --git a/tests/specs/remove/basic/__test__.jsonc b/tests/specs/remove/basic/__test__.jsonc index 495496b5cd..fd74900b44 100644 --- a/tests/specs/remove/basic/__test__.jsonc +++ b/tests/specs/remove/basic/__test__.jsonc @@ -1,7 +1,7 @@ { "tempDir": true, "steps": [{ - "args": ["add", "@std/assert", "@std/http"], + "args": ["add", "jsr:@std/assert", "jsr:@std/http"], "output": "add.out" }, { "args": ["eval", "console.log(Deno.readTextFileSync('deno.lock').trim())"],