From 2063ed7385712290d5f8e011145a58f7c95737e8 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 13 Nov 2022 10:42:15 -0500 Subject: [PATCH] feat(npm): require --unstable for npm specifiers in remote modules (#16612) --- cli/graph_util.rs | 8 ++------ cli/npm/resolution/specifier.rs | 14 +++++--------- cli/proc_state.rs | 9 +++++++++ cli/tests/integration/npm_tests.rs | 8 ++++++++ .../testdata/npm/remote_npm_specifier/main.out | 1 + .../testdata/npm/remote_npm_specifier/main.ts | 1 + .../testdata/npm/remote_npm_specifier/remote.ts | 3 +++ 7 files changed, 29 insertions(+), 15 deletions(-) create mode 100644 cli/tests/testdata/npm/remote_npm_specifier/main.out create mode 100644 cli/tests/testdata/npm/remote_npm_specifier/main.ts create mode 100644 cli/tests/testdata/npm/remote_npm_specifier/remote.ts diff --git a/cli/graph_util.rs b/cli/graph_util.rs index cfd79d4916..b919d50420 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -80,9 +80,7 @@ impl GraphData { let mut has_npm_specifier_in_graph = false; for (specifier, result) in graph.specifiers() { - if specifier.scheme() == "npm" - && NpmPackageReference::from_specifier(&specifier).is_ok() - { + if NpmPackageReference::from_specifier(&specifier).is_ok() { has_npm_specifier_in_graph = true; continue; } @@ -230,9 +228,7 @@ impl GraphData { for (dep_specifier, dep) in dependencies.iter().rev() { // todo(dsherret): ideally there would be a way to skip external dependencies // in the graph here rather than specifically npm package references - if dep_specifier.starts_with("npm:") - && NpmPackageReference::from_str(dep_specifier).is_ok() - { + if NpmPackageReference::from_str(dep_specifier).is_ok() { continue; } diff --git a/cli/npm/resolution/specifier.rs b/cli/npm/resolution/specifier.rs index c529d393b1..efcea9d993 100644 --- a/cli/npm/resolution/specifier.rs +++ b/cli/npm/resolution/specifier.rs @@ -35,10 +35,9 @@ impl NpmPackageReference { let specifier = match specifier.strip_prefix("npm:") { Some(s) => s, None => { - return Err(generic_error(format!( - "Not an npm specifier: {}", - specifier - ))); + // don't allocate a string here and instead use a static string + // because this is hit a lot when a url is not an npm specifier + return Err(generic_error("Not an npm specifier")); } }; let parts = specifier.split('/').collect::>(); @@ -244,11 +243,8 @@ pub fn resolve_npm_package_reqs(graph: &ModuleGraph) -> Vec { // fill this leaf's information for specifier in &specifiers { - if specifier.scheme() == "npm" { - // this will error elsewhere if not the case - if let Ok(npm_ref) = NpmPackageReference::from_specifier(specifier) { - leaf.reqs.insert(npm_ref.req); - } + if let Ok(npm_ref) = NpmPackageReference::from_specifier(specifier) { + leaf.reqs.insert(npm_ref.req); } else if !specifier.as_str().starts_with(&parent_specifier.as_str()) { leaf .dependencies diff --git a/cli/proc_state.rs b/cli/proc_state.rs index e50a4bdba6..1558f58659 100644 --- a/cli/proc_state.rs +++ b/cli/proc_state.rs @@ -524,6 +524,15 @@ impl ProcState { Some(Resolved::Ok { specifier, .. }) => { if let Ok(reference) = NpmPackageReference::from_specifier(specifier) { + if !self.options.unstable() + && matches!(found_referrer.scheme(), "http" | "https") + { + return Err(custom_error( + "NotSupported", + format!("importing npm specifiers in remote modules requires the --unstable flag (referrer: {})", found_referrer), + )); + } + return self .handle_node_resolve_result(node::node_resolve_npm_reference( &reference, diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs index ced9ad8a3e..07460d9906 100644 --- a/cli/tests/integration/npm_tests.rs +++ b/cli/tests/integration/npm_tests.rs @@ -179,6 +179,14 @@ itest!(sub_paths { http_server: true, }); +itest!(remote_npm_specifier { + args: "run --quiet npm/remote_npm_specifier/main.ts", + output: "npm/remote_npm_specifier/main.out", + envs: env_vars(), + http_server: true, + exit_code: 1, +}); + itest!(tarball_with_global_header { args: "run -A --quiet npm/tarball_with_global_header/main.js", output: "npm/tarball_with_global_header/main.out", diff --git a/cli/tests/testdata/npm/remote_npm_specifier/main.out b/cli/tests/testdata/npm/remote_npm_specifier/main.out new file mode 100644 index 0000000000..0cb08b7bc3 --- /dev/null +++ b/cli/tests/testdata/npm/remote_npm_specifier/main.out @@ -0,0 +1 @@ +error: importing npm specifiers in remote modules requires the --unstable flag (referrer: http://localhost:4545/npm/remote_npm_specifier/remote.ts) diff --git a/cli/tests/testdata/npm/remote_npm_specifier/main.ts b/cli/tests/testdata/npm/remote_npm_specifier/main.ts new file mode 100644 index 0000000000..20a99b6883 --- /dev/null +++ b/cli/tests/testdata/npm/remote_npm_specifier/main.ts @@ -0,0 +1 @@ +import "http://localhost:4545/npm/remote_npm_specifier/remote.ts"; diff --git a/cli/tests/testdata/npm/remote_npm_specifier/remote.ts b/cli/tests/testdata/npm/remote_npm_specifier/remote.ts new file mode 100644 index 0000000000..923ed3ed8e --- /dev/null +++ b/cli/tests/testdata/npm/remote_npm_specifier/remote.ts @@ -0,0 +1,3 @@ +import chalk from "npm:chalk"; + +console.log(chalk.green("test"));