From d43e48c4e96b02289d505cd2558ba85d7d6cb57b Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 28 Sep 2023 16:43:45 -0400 Subject: [PATCH] refactor(ext/node): remove dependency on deno_npm and deno_semver (#20718) This is required from BYONM (bring your own node_modules). Part of #18967 --- Cargo.lock | 2 - Cargo.toml | 2 - cli/Cargo.toml | 4 +- cli/factory.rs | 4 +- cli/lsp/analysis.rs | 7 +- cli/lsp/diagnostics.rs | 19 +- cli/lsp/documents.rs | 52 ++-- cli/lsp/language_server.rs | 15 +- cli/lsp/tsc.rs | 12 +- cli/module_loader.rs | 22 +- cli/npm/installer.rs | 2 +- cli/npm/resolution.rs | 6 +- cli/npm/resolvers/mod.rs | 53 ++-- cli/resolver.rs | 4 +- cli/standalone/mod.rs | 3 +- .../npm/deno_run_no_bin_entrypoint.out | 2 +- ...no_bin_entrypoint_non_existent_subpath.out | 2 +- cli/tools/check.rs | 5 +- cli/tools/task.rs | 4 +- cli/tsc/mod.rs | 58 ++-- cli/worker.rs | 34 ++- ext/node/Cargo.toml | 2 - ext/node/lib.rs | 15 - ext/node/package_json.rs | 6 + ext/node/resolution.rs | 288 ++++++++++-------- 25 files changed, 360 insertions(+), 263 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index de2a9c3d20..192f522fcb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1526,8 +1526,6 @@ dependencies = [ "deno_fs", "deno_media_type", "deno_net", - "deno_npm", - "deno_semver 0.5.1", "digest 0.10.7", "dsa", "ecb", diff --git a/Cargo.toml b/Cargo.toml index 7a4ac8ab42..c978464d9f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,8 +48,6 @@ deno_bench_util = { version = "0.114.0", path = "./bench_util" } test_util = { path = "./test_util" } deno_lockfile = "0.17.1" deno_media_type = { version = "0.1.1", features = ["module_specifier"] } -deno_npm = "0.15.1" -deno_semver = "0.5.0" # exts deno_broadcast_channel = { version = "0.114.0", path = "./ext/broadcast_channel" } diff --git a/cli/Cargo.toml b/cli/Cargo.toml index e4ac49842b..cb150e5db8 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -54,9 +54,9 @@ deno_emit = "=0.28.0" deno_graph = "=0.55.0" deno_lint = { version = "=0.51.0", features = ["docs"] } deno_lockfile.workspace = true -deno_npm.workspace = true +deno_npm = "0.15.1" deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "exclude_runtime_main_js", "include_js_files_for_snapshotting"] } -deno_semver.workspace = true +deno_semver = "0.5.1" deno_task_shell = "=0.13.2" eszip = "=0.53.0" napi_sym.workspace = true diff --git a/cli/factory.rs b/cli/factory.rs index 28788093b1..6acf248a88 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -623,10 +623,11 @@ impl CliFactory { &self, ) -> Result { let node_resolver = self.node_resolver().await?; + let npm_resolver = self.npm_resolver().await?; let fs = self.fs(); Ok(CliMainWorkerFactory::new( StorageKeyResolver::from_options(&self.options), - self.npm_resolver().await?.clone(), + npm_resolver.clone(), node_resolver.clone(), self.blob_store().clone(), Box::new(CliModuleLoaderFactory::new( @@ -641,6 +642,7 @@ impl CliFactory { self.node_code_translator().await?.clone(), fs.clone(), node_resolver.clone(), + npm_resolver.clone(), ), )), self.root_cert_store_provider().clone(), diff --git a/cli/lsp/analysis.rs b/cli/lsp/analysis.rs index 7a0c11212a..1b11deca89 100644 --- a/cli/lsp/analysis.rs +++ b/cli/lsp/analysis.rs @@ -199,9 +199,8 @@ impl<'a> TsResponseImportMapper<'a> { } if self.npm_resolver.in_npm_package(specifier) { - if let Ok(Some(pkg_id)) = self - .npm_resolver - .resolve_package_id_from_specifier(specifier) + if let Ok(Some(pkg_id)) = + self.npm_resolver.resolve_pkg_id_from_specifier(specifier) { let pkg_reqs = self.npm_resolution.resolve_pkg_reqs_from_pkg_id(&pkg_id); @@ -254,7 +253,7 @@ impl<'a> TsResponseImportMapper<'a> { let specifier_path = specifier.to_file_path().ok()?; let root_folder = self .npm_resolver - .resolve_package_folder_from_specifier(specifier) + .resolve_pkg_folder_from_specifier(specifier) .ok() .flatten()?; let package_json_path = root_folder.join("package.json"); diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 43ae35f54b..183901fb0d 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -800,8 +800,8 @@ fn generate_lint_diagnostics( } // ignore any npm package files - if let Some(node_resolver) = &snapshot.maybe_node_resolver { - if node_resolver.in_npm_package(document.specifier()) { + if let Some(npm) = &snapshot.npm { + if npm.node_resolver.in_npm_package(document.specifier()) { continue; } } @@ -1237,10 +1237,10 @@ fn diagnose_resolution( } else if let Ok(pkg_ref) = NpmPackageReqReference::from_specifier(specifier) { - if let Some(npm_resolver) = &snapshot.maybe_npm_resolver { + if let Some(npm) = &snapshot.npm { // show diagnostics for npm package references that aren't cached let req = pkg_ref.into_inner().req; - if !npm_resolver.is_pkg_req_folder_cached(&req) { + if !npm.npm_resolver.is_pkg_req_folder_cached(&req) { diagnostics .push(DenoDiagnostic::NoCacheNpm(req, specifier.clone())); } @@ -1250,10 +1250,10 @@ fn diagnose_resolution( if !deno_node::is_builtin_node_module(module_name) { diagnostics .push(DenoDiagnostic::InvalidNodeSpecifier(specifier.clone())); - } else if let Some(npm_resolver) = &snapshot.maybe_npm_resolver { + } else if let Some(npm) = &snapshot.npm { // check that a @types/node package exists in the resolver let types_node_req = PackageReq::from_str("@types/node").unwrap(); - if !npm_resolver.is_pkg_req_folder_cached(&types_node_req) { + if !npm.npm_resolver.is_pkg_req_folder_cached(&types_node_req) { diagnostics.push(DenoDiagnostic::NoCacheNpm( types_node_req, ModuleSpecifier::parse("npm:@types/node").unwrap(), @@ -1291,8 +1291,8 @@ fn diagnose_dependency( dependency_key: &str, dependency: &deno_graph::Dependency, ) { - if let Some(npm_resolver) = &snapshot.maybe_npm_resolver { - if npm_resolver.in_npm_package(referrer) { + if let Some(npm) = &snapshot.npm { + if npm.npm_resolver.in_npm_package(referrer) { return; // ignore, surface typescript errors instead } } @@ -1461,8 +1461,7 @@ mod tests { GlobalHttpCache::new(location.to_path_buf(), RealDenoCacheEnv), )), config: Default::default(), - maybe_node_resolver: None, - maybe_npm_resolver: None, + npm: None, } } diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 24d4e2a104..1c672f6237 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -2,6 +2,7 @@ use super::cache::calculate_fs_version; use super::cache::calculate_fs_version_at_path; +use super::language_server::StateNpmSnapshot; use super::text::LineIndex; use super::tsc; use super::tsc::AssetDocument; @@ -41,7 +42,6 @@ use deno_graph::Resolution; 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::npm::NpmPackageReqReference; @@ -1089,7 +1089,7 @@ impl Documents { &self, specifiers: Vec, referrer_doc: &AssetOrDocument, - maybe_node_resolver: Option<&Arc>, + maybe_npm: Option<&StateNpmSnapshot>, ) -> Vec> { let referrer = referrer_doc.specifier(); let dependencies = match referrer_doc { @@ -1098,11 +1098,12 @@ impl Documents { }; let mut results = Vec::new(); for specifier in specifiers { - if let Some(node_resolver) = maybe_node_resolver { - if node_resolver.in_npm_package(referrer) { + if let Some(npm) = maybe_npm { + if npm.node_resolver.in_npm_package(referrer) { // we're in an npm package, so use node resolution results.push(Some(NodeResolution::into_specifier_and_media_type( - node_resolver + npm + .node_resolver .resolve( &specifier, referrer, @@ -1126,9 +1127,9 @@ impl Documents { dependencies.as_ref().and_then(|d| d.deps.get(&specifier)) { if let Some(specifier) = dep.maybe_type.maybe_specifier() { - results.push(self.resolve_dependency(specifier, maybe_node_resolver)); + results.push(self.resolve_dependency(specifier, maybe_npm)); } else if let Some(specifier) = dep.maybe_code.maybe_specifier() { - results.push(self.resolve_dependency(specifier, maybe_node_resolver)); + results.push(self.resolve_dependency(specifier, maybe_npm)); } else { results.push(None); } @@ -1136,12 +1137,11 @@ impl Documents { .resolve_imports_dependency(&specifier) .and_then(|r| r.maybe_specifier()) { - results.push(self.resolve_dependency(specifier, maybe_node_resolver)); + results.push(self.resolve_dependency(specifier, maybe_npm)); } else if let Ok(npm_req_ref) = NpmPackageReqReference::from_str(&specifier) { - results - .push(node_resolve_npm_req_ref(npm_req_ref, maybe_node_resolver)); + results.push(node_resolve_npm_req_ref(npm_req_ref, maybe_npm)); } else { results.push(None); } @@ -1475,7 +1475,7 @@ impl Documents { fn resolve_dependency( &self, specifier: &ModuleSpecifier, - maybe_node_resolver: Option<&Arc>, + maybe_npm: Option<&StateNpmSnapshot>, ) -> Option<(ModuleSpecifier, MediaType)> { if let Some(module_name) = specifier.as_str().strip_prefix("node:") { if deno_node::is_builtin_node_module(module_name) { @@ -1487,7 +1487,7 @@ impl Documents { } if let Ok(npm_ref) = NpmPackageReqReference::from_specifier(specifier) { - return node_resolve_npm_req_ref(npm_ref, maybe_node_resolver); + return node_resolve_npm_req_ref(npm_ref, maybe_npm); } let doc = self.get(specifier)?; let maybe_module = doc.maybe_esm_module().and_then(|r| r.as_ref().ok()); @@ -1496,7 +1496,7 @@ impl Documents { if let Some(specifier) = maybe_types_dependency.and_then(|d| d.maybe_specifier()) { - self.resolve_dependency(specifier, maybe_node_resolver) + self.resolve_dependency(specifier, maybe_npm) } else { let media_type = doc.media_type(); Some((doc.specifier().clone(), media_type)) @@ -1519,18 +1519,26 @@ impl Documents { fn node_resolve_npm_req_ref( npm_req_ref: NpmPackageReqReference, - maybe_node_resolver: Option<&Arc>, + maybe_npm: Option<&StateNpmSnapshot>, ) -> Option<(ModuleSpecifier, MediaType)> { - maybe_node_resolver.map(|node_resolver| { + maybe_npm.map(|npm| { NodeResolution::into_specifier_and_media_type( - node_resolver - .resolve_npm_req_reference( - &npm_req_ref, - NodeResolutionMode::Types, - &PermissionsContainer::allow_all(), - ) + npm + .npm_resolver + .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req()) .ok() - .flatten(), + .and_then(|package_folder| { + npm + .node_resolver + .resolve_npm_reference( + &package_folder, + npm_req_ref.sub_path(), + NodeResolutionMode::Types, + &PermissionsContainer::allow_all(), + ) + .ok() + .flatten() + }), ) }) } diff --git a/cli/lsp/language_server.rs b/cli/lsp/language_server.rs index d9db2edbf4..38d07fb523 100644 --- a/cli/lsp/language_server.rs +++ b/cli/lsp/language_server.rs @@ -158,6 +158,12 @@ impl LspNpmConfigHash { #[derive(Debug, Clone)] pub struct LanguageServer(Arc>); +#[derive(Debug)] +pub struct StateNpmSnapshot { + pub node_resolver: Arc, + pub npm_resolver: Arc, +} + /// Snapshot of the state used by TSC. #[derive(Debug)] pub struct StateSnapshot { @@ -166,8 +172,7 @@ pub struct StateSnapshot { pub config: Arc, pub documents: Documents, pub maybe_import_map: Option>, - pub maybe_node_resolver: Option>, - pub maybe_npm_resolver: Option>, + pub npm: Option, } #[derive(Debug)] @@ -819,8 +824,10 @@ impl Inner { config: self.config.snapshot(), documents: self.documents.clone(), maybe_import_map: self.maybe_import_map.clone(), - maybe_node_resolver: Some(node_resolver), - maybe_npm_resolver: Some(npm_resolver), + npm: Some(StateNpmSnapshot { + node_resolver, + npm_resolver, + }), }) } diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index 9a33ff5f99..cf809408bb 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -3293,9 +3293,9 @@ fn op_is_node_file(state: &mut OpState, #[string] path: String) -> bool { match ModuleSpecifier::parse(&path) { Ok(specifier) => state .state_snapshot - .maybe_npm_resolver + .npm .as_ref() - .map(|r| r.in_npm_package(&specifier)) + .map(|n| n.npm_resolver.in_npm_package(&specifier)) .unwrap_or(false), Err(_) => false, } @@ -3341,7 +3341,7 @@ fn op_resolve( let resolved = state.state_snapshot.documents.resolve( args.specifiers, &referrer_doc, - state.state_snapshot.maybe_node_resolver.as_ref(), + state.state_snapshot.npm.as_ref(), ); Ok( resolved @@ -3477,8 +3477,7 @@ deno_core::extension!(deno_tsc, config: Default::default(), documents: Documents::new(options.cache.clone()), maybe_import_map: None, - maybe_node_resolver: None, - maybe_npm_resolver: None, + npm: None, }), options.performance, )); @@ -4304,8 +4303,7 @@ mod tests { cache_metadata: CacheMetadata::new(cache), config: Default::default(), maybe_import_map: None, - maybe_node_resolver: None, - maybe_npm_resolver: None, + npm: None, } } diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 4a1e0b671b..f1882d5d7b 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -12,6 +12,7 @@ use crate::graph_util::ModuleGraphBuilder; use crate::graph_util::ModuleGraphContainer; use crate::node; use crate::node::CliNodeCodeTranslator; +use crate::npm::CliNpmResolver; use crate::resolver::CliGraphResolver; use crate::tools::check; use crate::tools::check::TypeChecker; @@ -646,6 +647,7 @@ pub struct NpmModuleLoader { node_code_translator: Arc, fs: Arc, node_resolver: Arc, + npm_resolver: Arc, } impl NpmModuleLoader { @@ -654,12 +656,14 @@ impl NpmModuleLoader { node_code_translator: Arc, fs: Arc, node_resolver: Arc, + npm_resolver: Arc, ) -> Self { Self { cjs_resolutions, node_code_translator, fs, node_resolver, + npm_resolver, } } @@ -693,9 +697,13 @@ impl NpmModuleLoader { nv_ref: &NpmPackageNvReference, permissions: &PermissionsContainer, ) -> Result { + let package_folder = self + .npm_resolver + .resolve_pkg_folder_from_deno_module(nv_ref.nv())?; self .handle_node_resolve_result(self.node_resolver.resolve_npm_reference( - nv_ref, + &package_folder, + nv_ref.sub_path(), NodeResolutionMode::Execution, permissions, )) @@ -704,16 +712,20 @@ impl NpmModuleLoader { pub fn resolve_req_reference( &self, - reference: &NpmPackageReqReference, + req_ref: &NpmPackageReqReference, permissions: &PermissionsContainer, ) -> Result { + let package_folder = self + .npm_resolver + .resolve_pkg_folder_from_deno_module_req(req_ref.req())?; self - .handle_node_resolve_result(self.node_resolver.resolve_npm_req_reference( - reference, + .handle_node_resolve_result(self.node_resolver.resolve_npm_reference( + &package_folder, + req_ref.sub_path(), NodeResolutionMode::Execution, permissions, )) - .with_context(|| format!("Could not resolve '{reference}'.")) + .with_context(|| format!("Could not resolve '{}'.", req_ref)) } pub fn maybe_prepare_load( diff --git a/cli/npm/installer.rs b/cli/npm/installer.rs index 9e7b413b41..8f3db05319 100644 --- a/cli/npm/installer.rs +++ b/cli/npm/installer.rs @@ -105,7 +105,7 @@ impl PackageJsonDepsInstaller { let (req, info) = result?; let result = inner .npm_resolution - .resolve_package_req_as_pending_with_info(req, &info); + .resolve_pkg_req_as_pending_with_info(req, &info); if let Err(err) = result { if inner.npm_registry_api.mark_force_reload() { log::debug!("Failed to resolve package. Retrying. Error: {err:#}"); diff --git a/cli/npm/resolution.rs b/cli/npm/resolution.rs index 5595a71efa..3e76d5e85d 100644 --- a/cli/npm/resolution.rs +++ b/cli/npm/resolution.rs @@ -224,19 +224,19 @@ impl NpmResolution { /// Resolves a package requirement for deno graph. This should only be /// called by deno_graph's NpmResolver or for resolving packages in /// a package.json - pub fn resolve_package_req_as_pending( + pub fn resolve_pkg_req_as_pending( &self, pkg_req: &PackageReq, ) -> Result { // we should always have this because it should have been cached before here let package_info = self.api.get_cached_package_info(&pkg_req.name).unwrap(); - self.resolve_package_req_as_pending_with_info(pkg_req, &package_info) + self.resolve_pkg_req_as_pending_with_info(pkg_req, &package_info) } /// Resolves a package requirement for deno graph. This should only be /// called by deno_graph's NpmResolver or for resolving packages in /// a package.json - pub fn resolve_package_req_as_pending_with_info( + pub fn resolve_pkg_req_as_pending_with_info( &self, pkg_req: &PackageReq, package_info: &NpmPackageInfo, diff --git a/cli/npm/resolvers/mod.rs b/cli/npm/resolvers/mod.rs index 19dd726f57..efaad93ee6 100644 --- a/cli/npm/resolvers/mod.rs +++ b/cli/npm/resolvers/mod.rs @@ -23,7 +23,10 @@ use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::NodePermissions; use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::NpmResolver; +use deno_semver::npm::NpmPackageNvReference; +use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageNv; +use deno_semver::package::PackageNvReference; use deno_semver::package::PackageReq; use global::GlobalNpmPackageResolver; use serde::Deserialize; @@ -98,6 +101,19 @@ impl CliNpmResolver { .unwrap_or(false) } + pub fn resolve_pkg_nv_ref_from_pkg_req_ref( + &self, + req_ref: &NpmPackageReqReference, + ) -> Result { + let pkg_nv = self + .resolve_pkg_id_from_pkg_req(req_ref.req()) + .map(|id| id.nv)?; + Ok(NpmPackageNvReference::new(PackageNvReference { + nv: pkg_nv, + sub_path: req_ref.sub_path().map(|s| s.to_string()), + })) + } + pub fn resolve_pkg_id_from_pkg_req( &self, req: &PackageReq, @@ -127,7 +143,7 @@ impl CliNpmResolver { /// Resolve the root folder of the package the provided specifier is in. /// /// This will error when the provided specifier is not in an npm package. - pub fn resolve_package_folder_from_specifier( + pub fn resolve_pkg_folder_from_specifier( &self, specifier: &ModuleSpecifier, ) -> Result, AnyError> { @@ -145,8 +161,24 @@ impl CliNpmResolver { Ok(Some(path)) } + pub fn resolve_pkg_folder_from_deno_module_req( + &self, + req: &PackageReq, + ) -> Result { + let pkg_id = self.resolve_pkg_id_from_pkg_req(req)?; + self.resolve_pkg_folder_from_pkg_id(&pkg_id) + } + + pub fn resolve_pkg_folder_from_deno_module( + &self, + nv: &PackageNv, + ) -> Result { + let pkg_id = self.resolution.resolve_pkg_id_from_deno_module(nv)?; + self.resolve_pkg_folder_from_pkg_id(&pkg_id) + } + /// Resolves the package nv from the provided specifier. - pub fn resolve_package_id_from_specifier( + pub fn resolve_pkg_id_from_specifier( &self, specifier: &ModuleSpecifier, ) -> Result, AnyError> { @@ -266,22 +298,7 @@ impl NpmResolver for CliNpmResolver { &self, specifier: &ModuleSpecifier, ) -> Result, AnyError> { - self.resolve_package_folder_from_specifier(specifier) - } - - fn resolve_package_folder_from_deno_module( - &self, - pkg_nv: &PackageNv, - ) -> Result { - let pkg_id = self.resolution.resolve_pkg_id_from_deno_module(pkg_nv)?; - self.resolve_pkg_folder_from_pkg_id(&pkg_id) - } - - fn resolve_pkg_id_from_pkg_req( - &self, - req: &PackageReq, - ) -> Result { - self.resolution.resolve_pkg_id_from_pkg_req(req) + self.resolve_pkg_folder_from_specifier(specifier) } fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool { diff --git a/cli/resolver.rs b/cli/resolver.rs index 6fd48db821..1cbd3356e0 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -319,9 +319,7 @@ impl NpmResolver for CliGraphResolver { )); } - let result = self - .npm_resolution - .resolve_package_req_as_pending(package_req); + let result = self.npm_resolution.resolve_pkg_req_as_pending(package_req); match result { Ok(nv) => NpmPackageReqResolution::Ok(nv), Err(err) => { diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 5ab39d7195..e3ab448e37 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -404,6 +404,7 @@ pub async fn run( node_code_translator, fs.clone(), node_resolver.clone(), + npm_resolver.clone(), )), }), }; @@ -429,7 +430,7 @@ pub async fn run( }; let worker_factory = CliMainWorkerFactory::new( StorageKeyResolver::empty(), - npm_resolver.clone(), + npm_resolver, node_resolver, Default::default(), Box::new(module_loader_factory), diff --git a/cli/tests/testdata/npm/deno_run_no_bin_entrypoint.out b/cli/tests/testdata/npm/deno_run_no_bin_entrypoint.out index b878bc70ce..73bddfecf6 100644 --- a/cli/tests/testdata/npm/deno_run_no_bin_entrypoint.out +++ b/cli/tests/testdata/npm/deno_run_no_bin_entrypoint.out @@ -1 +1 @@ -error: package '@denotest/esm-basic' did not have a bin property in its package.json +error: '[WILDCARD]@denotest[WILDCARD]esm-basic[WILDCARD]package.json' did not have a bin property diff --git a/cli/tests/testdata/npm/deno_run_no_bin_entrypoint_non_existent_subpath.out b/cli/tests/testdata/npm/deno_run_no_bin_entrypoint_non_existent_subpath.out index 06d7292b09..ea5ee3d354 100644 --- a/cli/tests/testdata/npm/deno_run_no_bin_entrypoint_non_existent_subpath.out +++ b/cli/tests/testdata/npm/deno_run_no_bin_entrypoint_non_existent_subpath.out @@ -1,3 +1,3 @@ -error: package '@denotest/esm-basic' did not have a bin property in its package.json +error: '[WILDCARD]@denotest[WILDCARD]esm-basic[WILDCARD]package.json' did not have a bin property Fallback failed: Cannot find module 'file:///[WILDCARD]/non-existent.js' diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 85ce44b955..a61e3cfe15 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -138,7 +138,10 @@ impl TypeChecker { debug, graph: graph.clone(), hash_data, - maybe_node_resolver: Some(self.node_resolver.clone()), + maybe_npm: Some(tsc::RequestNpmState { + node_resolver: self.node_resolver.clone(), + npm_resolver: self.npm_resolver.clone(), + }), maybe_tsbuildinfo, root_names, check_mode: type_check_mode, diff --git a/cli/tools/task.rs b/cli/tools/task.rs index dcb53e4ecd..6a6c23e39d 100644 --- a/cli/tools/task.rs +++ b/cli/tools/task.rs @@ -268,7 +268,9 @@ fn resolve_npm_commands( let mut result = HashMap::new(); let snapshot = npm_resolver.snapshot(); for id in snapshot.top_level_packages() { - let bin_commands = node_resolver.resolve_binary_commands(&id.nv)?; + let package_folder = npm_resolver.resolve_pkg_folder_from_pkg_id(id)?; + let bin_commands = + node_resolver.resolve_binary_commands(&package_folder)?; for bin_command in bin_commands { result.insert( bin_command.to_string(), diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index 04e8dec048..50b7dc9e4d 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -4,6 +4,7 @@ use crate::args::TsConfig; use crate::args::TypeCheckMode; use crate::cache::FastInsecureHasher; use crate::node; +use crate::npm::CliNpmResolver; use crate::util::checksum; use crate::util::path::mapped_specifier_for_tsc; @@ -293,6 +294,12 @@ pub struct EmittedFile { pub media_type: MediaType, } +#[derive(Debug)] +pub struct RequestNpmState { + pub node_resolver: Arc, + pub npm_resolver: Arc, +} + /// A structure representing a request to be sent to the tsc runtime. #[derive(Debug)] pub struct Request { @@ -303,7 +310,7 @@ pub struct Request { pub debug: bool, pub graph: Arc, pub hash_data: u64, - pub maybe_node_resolver: Option>, + pub maybe_npm: Option, pub maybe_tsbuildinfo: Option, /// A vector of strings that represent the root/entry point modules for the /// program. @@ -327,7 +334,7 @@ struct State { graph: Arc, maybe_tsbuildinfo: Option, maybe_response: Option, - maybe_node_resolver: Option>, + maybe_npm: Option, remapped_specifiers: HashMap, root_map: HashMap, current_dir: PathBuf, @@ -340,7 +347,7 @@ impl Default for State { graph: Arc::new(ModuleGraph::new(GraphKind::All)), maybe_tsbuildinfo: Default::default(), maybe_response: Default::default(), - maybe_node_resolver: Default::default(), + maybe_npm: Default::default(), remapped_specifiers: Default::default(), root_map: Default::default(), current_dir: Default::default(), @@ -352,7 +359,7 @@ impl State { pub fn new( graph: Arc, hash_data: u64, - maybe_node_resolver: Option>, + maybe_npm: Option, maybe_tsbuildinfo: Option, root_map: HashMap, remapped_specifiers: HashMap, @@ -361,7 +368,7 @@ impl State { State { hash_data, graph, - maybe_node_resolver, + maybe_npm, maybe_tsbuildinfo, maybe_response: None, remapped_specifiers, @@ -496,9 +503,9 @@ fn op_load(state: &mut OpState, args: Value) -> Result { } } } else if state - .maybe_node_resolver + .maybe_npm .as_ref() - .map(|resolver| resolver.in_npm_package(specifier)) + .map(|npm| npm.node_resolver.in_npm_package(specifier)) .unwrap_or(false) { media_type = MediaType::from_specifier(specifier); @@ -650,9 +657,13 @@ fn resolve_graph_specifier_types( Ok(Some((module.specifier.clone(), module.media_type))) } Some(Module::Npm(module)) => { - if let Some(node_resolver) = &state.maybe_node_resolver { - let maybe_resolution = node_resolver.resolve_npm_reference( - &module.nv_reference, + if let Some(npm) = &state.maybe_npm { + let package_folder = npm + .npm_resolver + .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?; + let maybe_resolution = npm.node_resolver.resolve_npm_reference( + &package_folder, + module.nv_reference.sub_path(), NodeResolutionMode::Types, &PermissionsContainer::allow_all(), )?; @@ -665,11 +676,11 @@ fn resolve_graph_specifier_types( } Some(Module::External(module)) => { // we currently only use "External" for when the module is in an npm package - Ok(state.maybe_node_resolver.as_ref().map(|node_resolver| { + Ok(state.maybe_npm.as_ref().map(|npm| { let specifier = node::resolve_specifier_into_node_modules(&module.specifier); NodeResolution::into_specifier_and_media_type( - node_resolver.url_to_node_resolution(specifier).ok(), + npm.node_resolver.url_to_node_resolution(specifier).ok(), ) })) } @@ -682,10 +693,11 @@ fn resolve_non_graph_specifier_types( referrer: &ModuleSpecifier, state: &State, ) -> Result, AnyError> { - let node_resolver = match state.maybe_node_resolver.as_ref() { - Some(node_resolver) => node_resolver, + let npm = match state.maybe_npm.as_ref() { + Some(npm) => npm, None => return Ok(None), // we only support non-graph types for npm packages }; + let node_resolver = &npm.node_resolver; if node_resolver.in_npm_package(referrer) { // we're in an npm package, so use node resolution Ok(Some(NodeResolution::into_specifier_and_media_type( @@ -699,13 +711,17 @@ fn resolve_non_graph_specifier_types( .ok() .flatten(), ))) - } else if let Ok(npm_ref) = NpmPackageReqReference::from_str(specifier) { + } else if let Ok(npm_req_ref) = NpmPackageReqReference::from_str(specifier) { // todo(dsherret): add support for injecting this in the graph so // we don't need this special code here. // This could occur when resolving npm:@types/node when it is // injected and not part of the graph - let maybe_resolution = node_resolver.resolve_npm_req_reference( - &npm_ref, + let package_folder = npm + .npm_resolver + .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req())?; + let maybe_resolution = node_resolver.resolve_npm_reference( + &package_folder, + npm_req_ref.sub_path(), NodeResolutionMode::Types, &PermissionsContainer::allow_all(), )?; @@ -722,9 +738,9 @@ fn op_is_node_file(state: &mut OpState, #[string] path: &str) -> bool { let state = state.borrow::(); match ModuleSpecifier::parse(path) { Ok(specifier) => state - .maybe_node_resolver + .maybe_npm .as_ref() - .map(|r| r.in_npm_package(&specifier)) + .map(|n| n.node_resolver.in_npm_package(&specifier)) .unwrap_or(false), Err(_) => false, } @@ -783,7 +799,7 @@ pub fn exec(request: Request) -> Result { state.put(State::new( options.request.graph, options.request.hash_data, - options.request.maybe_node_resolver, + options.request.maybe_npm, options.request.maybe_tsbuildinfo, options.root_map, options.remapped_specifiers, @@ -952,7 +968,7 @@ mod tests { debug: false, graph: Arc::new(graph), hash_data, - maybe_node_resolver: None, + maybe_npm: None, maybe_tsbuildinfo: None, root_names: vec![(specifier.clone(), MediaType::TypeScript)], check_mode: TypeCheckMode::All, diff --git a/cli/worker.rs b/cli/worker.rs index b29f22e6ef..5d80ab6fd7 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -39,6 +39,7 @@ use deno_runtime::worker::MainWorker; use deno_runtime::worker::WorkerOptions; use deno_runtime::BootstrapOptions; use deno_runtime::WorkerLogLevel; +use deno_semver::npm::NpmPackageNvReference; use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReqReference; @@ -386,6 +387,9 @@ impl CliMainWorkerFactory { .npm_resolver .add_package_reqs(&[package_ref.req().clone()]) .await?; + let package_ref = shared + .npm_resolver + .resolve_pkg_nv_ref_from_pkg_req_ref(&package_ref)?; let node_resolution = self.resolve_binary_entrypoint(&package_ref, &permissions)?; let is_main_cjs = matches!(node_resolution, NodeResolution::CommonJs(_)); @@ -511,10 +515,18 @@ impl CliMainWorkerFactory { fn resolve_binary_entrypoint( &self, - package_ref: &NpmPackageReqReference, + package_ref: &NpmPackageNvReference, permissions: &PermissionsContainer, ) -> Result { - match self.shared.node_resolver.resolve_binary_export(package_ref) { + let package_folder = self + .shared + .npm_resolver + .resolve_pkg_folder_from_deno_module(package_ref.nv())?; + match self + .shared + .node_resolver + .resolve_binary_export(&package_folder, package_ref.sub_path()) + { Ok(node_resolution) => Ok(node_resolution), Err(original_err) => { // if the binary entrypoint was not found, fallback to regular node resolution @@ -534,7 +546,7 @@ impl CliMainWorkerFactory { /// resolve the binary entrypoint using regular node resolution fn resolve_binary_entrypoint_fallback( &self, - package_ref: &NpmPackageReqReference, + package_ref: &NpmPackageNvReference, permissions: &PermissionsContainer, ) -> Result, AnyError> { // only fallback if the user specified a sub path @@ -545,12 +557,16 @@ impl CliMainWorkerFactory { return Ok(None); } - let Some(resolution) = - self.shared.node_resolver.resolve_npm_req_reference( - package_ref, - NodeResolutionMode::Execution, - permissions, - )? + let package_folder = self + .shared + .npm_resolver + .resolve_pkg_folder_from_deno_module(package_ref.nv())?; + let Some(resolution) = self.shared.node_resolver.resolve_npm_reference( + &package_folder, + package_ref.sub_path(), + NodeResolutionMode::Execution, + permissions, + )? else { return Ok(None); }; diff --git a/ext/node/Cargo.toml b/ext/node/Cargo.toml index 4466d7ba0d..7a4ab813c4 100644 --- a/ext/node/Cargo.toml +++ b/ext/node/Cargo.toml @@ -25,8 +25,6 @@ deno_fetch.workspace = true deno_fs.workspace = true deno_media_type.workspace = true deno_net.workspace = true -deno_npm.workspace = true -deno_semver.workspace = true digest = { version = "0.10.5", features = ["core-api", "std"] } dsa = "0.6.1" ecb.workspace = true diff --git a/ext/node/lib.rs b/ext/node/lib.rs index d3e0d07dbe..a54d5a0102 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -16,10 +16,6 @@ use deno_core::JsRuntime; use deno_core::ModuleSpecifier; use deno_fs::sync::MaybeSend; use deno_fs::sync::MaybeSync; -use deno_npm::resolution::PackageReqNotFoundError; -use deno_npm::NpmPackageId; -use deno_semver::package::PackageNv; -use deno_semver::package::PackageReq; use once_cell::sync::Lazy; pub mod analyze; @@ -90,17 +86,6 @@ pub trait NpmResolver: std::fmt::Debug + MaybeSend + MaybeSync { specifier: &ModuleSpecifier, ) -> Result, AnyError>; - /// Resolves an npm package folder path from a Deno module. - fn resolve_package_folder_from_deno_module( - &self, - pkg_nv: &PackageNv, - ) -> Result; - - fn resolve_pkg_id_from_pkg_req( - &self, - req: &PackageReq, - ) -> Result; - fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool; fn in_npm_package_at_path(&self, path: &Path) -> bool { diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs index 0b20a019e7..0f7cc5bb1f 100644 --- a/ext/node/package_json.rs +++ b/ext/node/package_json.rs @@ -106,7 +106,13 @@ impl PackageJson { ) -> Result { let package_json: Value = serde_json::from_str(&source) .map_err(|err| anyhow::anyhow!("malformed package.json {}", err))?; + Self::load_from_value(path, package_json) + } + pub fn load_from_value( + path: PathBuf, + package_json: serde_json::Value, + ) -> Result { let imports_val = package_json.get("imports"); let main_val = package_json.get("main"); let module_val = package_json.get("module"); diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index dc247ddceb..dd89dc1fd2 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -15,10 +15,6 @@ use deno_core::url::Url; use deno_core::ModuleSpecifier; use deno_fs::FileSystemRc; use deno_media_type::MediaType; -use deno_semver::npm::NpmPackageNvReference; -use deno_semver::npm::NpmPackageReqReference; -use deno_semver::package::PackageNv; -use deno_semver::package::PackageNvReference; use crate::errors; use crate::AllowAllNodePermissions; @@ -329,46 +325,30 @@ impl NodeResolver { Ok(resolved) } - pub fn resolve_npm_req_reference( - &self, - reference: &NpmPackageReqReference, - mode: NodeResolutionMode, - permissions: &dyn NodePermissions, - ) -> Result, AnyError> { - let pkg_id = self - .npm_resolver - .resolve_pkg_id_from_pkg_req(reference.req())?; - let reference = NpmPackageNvReference::new(PackageNvReference { - nv: pkg_id.nv, - sub_path: reference.sub_path().map(ToOwned::to_owned), - }); - self.resolve_npm_reference(&reference, mode, permissions) - } - pub fn resolve_npm_reference( &self, - reference: &NpmPackageNvReference, + package_folder: &Path, + sub_path: Option<&str>, mode: NodeResolutionMode, permissions: &dyn NodePermissions, ) -> Result, AnyError> { - let package_folder = self - .npm_resolver - .resolve_package_folder_from_deno_module(reference.nv())?; let node_module_kind = NodeModuleKind::Esm; let maybe_resolved_path = self .package_config_resolve( - &reference - .sub_path() + &sub_path .map(|s| format!("./{s}")) .unwrap_or_else(|| ".".to_string()), - &package_folder, + package_folder, node_module_kind, DEFAULT_CONDITIONS, mode, permissions, ) .with_context(|| { - format!("Failed resolving package config for '{reference}'") + format!( + "Failed resolving package config for '{}'", + package_folder.join("package.json").display() + ) })?; let resolved_path = match maybe_resolved_path { Some(resolved_path) => resolved_path, @@ -392,17 +372,19 @@ impl NodeResolver { pub fn resolve_binary_commands( &self, - pkg_nv: &PackageNv, + package_folder: &Path, ) -> Result, AnyError> { - let package_folder = self - .npm_resolver - .resolve_package_folder_from_deno_module(pkg_nv)?; let package_json_path = package_folder.join("package.json"); - let package_json = - self.load_package_json(&AllowAllNodePermissions, package_json_path)?; + let package_json = self + .load_package_json(&AllowAllNodePermissions, package_json_path.clone())?; Ok(match package_json.bin { - Some(Value::String(_)) => vec![pkg_nv.name.to_string()], + Some(Value::String(_)) => { + let Some(name) = &package_json.name else { + bail!("'{}' did not have a name", package_json_path.display()); + }; + vec![name.to_string()] + } Some(Value::Object(o)) => { o.into_iter().map(|(key, _)| key).collect::>() } @@ -412,27 +394,13 @@ impl NodeResolver { pub fn resolve_binary_export( &self, - pkg_ref: &NpmPackageReqReference, + package_folder: &Path, + sub_path: Option<&str>, ) -> Result { - let pkg_nv = self - .npm_resolver - .resolve_pkg_id_from_pkg_req(pkg_ref.req())? - .nv; - let bin_name = pkg_ref.sub_path(); - let package_folder = self - .npm_resolver - .resolve_package_folder_from_deno_module(&pkg_nv)?; let package_json_path = package_folder.join("package.json"); - let package_json = - self.load_package_json(&AllowAllNodePermissions, package_json_path)?; - let bin = match &package_json.bin { - Some(bin) => bin, - None => bail!( - "package '{}' did not have a bin property in its package.json", - &pkg_nv.name, - ), - }; - let bin_entry = resolve_bin_entry_value(&pkg_nv, bin_name, bin)?; + let package_json = self + .load_package_json(&AllowAllNodePermissions, package_json_path.clone())?; + let bin_entry = resolve_bin_entry_value(&package_json, sub_path)?; let url = ModuleSpecifier::from_file_path(package_folder.join(bin_entry)).unwrap(); @@ -1293,13 +1261,19 @@ impl NodeResolver { } fn resolve_bin_entry_value<'a>( - pkg_nv: &PackageNv, + package_json: &'a PackageJson, bin_name: Option<&str>, - bin: &'a Value, ) -> Result<&'a str, AnyError> { + let bin = match &package_json.bin { + Some(bin) => bin, + None => bail!( + "'{}' did not have a bin property", + package_json.path.display(), + ), + }; let bin_entry = match bin { Value::String(_) => { - if bin_name.is_some() && bin_name.unwrap() != pkg_nv.name { + if bin_name.is_some() && bin_name != package_json.name.as_deref() { None } else { Some(bin) @@ -1308,29 +1282,50 @@ fn resolve_bin_entry_value<'a>( Value::Object(o) => { if let Some(bin_name) = bin_name { o.get(bin_name) - } else if o.len() == 1 || o.len() > 1 && o.values().all(|v| v == o.values().next().unwrap()) { + } else if o.len() == 1 + || o.len() > 1 && o.values().all(|v| v == o.values().next().unwrap()) + { o.values().next() } else { - o.get(&pkg_nv.name) + package_json.name.as_ref().and_then(|n| o.get(n)) } - }, - _ => bail!("package '{}' did not have a bin property with a string or object value in its package.json", pkg_nv), + } + _ => bail!( + "'{}' did not have a bin property with a string or object value", + package_json.path.display() + ), }; let bin_entry = match bin_entry { Some(e) => e, None => { + let prefix = package_json + .name + .as_ref() + .map(|n| { + let mut prefix = format!("npm:{}", n); + if let Some(version) = &package_json.version { + prefix.push('@'); + prefix.push_str(version); + } + prefix.push('/'); + prefix + }) + .unwrap_or_default(); let keys = bin .as_object() .map(|o| { o.keys() - .map(|k| format!(" * npm:{pkg_nv}/{k}")) + .map(|k| format!(" * {prefix}{k}")) .collect::>() }) .unwrap_or_default(); bail!( - "package '{}' did not have a bin entry for '{}' in its package.json{}", - pkg_nv, - bin_name.unwrap_or(&pkg_nv.name), + "'{}' did not have a bin entry{}{}", + package_json.path.display(), + bin_name + .or(package_json.name.as_deref()) + .map(|name| format!(" for '{}'", name)) + .unwrap_or_default(), if keys.is_empty() { "".to_string() } else { @@ -1342,8 +1337,8 @@ fn resolve_bin_entry_value<'a>( match bin_entry { Value::String(s) => Ok(s), _ => bail!( - "package '{}' had a non-string sub property of bin in its package.json", - pkg_nv, + "'{}' had a non-string sub property of bin", + package_json.path.display(), ), } } @@ -1595,102 +1590,141 @@ mod tests { use super::*; + fn build_package_json(json: Value) -> PackageJson { + PackageJson::load_from_value(PathBuf::from("/package.json"), json).unwrap() + } + #[test] fn test_resolve_bin_entry_value() { // should resolve the specified value - let value = json!({ - "bin1": "./value1", - "bin2": "./value2", - "test": "./value3", - }); + let pkg_json = build_package_json(json!({ + "name": "pkg", + "version": "1.1.1", + "bin": { + "bin1": "./value1", + "bin2": "./value2", + "pkg": "./value3", + } + })); assert_eq!( - resolve_bin_entry_value( - &PackageNv::from_str("test@1.1.1").unwrap(), - Some("bin1"), - &value - ) - .unwrap(), + resolve_bin_entry_value(&pkg_json, Some("bin1")).unwrap(), "./value1" ); // should resolve the value with the same name when not specified assert_eq!( - resolve_bin_entry_value( - &PackageNv::from_str("test@1.1.1").unwrap(), - None, - &value - ) - .unwrap(), + resolve_bin_entry_value(&pkg_json, None).unwrap(), "./value3" ); // should not resolve when specified value does not exist assert_eq!( - resolve_bin_entry_value( - &PackageNv::from_str("test@1.1.1").unwrap(), - Some("other"), - &value - ) - .err() - .unwrap() - .to_string(), + resolve_bin_entry_value(&pkg_json, Some("other"),) + .err() + .unwrap() + .to_string(), concat!( - "package 'test@1.1.1' did not have a bin entry for 'other' in its package.json\n", + "'/package.json' did not have a bin entry for 'other'\n", "\n", "Possibilities:\n", - " * npm:test@1.1.1/bin1\n", - " * npm:test@1.1.1/bin2\n", - " * npm:test@1.1.1/test" + " * npm:pkg@1.1.1/bin1\n", + " * npm:pkg@1.1.1/bin2\n", + " * npm:pkg@1.1.1/pkg" ) ); // should not resolve when default value can't be determined + let pkg_json = build_package_json(json!({ + "name": "pkg", + "version": "1.1.1", + "bin": { + "bin": "./value1", + "bin2": "./value2", + } + })); assert_eq!( - resolve_bin_entry_value( - &PackageNv::from_str("asdf@1.2.3").unwrap(), - None, - &value - ) - .err() - .unwrap() - .to_string(), + resolve_bin_entry_value(&pkg_json, None) + .err() + .unwrap() + .to_string(), concat!( - "package 'asdf@1.2.3' did not have a bin entry for 'asdf' in its package.json\n", + "'/package.json' did not have a bin entry for 'pkg'\n", "\n", "Possibilities:\n", - " * npm:asdf@1.2.3/bin1\n", - " * npm:asdf@1.2.3/bin2\n", - " * npm:asdf@1.2.3/test" + " * npm:pkg@1.1.1/bin\n", + " * npm:pkg@1.1.1/bin2", ) ); // should resolve since all the values are the same - let value = json!({ - "bin1": "./value", - "bin2": "./value", - }); + let pkg_json = build_package_json(json!({ + "name": "pkg", + "version": "1.2.3", + "bin": { + "bin1": "./value", + "bin2": "./value", + } + })); assert_eq!( - resolve_bin_entry_value( - &PackageNv::from_str("test@1.2.3").unwrap(), - None, - &value - ) - .unwrap(), + resolve_bin_entry_value(&pkg_json, None,).unwrap(), "./value" ); // should not resolve when specified and is a string - let value = json!("./value"); + let pkg_json = build_package_json(json!({ + "name": "pkg", + "version": "1.2.3", + "bin": "./value", + })); assert_eq!( - resolve_bin_entry_value( - &PackageNv::from_str("test@1.2.3").unwrap(), - Some("path"), - &value + resolve_bin_entry_value(&pkg_json, Some("path"),) + .err() + .unwrap() + .to_string(), + "'/package.json' did not have a bin entry for 'path'" + ); + + // no version in the package.json + let pkg_json = build_package_json(json!({ + "name": "pkg", + "bin": { + "bin1": "./value1", + "bin2": "./value2", + } + })); + assert_eq!( + resolve_bin_entry_value(&pkg_json, None) + .err() + .unwrap() + .to_string(), + concat!( + "'/package.json' did not have a bin entry for 'pkg'\n", + "\n", + "Possibilities:\n", + " * npm:pkg/bin1\n", + " * npm:pkg/bin2", + ) + ); + + // no name or version in the package.json + let pkg_json = build_package_json(json!({ + "bin": { + "bin1": "./value1", + "bin2": "./value2", + } + })); + assert_eq!( + resolve_bin_entry_value(&pkg_json, None) + .err() + .unwrap() + .to_string(), + concat!( + "'/package.json' did not have a bin entry\n", + "\n", + "Possibilities:\n", + " * bin1\n", + " * bin2", ) - .err() - .unwrap() - .to_string(), - "package 'test@1.2.3' did not have a bin entry for 'path' in its package.json" ); }