From a013b9113d36cade5f184e73ebf2aad3e50ac9bf Mon Sep 17 00:00:00 2001 From: David Sherret Date: Wed, 5 Jun 2024 15:18:06 -0400 Subject: [PATCH] fix: support importing statically unanalyzable npm specifiers (#24107) Closes https://github.com/denoland/deno/issues/20479 Closes https://github.com/denoland/deno/issues/18744 --- cli/module_loader.rs | 151 ++++++++---------- .../__test__.jsonc | 4 + .../npm/unanalyzable_dynamic_import/main.out | 1 + .../npm/unanalyzable_dynamic_import/main.ts | 4 + 4 files changed, 78 insertions(+), 82 deletions(-) create mode 100644 tests/specs/npm/unanalyzable_dynamic_import/__test__.jsonc create mode 100644 tests/specs/npm/unanalyzable_dynamic_import/main.out create mode 100644 tests/specs/npm/unanalyzable_dynamic_import/main.ts diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 1d5fdd85f6..c134f80e19 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -469,99 +469,86 @@ impl } let graph = self.graph_container.graph(); - let maybe_resolved = match graph.get(referrer) { - Some(Module::Js(module)) => { - module.dependencies.get(specifier).map(|d| &d.maybe_code) - } - _ => None, + let resolution = match graph.get(referrer) { + Some(Module::Js(module)) => module + .dependencies + .get(specifier) + .map(|d| &d.maybe_code) + .unwrap_or(&Resolution::None), + _ => &Resolution::None, }; - match maybe_resolved { - Some(Resolution::Ok(resolved)) => { - let specifier = &resolved.specifier; - let specifier = match graph.get(specifier) { - Some(Module::Npm(module)) => { - let package_folder = self - .shared - .node_resolver - .npm_resolver - .as_managed() - .unwrap() // byonm won't create a Module::Npm - .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?; - let maybe_resolution = self - .shared - .node_resolver - .resolve_package_sub_path_from_deno_module( - &package_folder, - module.nv_reference.sub_path(), - referrer, - NodeResolutionMode::Execution, - permissions, - ) - .with_context(|| { - format!("Could not resolve '{}'.", module.nv_reference) - })?; - match maybe_resolution { - Some(res) => res.into_url(), - None => return Err(generic_error("not found")), - } - } - Some(Module::Node(module)) => module.specifier.clone(), - Some(Module::Js(module)) => module.specifier.clone(), - Some(Module::Json(module)) => module.specifier.clone(), - Some(Module::External(module)) => { - node::resolve_specifier_into_node_modules(&module.specifier) - } - None => specifier.clone(), - }; - return Ok(specifier); - } - Some(Resolution::Err(err)) => { + let specifier = match resolution { + Resolution::Ok(resolved) => Cow::Borrowed(&resolved.specifier), + Resolution::Err(err) => { return Err(custom_error( "TypeError", format!("{}\n", err.to_string_with_range()), - )) + )); } - Some(Resolution::None) | None => {} - } - - // FIXME(bartlomieju): this is another hack way to provide NPM specifier - // support in REPL. This should be fixed. - let resolution = self.shared.resolver.resolve( - specifier, - &deno_graph::Range { - specifier: referrer.clone(), - start: deno_graph::Position::zeroed(), - end: deno_graph::Position::zeroed(), - }, - ResolutionMode::Execution, - ); + Resolution::None => Cow::Owned(self.shared.resolver.resolve( + specifier, + &deno_graph::Range { + specifier: referrer.clone(), + start: deno_graph::Position::zeroed(), + end: deno_graph::Position::zeroed(), + }, + ResolutionMode::Execution, + )?), + }; if self.shared.is_repl { - let specifier = resolution - .as_ref() - .ok() - .map(Cow::Borrowed) - .or_else(|| ModuleSpecifier::parse(specifier).ok().map(Cow::Owned)); - if let Some(specifier) = specifier { - if let Ok(reference) = - NpmPackageReqReference::from_specifier(&specifier) - { - return self - .shared - .node_resolver - .resolve_req_reference( - &reference, - permissions, - referrer, - NodeResolutionMode::Execution, - ) - .map(|res| res.into_url()); - } + if let Ok(reference) = NpmPackageReqReference::from_specifier(&specifier) + { + return self + .shared + .node_resolver + .resolve_req_reference( + &reference, + permissions, + referrer, + NodeResolutionMode::Execution, + ) + .map(|res| res.into_url()); } } - resolution.map_err(|err| err.into()) + let specifier = match graph.get(&specifier) { + Some(Module::Npm(module)) => { + let package_folder = self + .shared + .node_resolver + .npm_resolver + .as_managed() + .unwrap() // byonm won't create a Module::Npm + .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?; + let maybe_resolution = self + .shared + .node_resolver + .resolve_package_sub_path_from_deno_module( + &package_folder, + module.nv_reference.sub_path(), + referrer, + NodeResolutionMode::Execution, + permissions, + ) + .with_context(|| { + format!("Could not resolve '{}'.", module.nv_reference) + })?; + match maybe_resolution { + Some(res) => res.into_url(), + None => return Err(generic_error("not found")), + } + } + Some(Module::Node(module)) => module.specifier.clone(), + Some(Module::Js(module)) => module.specifier.clone(), + Some(Module::Json(module)) => module.specifier.clone(), + Some(Module::External(module)) => { + node::resolve_specifier_into_node_modules(&module.specifier) + } + None => specifier.into_owned(), + }; + Ok(specifier) } async fn load_prepared_module( diff --git a/tests/specs/npm/unanalyzable_dynamic_import/__test__.jsonc b/tests/specs/npm/unanalyzable_dynamic_import/__test__.jsonc new file mode 100644 index 0000000000..0ef1472536 --- /dev/null +++ b/tests/specs/npm/unanalyzable_dynamic_import/__test__.jsonc @@ -0,0 +1,4 @@ +{ + "args": "run -A --quiet main.ts", + "output": "main.out" +} diff --git a/tests/specs/npm/unanalyzable_dynamic_import/main.out b/tests/specs/npm/unanalyzable_dynamic_import/main.out new file mode 100644 index 0000000000..00750edc07 --- /dev/null +++ b/tests/specs/npm/unanalyzable_dynamic_import/main.out @@ -0,0 +1 @@ +3 diff --git a/tests/specs/npm/unanalyzable_dynamic_import/main.ts b/tests/specs/npm/unanalyzable_dynamic_import/main.ts new file mode 100644 index 0000000000..793833ee26 --- /dev/null +++ b/tests/specs/npm/unanalyzable_dynamic_import/main.ts @@ -0,0 +1,4 @@ +const specifier = "npm:@denotest/add"; +const { add } = await import(specifier); + +console.log(add(1, 2));