From 3eaf174bfc64b7c277899abd44ae3877538028df Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 5 Mar 2024 19:23:51 -0500 Subject: [PATCH] fix(node): improve cjs tracking (#22673) We were missing saying that a file is CJS when some Deno code imported from the node_modules directory at runtime. --- cli/cache/mod.rs | 4 +- cli/factory.rs | 7 +- cli/lsp/analysis.rs | 6 +- cli/lsp/documents.rs | 40 +--- cli/lsp/language_server.rs | 27 ++- cli/module_loader.rs | 33 ++- cli/resolver.rs | 224 +++++++++++------- cli/standalone/mod.rs | 38 ++- cli/tools/vendor/test.rs | 3 - tests/integration/npm_tests.rs | 27 ++- .../import_not_defined.out | 5 +- .../sub_path_import_not_defined.out | 5 +- 12 files changed, 250 insertions(+), 169 deletions(-) diff --git a/cli/cache/mod.rs b/cli/cache/mod.rs index 7d95a6423e..229a9cb544 100644 --- a/cli/cache/mod.rs +++ b/cli/cache/mod.rs @@ -196,7 +196,9 @@ impl Loader for FetchCacher { ) -> LoadFuture { use deno_graph::source::CacheSetting as LoaderCacheSetting; - if specifier.path().contains("/node_modules/") { + if specifier.scheme() == "file" + && specifier.path().contains("/node_modules/") + { // The specifier might be in a completely different symlinked tree than // what the node_modules url is in (ex. `/my-project-1/node_modules` // symlinked to `/my-project-2/node_modules`), so first we checked if the path diff --git a/cli/factory.rs b/cli/factory.rs index 54ec6ac5ec..eb025a5585 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -483,14 +483,12 @@ impl CliFactory { .get_or_try_init_async( async { Ok(Arc::new(CliGraphResolver::new(CliGraphResolverOptions { - fs: self.fs().clone(), - cjs_resolutions: Some(self.cjs_resolutions().clone()), sloppy_imports_resolver: if self.options.unstable_sloppy_imports() { Some(SloppyImportsResolver::new(self.fs().clone())) } else { None }, - node_resolver: Some(self.node_resolver().await?.clone()), + node_resolver: Some(self.cli_node_resolver().await?.clone()), npm_resolver: if self.options.no_npm() { None } else { @@ -714,7 +712,8 @@ impl CliFactory { .cli_node_resolver .get_or_try_init_async(async { Ok(Arc::new(CliNodeResolver::new( - self.cjs_resolutions().clone(), + Some(self.cjs_resolutions().clone()), + self.fs().clone(), self.node_resolver().await?.clone(), self.npm_resolver().await?.clone(), ))) diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 3dd78e428a..71f07275ef 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -8,6 +8,7 @@ use super::tsc; use crate::args::jsr_url; use crate::npm::CliNpmResolver; +use crate::resolver::CliNodeResolver; use crate::tools::lint::create_linter; use crate::util::path::specifier_to_file_path; @@ -23,7 +24,6 @@ use deno_core::serde_json::json; use deno_core::ModuleSpecifier; use deno_lint::diagnostic::LintDiagnostic; use deno_lint::rules::LintRule; -use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_node::NpmResolver; use deno_runtime::deno_node::PathClean; use deno_runtime::permissions::PermissionsContainer; @@ -179,7 +179,7 @@ fn code_as_string(code: &Option) -> String { pub struct TsResponseImportMapper<'a> { documents: &'a Documents, maybe_import_map: Option<&'a ImportMap>, - node_resolver: Option<&'a NodeResolver>, + node_resolver: Option<&'a CliNodeResolver>, npm_resolver: Option<&'a dyn CliNpmResolver>, } @@ -187,7 +187,7 @@ impl<'a> TsResponseImportMapper<'a> { pub fn new( documents: &'a Documents, maybe_import_map: Option<&'a ImportMap>, - node_resolver: Option<&'a NodeResolver>, + node_resolver: Option<&'a CliNodeResolver>, npm_resolver: Option<&'a dyn CliNpmResolver>, ) -> Self { Self { diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 21a613bfaa..b825bc0201 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -19,6 +19,7 @@ use crate::lsp::logging::lsp_warn; use crate::npm::CliNpmResolver; use crate::resolver::CliGraphResolver; use crate::resolver::CliGraphResolverOptions; +use crate::resolver::CliNodeResolver; use crate::resolver::SloppyImportsFsEntry; use crate::resolver::SloppyImportsResolution; use crate::resolver::SloppyImportsResolver; @@ -40,11 +41,9 @@ use deno_graph::source::ResolutionMode; use deno_graph::GraphImport; use deno_graph::Resolution; use deno_lockfile::Lockfile; -use deno_runtime::deno_fs::RealFs; use deno_runtime::deno_node; use deno_runtime::deno_node::NodeResolution; use deno_runtime::deno_node::NodeResolutionMode; -use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_node::PackageJson; use deno_runtime::permissions::PermissionsContainer; use deno_semver::jsr::JsrPackageReqReference; @@ -835,7 +834,7 @@ pub struct UpdateDocumentConfigOptions<'a> { pub maybe_config_file: Option<&'a ConfigFile>, pub maybe_package_json: Option<&'a PackageJson>, pub maybe_lockfile: Option>>, - pub node_resolver: Option>, + pub node_resolver: Option>, pub npm_resolver: Option>, } @@ -897,10 +896,8 @@ impl Documents { resolver_config_hash: 0, imports: Default::default(), resolver: Arc::new(CliGraphResolver::new(CliGraphResolverOptions { - fs: Arc::new(RealFs), node_resolver: None, npm_resolver: None, - cjs_resolutions: None, package_json_deps_provider: Arc::new(PackageJsonDepsProvider::default()), maybe_jsx_import_source_config: None, maybe_import_map: None, @@ -1273,7 +1270,7 @@ impl Documents { NpmPackageReqReference::from_str(&specifier) { results.push(node_resolve_npm_req_ref( - npm_req_ref, + &npm_req_ref, maybe_npm, referrer, )); @@ -1408,12 +1405,9 @@ impl Documents { ); let deps_provider = Arc::new(PackageJsonDepsProvider::new(maybe_package_json_deps)); - let fs = Arc::new(RealFs); self.resolver = Arc::new(CliGraphResolver::new(CliGraphResolverOptions { - fs: fs.clone(), node_resolver: options.node_resolver, npm_resolver: options.npm_resolver, - cjs_resolutions: None, // only used for runtime package_json_deps_provider: deps_provider, maybe_jsx_import_source_config: maybe_jsx_config, maybe_import_map: options.maybe_import_map, @@ -1693,7 +1687,7 @@ impl Documents { } if let Ok(npm_ref) = NpmPackageReqReference::from_specifier(specifier) { - return node_resolve_npm_req_ref(npm_ref, maybe_npm, referrer); + return node_resolve_npm_req_ref(&npm_ref, maybe_npm, referrer); } let doc = self.get(specifier)?; let maybe_module = doc.maybe_js_module().and_then(|r| r.as_ref().ok()); @@ -1724,29 +1718,21 @@ impl Documents { } fn node_resolve_npm_req_ref( - npm_req_ref: NpmPackageReqReference, + npm_req_ref: &NpmPackageReqReference, maybe_npm: Option<&StateNpmSnapshot>, referrer: &ModuleSpecifier, ) -> Option<(ModuleSpecifier, MediaType)> { maybe_npm.map(|npm| { NodeResolution::into_specifier_and_media_type( npm - .npm_resolver - .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req(), referrer) - .ok() - .and_then(|package_folder| { - npm - .node_resolver - .resolve_package_subpath_from_deno_module( - &package_folder, - npm_req_ref.sub_path(), - referrer, - NodeResolutionMode::Types, - &PermissionsContainer::allow_all(), - ) - .ok() - .flatten() - }), + .node_resolver + .resolve_req_reference( + npm_req_ref, + &PermissionsContainer::allow_all(), + referrer, + NodeResolutionMode::Types, + ) + .ok(), ) }) } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index b108eb54ea..c22752e9ed 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -121,6 +121,7 @@ use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedCreateOptions; use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption; use crate::npm::CliNpmResolverManagedSnapshotOption; +use crate::resolver::CliNodeResolver; use crate::tools::fmt::format_file; use crate::tools::fmt::format_parsed_source; use crate::tools::upgrade::check_for_upgrades_for_lsp; @@ -146,7 +147,7 @@ struct LspNpmServices { /// Npm's search api. search_api: CliNpmSearchApi, /// Node resolver. - node_resolver: Option>, + node_resolver: Option>, /// Resolver for npm packages. resolver: Option>, } @@ -171,7 +172,7 @@ pub struct LanguageServer(Arc>, CancellationToken); #[derive(Clone, Debug)] pub struct StateNpmSnapshot { - pub node_resolver: Arc, + pub node_resolver: Arc, pub npm_resolver: Arc, } @@ -760,10 +761,18 @@ impl Inner { .map(|resolver| resolver.clone_snapshotted()) .map(|resolver| { let fs = Arc::new(deno_fs::RealFs); - let node_resolver = - Arc::new(NodeResolver::new(fs, resolver.clone().into_npm_resolver())); - StateNpmSnapshot { + let node_resolver = Arc::new(NodeResolver::new( + fs.clone(), + resolver.clone().into_npm_resolver(), + )); + let cli_node_resolver = Arc::new(CliNodeResolver::new( + None, + fs, node_resolver, + resolver.clone(), + )); + StateNpmSnapshot { + node_resolver: cli_node_resolver, npm_resolver: resolver, } }); @@ -907,9 +916,15 @@ impl Inner { self.config.maybe_node_modules_dir_path().cloned(), ) .await; - self.npm.node_resolver = Some(Arc::new(NodeResolver::new( + let node_resolver = Arc::new(NodeResolver::new( Arc::new(deno_fs::RealFs), npm_resolver.clone().into_npm_resolver(), + )); + self.npm.node_resolver = Some(Arc::new(CliNodeResolver::new( + None, + Arc::new(deno_fs::RealFs), + node_resolver, + npm_resolver.clone(), ))); self.npm.resolver = Some(npm_resolver); diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 0951a287c6..17ec535f95 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -52,6 +52,7 @@ use deno_graph::Module; use deno_graph::Resolution; use deno_lockfile::Lockfile; use deno_runtime::deno_fs; +use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use deno_terminal::colors; @@ -513,9 +514,13 @@ impl CliModuleLoader { if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( specifier, referrer, + NodeResolutionMode::Execution, permissions, ) { - return result; + return match result? { + Some(res) => Ok(res.into_url()), + None => Err(generic_error("not found")), + }; } let graph = self.shared.graph_container.graph(); @@ -538,18 +543,23 @@ impl CliModuleLoader { .as_managed() .unwrap() // byonm won't create a Module::Npm .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?; - self + let maybe_resolution = self .shared .node_resolver - .resolve_package_sub_path( + .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(), @@ -592,11 +602,16 @@ impl CliModuleLoader { if let Ok(reference) = NpmPackageReqReference::from_specifier(&specifier) { - return self.shared.node_resolver.resolve_req_reference( - &reference, - permissions, - referrer, - ); + return self + .shared + .node_resolver + .resolve_req_reference( + &reference, + permissions, + referrer, + NodeResolutionMode::Execution, + ) + .map(|res| res.into_url()); } } } diff --git a/cli/resolver.rs b/cli/resolver.rs index de85992a70..e1a2145d3b 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -3,7 +3,6 @@ use deno_ast::MediaType; use deno_core::anyhow::anyhow; use deno_core::anyhow::Context; -use deno_core::error::generic_error; use deno_core::error::AnyError; use deno_core::futures::future; use deno_core::futures::future::LocalBoxFuture; @@ -22,10 +21,12 @@ use deno_runtime::deno_fs; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::is_builtin_node_module; use deno_runtime::deno_node::parse_npm_pkg_name; +use deno_runtime::deno_node::NodePermissions; use deno_runtime::deno_node::NodeResolution; use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_node::NpmResolver as DenoNodeNpmResolver; +use deno_runtime::deno_node::PackageJson; use deno_runtime::permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; @@ -63,20 +64,26 @@ pub struct ModuleCodeStringSource { pub media_type: MediaType, } +#[derive(Debug)] pub struct CliNodeResolver { - cjs_resolutions: Arc, + // not used in the LSP + cjs_resolutions: Option>, + fs: Arc, node_resolver: Arc, + // todo(dsherret): remove this pub(crate) pub(crate) npm_resolver: Arc, } impl CliNodeResolver { pub fn new( - cjs_resolutions: Arc, + cjs_resolutions: Option>, + fs: Arc, node_resolver: Arc, npm_resolver: Arc, ) -> Self { Self { cjs_resolutions, + fs, node_resolver, npm_resolver, } @@ -86,81 +93,150 @@ impl CliNodeResolver { self.npm_resolver.in_npm_package(referrer) } + pub fn get_closest_package_json( + &self, + referrer: &ModuleSpecifier, + permissions: &dyn NodePermissions, + ) -> Result, AnyError> { + self + .node_resolver + .get_closest_package_json(referrer, permissions) + } + pub fn resolve_if_in_npm_package( &self, specifier: &str, referrer: &ModuleSpecifier, + mode: NodeResolutionMode, permissions: &PermissionsContainer, - ) -> Option> { + ) -> Option, AnyError>> { if self.in_npm_package(referrer) { // we're in an npm package, so use node resolution - Some( - self - .handle_node_resolve_result(self.node_resolver.resolve( - specifier, - referrer, - NodeResolutionMode::Execution, - permissions, - )) - .with_context(|| { - format!("Could not resolve '{specifier}' from '{referrer}'.") - }), - ) + Some(self.resolve(specifier, referrer, mode, permissions)) } else { None } } + pub fn resolve( + &self, + specifier: &str, + referrer: &ModuleSpecifier, + mode: NodeResolutionMode, + permissions: &PermissionsContainer, + ) -> Result, AnyError> { + self.handle_node_resolve_result(self.node_resolver.resolve( + specifier, + referrer, + mode, + permissions, + )) + } + pub fn resolve_req_reference( &self, req_ref: &NpmPackageReqReference, permissions: &PermissionsContainer, referrer: &ModuleSpecifier, - ) -> Result { + mode: NodeResolutionMode, + ) -> Result { let package_folder = self .npm_resolver .resolve_pkg_folder_from_deno_module_req(req_ref.req(), referrer)?; - self - .resolve_package_sub_path( - &package_folder, - req_ref.sub_path(), - referrer, - permissions, - ) - .with_context(|| format!("Could not resolve '{}'.", req_ref)) + let maybe_resolution = self.resolve_package_sub_path_from_deno_module( + &package_folder, + req_ref.sub_path(), + referrer, + mode, + permissions, + )?; + match maybe_resolution { + Some(resolution) => Ok(resolution), + None => { + if self.npm_resolver.as_byonm().is_some() { + let package_json_path = package_folder.join("package.json"); + if !self.fs.exists_sync(&package_json_path) { + return Err(anyhow!( + "Could not find '{}'. Deno expects the node_modules/ directory to be up to date. Did you forget to run `npm install`?", + package_json_path.display() + )); + } + } + Err(anyhow!( + "Failed resolving package subpath for '{}' in '{}'.", + req_ref, + package_folder.display() + )) + } + } } - pub fn resolve_package_sub_path( + pub fn resolve_package_sub_path_from_deno_module( &self, package_folder: &Path, sub_path: Option<&str>, referrer: &ModuleSpecifier, + mode: NodeResolutionMode, permissions: &PermissionsContainer, - ) -> Result { + ) -> Result, AnyError> { self.handle_node_resolve_result( self.node_resolver.resolve_package_subpath_from_deno_module( package_folder, sub_path, referrer, - NodeResolutionMode::Execution, + mode, permissions, ), ) } + pub fn handle_if_in_node_modules( + &self, + specifier: ModuleSpecifier, + ) -> Result { + // skip canonicalizing if we definitely know it's unnecessary + if specifier.scheme() == "file" + && specifier.path().contains("/node_modules/") + { + // Specifiers in the node_modules directory are canonicalized + // so canoncalize then check if it's in the node_modules directory. + // If so, check if we need to store this specifier as being a CJS + // resolution. + let specifier = + crate::node::resolve_specifier_into_node_modules(&specifier); + if self.in_npm_package(&specifier) { + if let Some(cjs_resolutions) = &self.cjs_resolutions { + let resolution = + self.node_resolver.url_to_node_resolution(specifier)?; + if let NodeResolution::CommonJs(specifier) = &resolution { + cjs_resolutions.insert(specifier.clone()); + } + return Ok(resolution.into_url()); + } else { + return Ok(specifier); + } + } + } + + Ok(specifier) + } + fn handle_node_resolve_result( &self, result: Result, AnyError>, - ) -> Result { - let response = match result? { - Some(response) => response, - None => return Err(generic_error("not found")), - }; - if let NodeResolution::CommonJs(specifier) = &response { - // remember that this was a common js resolution - self.cjs_resolutions.insert(specifier.clone()); + ) -> Result, AnyError> { + match result? { + Some(response) => { + if let NodeResolution::CommonJs(specifier) = &response { + // remember that this was a common js resolution + if let Some(cjs_resolutions) = &self.cjs_resolutions { + cjs_resolutions.insert(specifier.clone()); + } + } + Ok(Some(response)) + } + None => Ok(None), } - Ok(response.into_url()) } } @@ -359,24 +435,20 @@ impl MappedSpecifierResolver { /// import map, JSX settings. #[derive(Debug)] pub struct CliGraphResolver { - fs: Arc, sloppy_imports_resolver: Option, mapped_specifier_resolver: MappedSpecifierResolver, maybe_default_jsx_import_source: Option, maybe_jsx_import_source_module: Option, maybe_vendor_specifier: Option, - cjs_resolutions: Option>, - node_resolver: Option>, + node_resolver: Option>, npm_resolver: Option>, found_package_json_dep_flag: Arc, bare_node_builtins_enabled: bool, } pub struct CliGraphResolverOptions<'a> { - pub fs: Arc, - pub cjs_resolutions: Option>, pub sloppy_imports_resolver: Option, - pub node_resolver: Option>, + pub node_resolver: Option>, pub npm_resolver: Option>, pub package_json_deps_provider: Arc, pub maybe_jsx_import_source_config: Option, @@ -393,8 +465,6 @@ impl CliGraphResolver { .map(|n| n.as_byonm().is_some()) .unwrap_or(false); Self { - fs: options.fs, - cjs_resolutions: options.cjs_resolutions, sloppy_imports_resolver: options.sloppy_imports_resolver, mapped_specifier_resolver: MappedSpecifierResolver::new( options.maybe_import_map, @@ -496,7 +566,7 @@ impl Resolver for CliGraphResolver { } let referrer = &referrer_range.specifier; - let result = self + let result: Result<_, ResolveError> = self .mapped_specifier_resolver .resolve(specifier, referrer) .map_err(|err| err.into()) @@ -549,46 +619,16 @@ impl Resolver for CliGraphResolver { if let Ok(npm_req_ref) = NpmPackageReqReference::from_specifier(specifier) { - let package_folder = resolver - .resolve_pkg_folder_from_deno_module_req( - npm_req_ref.req(), - referrer, - )?; let node_resolver = self.node_resolver.as_ref().unwrap(); - let package_json_path = package_folder.join("package.json"); - if !self.fs.exists_sync(&package_json_path) { - return Err(ResolveError::Other(anyhow!( - "Could not find '{}'. Deno expects the node_modules/ directory to be up to date. Did you forget to run `npm install`?", - package_json_path.display() - ))); - } - let maybe_resolution = node_resolver - .resolve_package_subpath_from_deno_module( - &package_folder, - npm_req_ref.sub_path(), + return node_resolver + .resolve_req_reference( + &npm_req_ref, + &PermissionsContainer::allow_all(), referrer, to_node_mode(mode), - &PermissionsContainer::allow_all(), - )?; - match maybe_resolution { - Some(resolution) => { - if let Some(cjs_resolutions) = &self.cjs_resolutions { - if let NodeResolution::CommonJs(specifier) = &resolution { - // remember that this was a common js resolution - cjs_resolutions.insert(specifier.clone()); - } - } - - return Ok(resolution.into_url()); - } - None => { - return Err(ResolveError::Other(anyhow!( - "Failed resolving package subpath for '{}' in '{}'.", - npm_req_ref, - package_folder.display() - ))); - } - } + ) + .map(|res| res.into_url()) + .map_err(|err| err.into()); } } Err(_) => { @@ -601,14 +641,8 @@ impl Resolver for CliGraphResolver { &PermissionsContainer::allow_all(), ); match node_result { - Ok(Some(resolution)) => { - if let Some(cjs_resolutions) = &self.cjs_resolutions { - if let NodeResolution::CommonJs(specifier) = &resolution { - // remember that this was a common js resolution - cjs_resolutions.insert(specifier.clone()); - } - } - return Ok(resolution.into_url()); + Ok(Some(res)) => { + return Ok(res.into_url()); } Ok(None) => { self @@ -639,7 +673,13 @@ impl Resolver for CliGraphResolver { } } - result + let specifier = result?; + match &self.node_resolver { + Some(node_resolver) => node_resolver + .handle_if_in_node_modules(specifier) + .map_err(|e| e.into()), + None => Ok(specifier), + } } } diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 312a1841db..9dff56af11 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -44,6 +44,7 @@ use deno_core::RequestedModuleType; use deno_core::ResolutionKind; use deno_runtime::deno_fs; use deno_runtime::deno_node::analyze::NodeCodeTranslator; +use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::NodeResolver; use deno_runtime::deno_tls::rustls::RootCertStore; use deno_runtime::deno_tls::RootCertStoreProvider; @@ -111,9 +112,13 @@ impl ModuleLoader for EmbeddedModuleLoader { if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( specifier, &referrer, + NodeResolutionMode::Execution, permissions, ) { - return result; + return match result? { + Some(res) => Ok(res.into_url()), + None => Err(generic_error("not found")), + }; } let maybe_mapped = self @@ -128,18 +133,26 @@ impl ModuleLoader for EmbeddedModuleLoader { .map(|r| r.as_str()) .unwrap_or(specifier); if let Ok(reference) = NpmPackageReqReference::from_str(specifier_text) { - return self.shared.node_resolver.resolve_req_reference( - &reference, - permissions, - &referrer, - ); + return self + .shared + .node_resolver + .resolve_req_reference( + &reference, + permissions, + &referrer, + NodeResolutionMode::Execution, + ) + .map(|res| res.into_url()); } - match maybe_mapped { - Some(resolved) => Ok(resolved), - None => deno_core::resolve_import(specifier, referrer.as_str()) - .map_err(|err| err.into()), - } + let specifier = match maybe_mapped { + Some(resolved) => resolved, + None => deno_core::resolve_import(specifier, referrer.as_str())?, + }; + self + .shared + .node_resolver + .handle_if_in_node_modules(specifier) } fn load( @@ -452,7 +465,8 @@ pub async fn run( Arc::new(parse_from_json(&base, &source).unwrap().import_map) }); let cli_node_resolver = Arc::new(CliNodeResolver::new( - cjs_resolutions.clone(), + Some(cjs_resolutions.clone()), + fs.clone(), node_resolver.clone(), npm_resolver.clone(), )); diff --git a/cli/tools/vendor/test.rs b/cli/tools/vendor/test.rs index 6a960c302e..c5e98c05e4 100644 --- a/cli/tools/vendor/test.rs +++ b/cli/tools/vendor/test.rs @@ -20,7 +20,6 @@ use deno_graph::source::Loader; use deno_graph::DefaultModuleAnalyzer; use deno_graph::GraphKind; use deno_graph::ModuleGraph; -use deno_runtime::deno_fs::RealFs; use import_map::ImportMap; use crate::args::JsxImportSourceConfig; @@ -295,10 +294,8 @@ fn build_resolver( original_import_map: Option, ) -> CliGraphResolver { CliGraphResolver::new(CliGraphResolverOptions { - fs: Arc::new(RealFs), node_resolver: None, npm_resolver: None, - cjs_resolutions: None, sloppy_imports_resolver: None, package_json_deps_provider: Default::default(), maybe_jsx_import_source_config, diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index 33e331fc3d..e4e06e8302 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -2611,10 +2611,7 @@ fn cjs_rexport_analysis_json() { let dir = test_context.temp_dir(); dir.write("deno.json", r#"{ "unstable": [ "byonm" ] }"#); - dir.write( - "package.json", - r#"{ "name": "test", "packages": { "my-package": "1.0.0" } }"#, - ); + dir.write("package.json", r#"{ "name": "test" }"#); dir.write( "main.js", "import data from 'my-package';\nconsole.log(data);\n", @@ -2670,6 +2667,28 @@ fn cjs_rexport_analysis_json() { ); } +#[test] +fn cjs_export_analysis_import_cjs_directly_relative_import() { + let test_context = TestContextBuilder::for_npm().use_temp_cwd().build(); + let dir = test_context.temp_dir(); + dir.write("deno.json", r#"{ "unstable": [ "byonm" ] }"#); + + dir.write( + "package.json", + r#"{ "name": "test", "dependencies": { "@denotest/cjs-default-export": "1.0.0" } }"#, + ); + // previously it wasn't doing cjs export analysis on this file + dir.write( + "main.ts", + "import { named } from './node_modules/@denotest/cjs-default-export/index.js';\nconsole.log(named());\n", + ); + + test_context.run_npm("install"); + + let output = test_context.new_command().args("run main.ts").run(); + output.assert_matches_text("2\n"); +} + itest!(imports_package_json { args: "run --node-modules-dir=false npm/imports_package_json/main.js", output: "npm/imports_package_json/main.out", diff --git a/tests/testdata/npm/imports_package_json/import_not_defined.out b/tests/testdata/npm/imports_package_json/import_not_defined.out index 3580d9007a..4cf72ef821 100644 --- a/tests/testdata/npm/imports_package_json/import_not_defined.out +++ b/tests/testdata/npm/imports_package_json/import_not_defined.out @@ -1,6 +1,3 @@ Download http://localhost:4545/npm/registry/@denotest/imports-package-json Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz -error: Could not resolve '#not-defined' from 'file:///[WILDCARD]/@denotest/imports-package-json/1.0.0/import_not_defined.js'. - -Caused by: - [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#not-defined" is not defined in package [WILDCARD]package.json imported from [WILDCARD]import_not_defined.js +error: [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#not-defined" is not defined in package [WILDCARD]package.json imported from [WILDCARD]import_not_defined.js diff --git a/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out b/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out index 04a21c99eb..1ce2a4c0ec 100644 --- a/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out +++ b/tests/testdata/npm/imports_package_json/sub_path_import_not_defined.out @@ -1,6 +1,3 @@ Download http://localhost:4545/npm/registry/@denotest/imports-package-json Download http://localhost:4545/npm/registry/@denotest/imports-package-json/1.0.0.tgz -error: Could not resolve '#hi' from 'file:///[WILDCARD]/@denotest/imports-package-json/1.0.0/sub_path/import_not_defined.js'. - -Caused by: - [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#hi" is not defined in package [WILDCARD]sub_path[WILDCARD]package.json imported from [WILDCARD]import_not_defined.js +error: [ERR_PACKAGE_IMPORT_NOT_DEFINED] Package import specifier "#hi" is not defined in package [WILDCARD]sub_path[WILDCARD]package.json imported from [WILDCARD]import_not_defined.js