From c069539e0e68175cc0cd9c6aa30a412672f5ed93 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 27 Jan 2025 11:48:54 -0500 Subject: [PATCH] clippy and finish --- cli/factory.rs | 2 + cli/lib/worker.rs | 25 ++-- cli/lsp/resolver.rs | 17 ++- cli/lsp/tsc.rs | 2 + cli/module_loader.rs | 9 +- cli/npm/installer/local.rs | 3 +- cli/rt/run.rs | 48 +++++--- cli/tools/check.rs | 2 +- cli/tools/info.rs | 36 +++--- cli/tsc/mod.rs | 10 +- ext/node/ops/require.rs | 52 ++++---- resolvers/deno/factory.rs | 12 +- resolvers/deno/npm/byonm.rs | 20 ++- resolvers/deno/npm/managed/local.rs | 1 - resolvers/deno/npm/managed/mod.rs | 1 - resolvers/deno/npm/mod.rs | 12 +- resolvers/node/analyze.rs | 11 +- resolvers/node/cache.rs | 183 ++++++++++++++++++++-------- resolvers/node/errors.rs | 2 +- resolvers/node/lib.rs | 2 + resolvers/node/package_json.rs | 7 +- resolvers/node/path.rs | 13 +- resolvers/node/resolution.rs | 26 ++-- 23 files changed, 324 insertions(+), 172 deletions(-) diff --git a/cli/factory.rs b/cli/factory.rs index fb08bbcbc0..d653c66404 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -643,6 +643,8 @@ impl CliFactory { self.workspace_factory()?.clone(), ResolverFactoryOptions { conditions_from_resolution_mode: Default::default(), + // todo(dsherret): configure a resolution cache + node_resolution_cache: None, no_sloppy_imports_cache: false, npm_system_info: self.flags.subcommand.npm_system_info(), specified_import_map: Some(Box::new(CliSpecifiedImportMapProvider { diff --git a/cli/lib/worker.rs b/cli/lib/worker.rs index 4354289e45..a8c5528677 100644 --- a/cli/lib/worker.rs +++ b/cli/lib/worker.rs @@ -7,6 +7,8 @@ use std::sync::Arc; use deno_core::error::JsError; use deno_node::NodeRequireLoaderRc; +use deno_path_util::url_from_file_path; +use deno_path_util::url_to_file_path; use deno_resolver::npm::DenoInNpmPackageChecker; use deno_resolver::npm::NpmResolver; use deno_runtime::colors; @@ -136,6 +138,9 @@ pub fn create_isolate_create_params() -> Option { #[derive(Debug, thiserror::Error, deno_error::JsError)] pub enum ResolveNpmBinaryEntrypointError { + #[class(inherit)] + #[error(transparent)] + PathToUrl(#[from] deno_path_util::PathToUrlError), #[class(inherit)] #[error(transparent)] ResolvePkgJsonBinExport(ResolvePkgJsonBinExportError), @@ -526,13 +531,13 @@ impl LibMainWorkerFactory { .node_resolver .resolve_binary_export(package_folder, sub_path) { - Ok(path) => Ok(path), + Ok(path) => Ok(url_from_file_path(&path)?), Err(original_err) => { // if the binary entrypoint was not found, fallback to regular node resolution let result = self.resolve_binary_entrypoint_fallback(package_folder, sub_path); match result { - Ok(Some(specifier)) => Ok(specifier), + Ok(Some(path)) => Ok(url_from_file_path(&path)?), Ok(None) => { Err(ResolveNpmBinaryEntrypointError::ResolvePkgJsonBinExport( original_err, @@ -574,12 +579,18 @@ impl LibMainWorkerFactory { .map_err( ResolveNpmBinaryEntrypointFallbackError::PackageSubpathResolve, )?; - let Ok(path) = specifier.into_path() else { - Err(ResolveNpmBinaryEntrypointFallbackError::ModuleNotFound( - specifier, - )) + let path = match specifier { + UrlOrPath::Url(ref url) => match url_to_file_path(url) { + Ok(path) => path, + Err(_) => { + return Err(ResolveNpmBinaryEntrypointFallbackError::ModuleNotFound( + specifier, + )); + } + }, + UrlOrPath::Path(path) => path, }; - if self.shared.sys.fs_exists_no_err(p) { + if self.shared.sys.fs_exists_no_err(&path) { Ok(Some(path)) } else { Err(ResolveNpmBinaryEntrypointFallbackError::ModuleNotFound( diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index 13b4c73fff..cca9e4caa5 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -36,6 +36,8 @@ use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; use indexmap::IndexMap; +use node_resolver::cache::NodeResolutionSys; +use node_resolver::cache::NodeResolutionThreadLocalCache; use node_resolver::DenoIsBuiltInNodeModuleChecker; use node_resolver::NodeResolutionKind; use node_resolver::PackageJsonThreadLocalCache; @@ -207,6 +209,8 @@ impl LspScopeResolver { NodeResolutionKind::Execution, ) }) + .ok()? + .into_url() .ok()?, )) .0; @@ -257,7 +261,7 @@ impl LspScopeResolver { root_node_modules_dir: byonm_npm_resolver .root_node_modules_path() .map(|p| p.to_path_buf()), - sys: CliSys::default(), + sys: factory.node_resolution_sys.clone(), pkg_json_resolver: self.pkg_json_resolver.clone(), }, ) @@ -522,6 +526,8 @@ impl LspResolver { resolution_mode, NodeResolutionKind::Types, ) + .ok()? + .into_url() .ok()?, ))) } @@ -669,6 +675,7 @@ struct ResolverFactoryServices { struct ResolverFactory<'a> { config_data: Option<&'a Arc>, pkg_json_resolver: Arc, + node_resolution_sys: NodeResolutionSys, sys: CliSys, services: ResolverFactoryServices, } @@ -684,6 +691,10 @@ impl<'a> ResolverFactory<'a> { Self { config_data, pkg_json_resolver, + node_resolution_sys: NodeResolutionSys::new( + sys.clone(), + Some(Arc::new(NodeResolutionThreadLocalCache)), + ), sys, services: Default::default(), } @@ -702,7 +713,7 @@ impl<'a> ResolverFactory<'a> { let sys = CliSys::default(); let options = if enable_byonm { CliNpmResolverCreateOptions::Byonm(CliByonmNpmResolverCreateOptions { - sys, + sys: self.node_resolution_sys.clone(), pkg_json_resolver: self.pkg_json_resolver.clone(), root_node_modules_dir: self.config_data.and_then(|config_data| { config_data.node_modules_dir.clone().or_else(|| { @@ -926,7 +937,7 @@ impl<'a> ResolverFactory<'a> { DenoIsBuiltInNodeModuleChecker, npm_resolver.clone(), self.pkg_json_resolver.clone(), - self.sys.clone(), + self.node_resolution_sys.clone(), node_resolver::ConditionsFromResolutionMode::default(), ))) }) diff --git a/cli/lsp/tsc.rs b/cli/lsp/tsc.rs index e1b3691c0a..06161eb71a 100644 --- a/cli/lsp/tsc.rs +++ b/cli/lsp/tsc.rs @@ -49,6 +49,7 @@ use indexmap::IndexMap; use indexmap::IndexSet; use lazy_regex::lazy_regex; use log::error; +use node_resolver::cache::NodeResolutionThreadLocalCache; use node_resolver::ResolutionMode; use once_cell::sync::Lazy; use regex::Captures; @@ -4626,6 +4627,7 @@ fn op_resolve_inner( }) .collect(); state.performance.measure(mark); + NodeResolutionThreadLocalCache::clear(); Ok(specifiers) } diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 276637dada..d623c2d3b6 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -666,7 +666,12 @@ impl ResolutionMode::Import, NodeResolutionKind::Execution, ) - .map_err(|e| JsErrorBox::from_err(e).into()); + .map_err(|e| JsErrorBox::from_err(e).into()) + .and_then(|url_or_path| { + url_or_path + .into_url() + .map_err(|e| JsErrorBox::from_err(e).into()) + }); } } @@ -695,6 +700,8 @@ impl source, }) })? + .into_url() + .map_err(JsErrorBox::from_err)? } Some(Module::Node(module)) => module.specifier.clone(), Some(Module::Js(module)) => module.specifier.clone(), diff --git a/cli/npm/installer/local.rs b/cli/npm/installer/local.rs index 229d14ec48..5e14b607f7 100644 --- a/cli/npm/installer/local.rs +++ b/cli/npm/installer/local.rs @@ -2,6 +2,7 @@ //! Code for local node_modules resolution. +use std::borrow::Cow; use std::cell::RefCell; use std::cmp::Ordering; use std::collections::hash_map::Entry; @@ -581,7 +582,7 @@ async fn sync_resolution_with_fs( symlink_package_dir( &local_registry_package_path, &join_package_name( - Cow::Owned(deno_node_modules_dir), + Cow::Borrowed(&deno_node_modules_dir), &package.id.nv.name, ), )?; diff --git a/cli/rt/run.rs b/cli/rt/run.rs index c61126cfb1..44cfe917b4 100644 --- a/cli/rt/run.rs +++ b/cli/rt/run.rs @@ -73,6 +73,7 @@ use deno_runtime::WorkerExecutionMode; use deno_runtime::WorkerLogLevel; use deno_semver::npm::NpmPackageReqReference; use node_resolver::analyze::NodeCodeTranslator; +use node_resolver::cache::NodeResolutionSys; use node_resolver::errors::ClosestPkgJsonError; use node_resolver::DenoIsBuiltInNodeModuleChecker; use node_resolver::NodeResolutionKind; @@ -224,7 +225,10 @@ impl ModuleLoader for EmbeddedModuleLoader { referrer_kind, NodeResolutionKind::Execution, ) - .map_err(JsErrorBox::from_err)?, + .map_err(JsErrorBox::from_err) + .and_then(|url_or_path| { + url_or_path.into_url().map_err(JsErrorBox::from_err) + })?, ), Ok(MappedResolution::PackageJson { dep_result, @@ -235,17 +239,22 @@ impl ModuleLoader for EmbeddedModuleLoader { .as_ref() .map_err(|e| JsErrorBox::from_err(e.clone()))? { - PackageJsonDepValue::Req(req) => self - .shared - .npm_req_resolver - .resolve_req_with_sub_path( - req, - sub_path.as_deref(), - &referrer, - referrer_kind, - NodeResolutionKind::Execution, - ) - .map_err(|e| JsErrorBox::from_err(e).into()), + PackageJsonDepValue::Req(req) => Ok( + self + .shared + .npm_req_resolver + .resolve_req_with_sub_path( + req, + sub_path.as_deref(), + &referrer, + referrer_kind, + NodeResolutionKind::Execution, + ) + .map_err(JsErrorBox::from_err) + .and_then(|url_or_path| { + url_or_path.into_url().map_err(JsErrorBox::from_err) + })?, + ), PackageJsonDepValue::Workspace(version_req) => { let pkg_folder = self .shared @@ -266,7 +275,10 @@ impl ModuleLoader for EmbeddedModuleLoader { referrer_kind, NodeResolutionKind::Execution, ) - .map_err(JsErrorBox::from_err)?, + .map_err(JsErrorBox::from_err) + .and_then(|url_or_path| { + url_or_path.into_url().map_err(JsErrorBox::from_err) + })?, ) } }, @@ -285,7 +297,10 @@ impl ModuleLoader for EmbeddedModuleLoader { referrer_kind, NodeResolutionKind::Execution, ) - .map_err(JsErrorBox::from_err)?, + .map_err(JsErrorBox::from_err) + .and_then(|url_or_path| { + url_or_path.into_url().map_err(JsErrorBox::from_err) + })?, ); } @@ -674,6 +689,7 @@ pub async fn run( }; NpmRegistryReadPermissionChecker::new(sys.clone(), mode) }; + let node_resolution_sys = NodeResolutionSys::new(sys.clone(), None); let (in_npm_pkg_checker, npm_resolver) = match metadata.node_modules { Some(NodeModules::Managed { node_modules_dir }) => { // create an npmrc that uses the fake npm_registry_url to resolve packages @@ -723,7 +739,7 @@ pub async fn run( DenoInNpmPackageChecker::new(CreateInNpmPkgCheckerOptions::Byonm); let npm_resolver = NpmResolver::::new::( NpmResolverCreateOptions::Byonm(ByonmNpmResolverCreateOptions { - sys: sys.clone(), + sys: node_resolution_sys.clone(), pkg_json_resolver: pkg_json_resolver.clone(), root_node_modules_dir, }), @@ -767,7 +783,7 @@ pub async fn run( DenoIsBuiltInNodeModuleChecker, npm_resolver.clone(), pkg_json_resolver.clone(), - sys.clone(), + node_resolution_sys, node_resolver::ConditionsFromResolutionMode::default(), )); let cjs_tracker = Arc::new(CjsTracker::new( diff --git a/cli/tools/check.rs b/cli/tools/check.rs index d8128432b3..d3796cb740 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -667,7 +667,7 @@ fn resolve_npm_nv_ref( node_resolver::NodeResolutionKind::Types, ) .ok()?; - Some(resolved) + resolved.into_url().ok() } /// Matches the `@ts-check` pragma. diff --git a/cli/tools/info.rs b/cli/tools/info.rs index 765156793f..e297cd5b72 100644 --- a/cli/tools/info.rs +++ b/cli/tools/info.rs @@ -75,13 +75,17 @@ pub async fn info( target_pkg_json, sub_path, .. - } => Some(node_resolver.resolve_package_subpath_from_deno_module( - target_pkg_json.clone().dir_path(), - sub_path.as_deref(), - Some(&cwd_url), - node_resolver::ResolutionMode::Import, - node_resolver::NodeResolutionKind::Execution, - )?), + } => Some( + node_resolver + .resolve_package_subpath_from_deno_module( + target_pkg_json.clone().dir_path(), + sub_path.as_deref(), + Some(&cwd_url), + node_resolver::ResolutionMode::Import, + node_resolver::NodeResolutionKind::Execution, + )? + .into_url()?, + ), deno_config::workspace::MappedResolution::PackageJson { alias, sub_path, @@ -94,13 +98,17 @@ pub async fn info( alias, version_req, )?; - Some(node_resolver.resolve_package_subpath_from_deno_module( - pkg_folder, - sub_path.as_deref(), - Some(&cwd_url), - node_resolver::ResolutionMode::Import, - node_resolver::NodeResolutionKind::Execution, - )?) + Some( + node_resolver + .resolve_package_subpath_from_deno_module( + pkg_folder, + sub_path.as_deref(), + Some(&cwd_url), + node_resolver::ResolutionMode::Import, + node_resolver::NodeResolutionKind::Execution, + )? + .into_url()?, + ) } deno_package_json::PackageJsonDepValue::Req(req) => { Some(ModuleSpecifier::parse(&format!( diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index 868610cb40..573c39df1f 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -706,6 +706,9 @@ pub enum ResolveError { )] ModuleResolution(#[from] deno_core::ModuleResolutionError), #[class(inherit)] + #[error(transparent)] + FilePathToUrl(#[from] deno_path_util::PathToUrlError), + #[class(inherit)] #[error("{0}")] PackageSubpathResolve(PackageSubpathResolveError), #[class(inherit)] @@ -937,7 +940,7 @@ fn resolve_graph_specifier_types( NodeResolutionKind::Types, ); let maybe_url = match res_result { - Ok(url) => Some(url), + Ok(path_or_url) => Some(path_or_url.into_url()?), Err(err) => match err.code() { NodeJsErrorCode::ERR_TYPES_NOT_FOUND | NodeJsErrorCode::ERR_MODULE_NOT_FOUND => None, @@ -965,6 +968,9 @@ fn resolve_graph_specifier_types( #[derive(Debug, Error, deno_error::JsError)] pub enum ResolveNonGraphSpecifierTypesError { + #[class(inherit)] + #[error(transparent)] + FilePathToUrl(#[from] deno_path_util::PathToUrlError), #[class(inherit)] #[error(transparent)] ResolvePkgFolderFromDenoReq(#[from] ResolvePkgFolderFromDenoReqError), @@ -1019,7 +1025,7 @@ fn resolve_non_graph_specifier_types( NodeResolutionKind::Types, ); let maybe_url = match res_result { - Ok(url) => Some(url), + Ok(url_or_path) => Some(url_or_path.into_url()?), Err(err) => match err.code() { NodeJsErrorCode::ERR_TYPES_NOT_FOUND | NodeJsErrorCode::ERR_MODULE_NOT_FOUND => None, diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index 3178894176..53f57f0c68 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -23,6 +23,7 @@ use node_resolver::InNpmPackageChecker; use node_resolver::NodeResolutionKind; use node_resolver::NpmPackageFolderResolver; use node_resolver::ResolutionMode; +use node_resolver::UrlOrPath; use node_resolver::UrlOrPathRef; use node_resolver::REQUIRE_CONDITIONS; use sys_traits::FsCanonicalize; @@ -278,11 +279,12 @@ pub fn op_require_resolve_deno_dir< TSys, >>(); + let path = PathBuf::from(parent_filename); Ok( resolver .resolve_package_folder_from_package( &request, - &UrlOrPathRef::new_path(PathBuf::from(parent_filename))), + &UrlOrPathRef::from_path(&path), ) .ok() .map(|p| p.to_string_lossy().into_owned()), @@ -488,9 +490,7 @@ pub fn op_require_try_self< let pkg_json_resolver = state.borrow::>(); let pkg = pkg_json_resolver - .get_closest_package_json(&PathBuf::from( - parent_path.unwrap(), - )) + .get_closest_package_json(&PathBuf::from(parent_path.unwrap())) .ok() .flatten(); if pkg.is_none() { @@ -516,13 +516,13 @@ pub fn op_require_try_self< return Ok(None); } - let referrer = deno_core::url::Url::from_file_path(&pkg.path).unwrap(); if let Some(exports) = &pkg.exports { let node_resolver = state.borrow::>(); + let referrer = UrlOrPathRef::from_path(&pkg.path); let r = node_resolver.package_exports_resolve( &pkg.path, &expansion, @@ -532,11 +532,7 @@ pub fn op_require_try_self< REQUIRE_CONDITIONS, NodeResolutionKind::Execution, )?; - Ok(Some(if r.scheme() == "file" { - url_to_file_path_string(&r)? - } else { - r.to_string() - })) + Ok(Some(url_or_path_to_string(r)?)) } else { Ok(None) } @@ -628,22 +624,21 @@ pub fn op_require_resolve_exports< let referrer = if parent_path.is_empty() { None } else { - Some(Url::from_file_path(parent_path).unwrap()) + Some(PathBuf::from(parent_path)) }; let r = node_resolver.package_exports_resolve( &pkg.path, &format!(".{expansion}"), exports, - referrer.as_ref(), + referrer + .as_ref() + .map(|r| UrlOrPathRef::from_path(r)) + .as_ref(), ResolutionMode::Require, REQUIRE_CONDITIONS, NodeResolutionKind::Execution, )?; - Ok(Some(if r.scheme() == "file" { - url_to_file_path_string(&r)? - } else { - r.to_string() - })) + Ok(Some(url_or_path_to_string(r)?)) } deno_error::js_error_wrapper!( @@ -702,8 +697,7 @@ pub fn op_require_package_imports_resolve< let referrer_path = ensure_read_permission::

(state, &referrer_path) .map_err(RequireErrorKind::Permission)?; let pkg_json_resolver = state.borrow::>(); - let Some(pkg) = pkg_json_resolver - .get_closest_package_json(&referrer_path)? + let Some(pkg) = pkg_json_resolver.get_closest_package_json(&referrer_path)? else { return Ok(None); }; @@ -714,16 +708,15 @@ pub fn op_require_package_imports_resolve< TNpmPackageFolderResolver, TSys, >>(); - let referrer_url = Url::from_file_path(&referrer_filename).unwrap(); let url = node_resolver.package_imports_resolve( &request, - Some(&referrer_url), + Some(&UrlOrPathRef::from_path(&referrer_path)), ResolutionMode::Require, Some(&pkg), REQUIRE_CONDITIONS, NodeResolutionKind::Execution, )?; - Ok(Some(url_to_file_path_string(&url)?)) + Ok(Some(url_or_path_to_string(url)?)) } else { Ok(None) } @@ -739,11 +732,6 @@ pub fn op_require_break_on_next_statement(state: Rc>) { inspector.wait_for_session_and_break_on_next_statement() } -fn url_to_file_path_string(url: &Url) -> Result { - let file_path = url_to_file_path(url)?; - Ok(file_path.to_string_lossy().into_owned()) -} - #[op2(fast)] pub fn op_require_can_parse_as_esm( scope: &mut v8::HandleScope, @@ -769,3 +757,13 @@ pub fn op_require_can_parse_as_esm( let mut source = v8::script_compiler::Source::new(source, Some(&origin)); v8::script_compiler::compile_module(scope, &mut source).is_some() } + +fn url_or_path_to_string( + url: UrlOrPath, +) -> Result { + if url.is_file() { + Ok(url.into_path()?.to_string_lossy().to_string()) + } else { + Ok(url.to_string_lossy().to_string()) + } +} diff --git a/resolvers/deno/factory.rs b/resolvers/deno/factory.rs index c74f694923..b30e6fa060 100644 --- a/resolvers/deno/factory.rs +++ b/resolvers/deno/factory.rs @@ -23,7 +23,7 @@ use deno_npm::NpmSystemInfo; use deno_path_util::fs::canonicalize_path_maybe_not_exists; use deno_path_util::normalize_path; use futures::future::FutureExt; -use node_resolver::cache::SysCache; +use node_resolver::cache::NodeResolutionSys; use node_resolver::ConditionsFromResolutionMode; use node_resolver::DenoIsBuiltInNodeModuleChecker; use node_resolver::NodeResolver; @@ -556,6 +556,7 @@ pub struct ResolverFactoryOptions { pub conditions_from_resolution_mode: ConditionsFromResolutionMode, pub no_sloppy_imports_cache: bool, pub npm_system_info: NpmSystemInfo, + pub node_resolution_cache: Option, pub package_json_cache: Option, pub package_json_dep_resolution: Option, pub specified_import_map: Option>, @@ -585,7 +586,7 @@ pub struct ResolverFactory< + 'static, > { options: ResolverFactoryOptions, - sys: SysCache, + sys: NodeResolutionSys, deno_resolver: async_once_cell::OnceCell>, in_npm_package_checker: Deferred, node_resolver: Deferred< @@ -641,8 +642,10 @@ impl< options: ResolverFactoryOptions, ) -> Self { Self { - options, - sys: SysCache::new(workspace_factory.sys.clone()), + sys: NodeResolutionSys::new( + workspace_factory.sys.clone(), + options.node_resolution_cache.clone(), + ), deno_resolver: Default::default(), in_npm_package_checker: Default::default(), node_resolver: Default::default(), @@ -653,6 +656,7 @@ impl< sloppy_imports_resolver: Default::default(), workspace_factory, workspace_resolver: Default::default(), + options, } } diff --git a/resolvers/deno/npm/byonm.rs b/resolvers/deno/npm/byonm.rs index 5401f38b02..5decc4d82a 100644 --- a/resolvers/deno/npm/byonm.rs +++ b/resolvers/deno/npm/byonm.rs @@ -11,7 +11,7 @@ use deno_path_util::url_to_file_path; use deno_semver::package::PackageReq; use deno_semver::StackString; use deno_semver::Version; -use node_resolver::cache::SysCache; +use node_resolver::cache::NodeResolutionSys; use node_resolver::errors::PackageFolderResolveError; use node_resolver::errors::PackageFolderResolveIoError; use node_resolver::errors::PackageJsonLoadError; @@ -49,7 +49,7 @@ pub enum ByonmResolvePkgFolderFromDenoReqError { pub struct ByonmNpmResolverCreateOptions { // todo(dsherret): investigate removing this pub root_node_modules_dir: Option, - pub sys: SysCache, + pub sys: NodeResolutionSys, pub pkg_json_resolver: PackageJsonResolverRc, } @@ -61,7 +61,7 @@ pub type ByonmNpmResolverRc = pub struct ByonmNpmResolver< TSys: FsCanonicalize + FsRead + FsMetadata + FsReadDir, > { - sys: SysCache, + sys: NodeResolutionSys, pkg_json_resolver: PackageJsonResolverRc, root_node_modules_dir: Option, } @@ -137,7 +137,7 @@ impl referrer: &Url, ) -> Result { fn node_resolve_dir( - sys: &SysCache, + sys: &NodeResolutionSys, alias: &str, start_dir: &Path, ) -> std::io::Result> { @@ -147,8 +147,7 @@ impl if sys.is_dir(&sub_dir) { return Ok(Some( deno_path_util::fs::canonicalize_path_maybe_not_exists( - sys.sys(), - &sub_dir, + sys, &sub_dir, )?, )); } @@ -307,7 +306,7 @@ impl // now check if node_modules/.deno/ matches this constraint let root_node_modules_dir = self.root_node_modules_dir.as_ref()?; let node_modules_deno_dir = root_node_modules_dir.join(".deno"); - let Ok(entries) = self.sys.sys().fs_read_dir(&node_modules_deno_dir) else { + let Ok(entries) = self.sys.fs_read_dir(&node_modules_deno_dir) else { return None; }; let search_prefix = format!( @@ -344,8 +343,7 @@ impl if let Some(tag) = req.version_req.tag() { let initialized_file = node_modules_deno_dir.join(&entry_name).join(".initialized"); - let Ok(contents) = - self.sys.sys().fs_read_to_string_lossy(&initialized_file) + let Ok(contents) = self.sys.fs_read_to_string_lossy(&initialized_file) else { continue; }; @@ -388,7 +386,7 @@ impl referrer: &UrlOrPathRef, ) -> Result { fn inner( - sys: &SysCache, + sys: &NodeResolutionSys, name: &str, referrer: &UrlOrPathRef, ) -> Result { @@ -422,7 +420,7 @@ impl } let path = inner(&self.sys, name, referrer)?; - self.sys.canonicalize(&path).map_err(|err| { + self.sys.fs_canonicalize(&path).map_err(|err| { PackageFolderResolveIoError { package_name: name.to_string(), referrer: referrer.display(), diff --git a/resolvers/deno/npm/managed/local.rs b/resolvers/deno/npm/managed/local.rs index 7bd087fd17..6caab5a8d3 100644 --- a/resolvers/deno/npm/managed/local.rs +++ b/resolvers/deno/npm/managed/local.rs @@ -15,7 +15,6 @@ use node_resolver::errors::PackageFolderResolveIoError; use node_resolver::errors::PackageNotFoundError; use node_resolver::errors::ReferrerNotFoundError; use node_resolver::NpmPackageFolderResolver; -use node_resolver::UrlOrPath; use node_resolver::UrlOrPathRef; use sys_traits::FsCanonicalize; use sys_traits::FsMetadata; diff --git a/resolvers/deno/npm/managed/mod.rs b/resolvers/deno/npm/managed/mod.rs index b58cabf1ce..c74ce470b5 100644 --- a/resolvers/deno/npm/managed/mod.rs +++ b/resolvers/deno/npm/managed/mod.rs @@ -19,7 +19,6 @@ use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; use node_resolver::InNpmPackageChecker; use node_resolver::NpmPackageFolderResolver; -use node_resolver::UrlOrPath; use node_resolver::UrlOrPathRef; use sys_traits::FsCanonicalize; use sys_traits::FsMetadata; diff --git a/resolvers/deno/npm/mod.rs b/resolvers/deno/npm/mod.rs index 1b13df9157..e4cf4704ad 100644 --- a/resolvers/deno/npm/mod.rs +++ b/resolvers/deno/npm/mod.rs @@ -426,14 +426,12 @@ impl< ), PackageResolveErrorKind::PackageFolderResolve(err) => { match err.as_kind() { - PackageFolderResolveErrorKind::PathToUrl(err) => { - return Err( - ResolveIfForNpmPackageErrorKind::NodeResolve( - NodeResolveErrorKind::PathToUrl(err.clone()).into_box(), - ) - .into_box(), + PackageFolderResolveErrorKind::PathToUrl(err) => Err( + ResolveIfForNpmPackageErrorKind::NodeResolve( + NodeResolveErrorKind::PathToUrl(err.clone()).into_box(), ) - } + .into_box(), + ), PackageFolderResolveErrorKind::Io( PackageFolderResolveIoError { package_name, .. }, ) diff --git a/resolvers/node/analyze.rs b/resolvers/node/analyze.rs index fc6a7845aa..dd1e7dbc41 100644 --- a/resolvers/node/analyze.rs +++ b/resolvers/node/analyze.rs @@ -7,7 +7,6 @@ use std::path::Path; use std::path::PathBuf; use deno_error::JsErrorBox; -use deno_path_util::url_from_file_path; use deno_path_util::url_to_file_path; use futures::future::LocalBoxFuture; use futures::stream::FuturesUnordered; @@ -369,12 +368,12 @@ impl< todo!(); } - let referrer = UrlOrPathRef::new_url(referrer); + let referrer = UrlOrPathRef::from_url(referrer); let referrer_path = referrer.path().unwrap(); if specifier.starts_with("./") || specifier.starts_with("../") { if let Some(parent) = referrer_path.parent() { return self - .file_extension_probe(parent.join(specifier), &referrer_path) + .file_extension_probe(parent.join(specifier), referrer_path) .map(|p| Some(UrlOrPath::Path(p))); } else { todo!(); @@ -446,7 +445,7 @@ impl< return Ok(Some(UrlOrPath::Path(d.join("index.js").clean()))); } return self - .file_extension_probe(d, &referrer_path) + .file_extension_probe(d, referrer_path) .map(|p| Some(UrlOrPath::Path(p))); } else if let Some(main) = package_json.main(deno_package_json::NodeModuleKind::Cjs) @@ -468,13 +467,13 @@ impl< } else { parent.join("node_modules").join(specifier) }; - if let Ok(path) = self.file_extension_probe(path, &referrer_path) { + if let Ok(path) = self.file_extension_probe(path, referrer_path) { return Ok(Some(UrlOrPath::Path(path))); } last = parent; } - Err(not_found(specifier, &referrer_path)) + Err(not_found(specifier, referrer_path)) } fn file_extension_probe( diff --git a/resolvers/node/cache.rs b/resolvers/node/cache.rs index 698ca775ff..96b2a91f92 100644 --- a/resolvers/node/cache.rs +++ b/resolvers/node/cache.rs @@ -1,73 +1,136 @@ +// Copyright 2018-2025 the Deno authors. MIT license. + use std::cell::RefCell; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; -use std::rc::Rc; +use sys_traits::BaseFsCanonicalize; +use sys_traits::BaseFsRead; +use sys_traits::BaseFsReadDir; use sys_traits::FileType; use sys_traits::FsCanonicalize; use sys_traits::FsMetadata; use sys_traits::FsMetadataValue; +use sys_traits::FsRead; +use sys_traits::FsReadDir; -// todo(THIS PR): better name lol -#[derive(Debug, Default)] -pub struct SysCache { - sys: TSys, - // todo: combine Rcs - cache: Rc>>>, - canonicalize_cache: Rc>>, +pub trait NodeResolutionCache: + std::fmt::Debug + crate::sync::MaybeSend + crate::sync::MaybeSync +{ + fn get_canonicalized( + &self, + path: &Path, + ) -> Option>; + fn set_canonicalized(&self, from: PathBuf, to: &std::io::Result); + fn get_file_type(&self, path: &Path) -> Option>; + fn set_file_type(&self, path: PathBuf, value: Option); } -impl Clone for SysCache { +thread_local! { + static CANONICALIZED_CACHE: RefCell>> = RefCell::new(HashMap::new()); + static FILE_TYPE_CACHE: RefCell>> = RefCell::new(HashMap::new()); +} + +#[derive(Debug)] +pub struct NodeResolutionThreadLocalCache; + +impl NodeResolutionThreadLocalCache { + pub fn clear() { + CANONICALIZED_CACHE.with_borrow_mut(|cache| cache.clear()); + FILE_TYPE_CACHE.with_borrow_mut(|cache| cache.clear()); + } +} + +impl NodeResolutionCache for NodeResolutionThreadLocalCache { + fn get_canonicalized( + &self, + path: &Path, + ) -> Option> { + CANONICALIZED_CACHE.with_borrow(|cache| { + let item = cache.get(path)?; + Some(match item { + Some(value) => Ok(value.clone()), + None => Err(std::io::Error::new( + std::io::ErrorKind::NotFound, + "Not found.", + )), + }) + }) + } + + fn set_canonicalized(&self, from: PathBuf, to: &std::io::Result) { + CANONICALIZED_CACHE.with_borrow_mut(|cache| match to { + Ok(to) => { + cache.insert(from, Some(to.clone())); + } + Err(err) => { + if err.kind() == std::io::ErrorKind::NotFound { + cache.insert(from, None); + } + } + }); + } + + fn get_file_type(&self, path: &Path) -> Option> { + FILE_TYPE_CACHE.with_borrow(|cache| cache.get(path).cloned()) + } + + fn set_file_type(&self, path: PathBuf, value: Option) { + FILE_TYPE_CACHE.with_borrow_mut(|cache| { + cache.insert(path, value); + }) + } +} + +#[allow(clippy::disallowed_types)] +pub type NodeResolutionCacheRc = crate::sync::MaybeArc; + +#[derive(Debug, Default)] +pub struct NodeResolutionSys { + sys: TSys, + cache: Option, +} + +impl Clone for NodeResolutionSys { fn clone(&self) -> Self { Self { sys: self.sys.clone(), cache: self.cache.clone(), - canonicalize_cache: self.canonicalize_cache.clone(), } } } -impl SysCache { - pub fn new(sys: TSys) -> Self { - Self { - sys, - cache: Default::default(), - canonicalize_cache: Default::default(), - } - } - - pub fn sys(&self) -> &TSys { - &self.sys +impl NodeResolutionSys { + pub fn new(sys: TSys, store: Option) -> Self { + Self { sys, cache: store } } pub fn is_file(&self, path: &Path) -> bool { - match self.get(path) { + match self.get_file_type(path) { Ok(file_type) => file_type.is_file(), Err(_) => false, } } pub fn is_dir(&self, path: &Path) -> bool { - match self.get(path) { + match self.get_file_type(path) { Ok(file_type) => file_type.is_dir(), Err(_) => false, } } pub fn exists_(&self, path: &Path) -> bool { - match self.get(path) { - Ok(_) => true, - Err(_) => false, - } + self.get_file_type(path).is_ok() } - // todo(THIS PR): better name - pub fn get(&self, path: &Path) -> std::io::Result { + pub fn get_file_type(&self, path: &Path) -> std::io::Result { { - if let Some(file_type) = self.cache.borrow().get(path) { - return match *file_type { - Some(file_type) => Ok(file_type), + if let Some(maybe_value) = + self.cache.as_ref().and_then(|c| c.get_file_type(path)) + { + return match maybe_value { + Some(value) => Ok(value), None => Err(std::io::Error::new( std::io::ErrorKind::NotFound, "Not found.", @@ -77,32 +140,56 @@ impl SysCache { } match self.sys.fs_metadata(path) { Ok(metadata) => { - self - .cache - .borrow_mut() - .insert(path.to_path_buf(), Some(metadata.file_type())); + if let Some(cache) = &self.cache { + cache.set_file_type(path.to_path_buf(), Some(metadata.file_type())); + } Ok(metadata.file_type()) } Err(err) => { - self.cache.borrow_mut().insert(path.to_path_buf(), None); + if let Some(cache) = &self.cache { + cache.set_file_type(path.to_path_buf(), None); + } Err(err) } } } } -impl SysCache { - pub fn canonicalize(&self, path: &Path) -> std::io::Result { - { - if let Some(path) = self.canonicalize_cache.borrow().get(path) { - return Ok(path.clone()); +impl BaseFsCanonicalize for NodeResolutionSys { + fn base_fs_canonicalize(&self, from: &Path) -> std::io::Result { + if let Some(cache) = &self.cache { + if let Some(result) = cache.get_canonicalized(from) { + return result; } } - let canon = self.sys.fs_canonicalize(path)?; - self - .canonicalize_cache - .borrow_mut() - .insert(path.to_path_buf(), canon.clone()); - Ok(canon) + let result = self.sys.base_fs_canonicalize(from); + if let Some(cache) = &self.cache { + cache.set_canonicalized(from.to_path_buf(), &result); + } + result + } +} + +impl BaseFsReadDir for NodeResolutionSys { + type ReadDirEntry = TSys::ReadDirEntry; + + #[inline(always)] + fn base_fs_read_dir( + &self, + path: &Path, + ) -> std::io::Result< + Box> + '_>, + > { + self.sys.base_fs_read_dir(path) + } +} + +impl BaseFsRead for NodeResolutionSys { + #[inline(always)] + fn base_fs_read( + &self, + path: &Path, + ) -> std::io::Result> { + self.sys.base_fs_read(path) } } diff --git a/resolvers/node/errors.rs b/resolvers/node/errors.rs index a81166f3b6..85ab5ca444 100644 --- a/resolvers/node/errors.rs +++ b/resolvers/node/errors.rs @@ -166,7 +166,7 @@ impl NodeJsErrorCoded for PackageFolderResolveError { PackageFolderResolveErrorKind::PackageNotFound(e) => e.code(), PackageFolderResolveErrorKind::ReferrerNotFound(e) => e.code(), PackageFolderResolveErrorKind::Io(e) => e.code(), - PackageFolderResolveErrorKind::PathToUrl(e) => { + PackageFolderResolveErrorKind::PathToUrl(_) => { NodeJsErrorCode::ERR_INVALID_FILE_URL_PATH } } diff --git a/resolvers/node/lib.rs b/resolvers/node/lib.rs index 39255eba61..6c256e9987 100644 --- a/resolvers/node/lib.rs +++ b/resolvers/node/lib.rs @@ -17,6 +17,8 @@ mod sync; pub use builtin_modules::DenoIsBuiltInNodeModuleChecker; pub use builtin_modules::IsBuiltInNodeModuleChecker; pub use builtin_modules::DENO_SUPPORTED_BUILTIN_NODE_MODULES; +pub use cache::NodeResolutionCache; +pub use cache::NodeResolutionCacheRc; pub use deno_package_json::PackageJson; pub use npm::InNpmPackageChecker; pub use npm::NpmPackageFolderResolver; diff --git a/resolvers/node/package_json.rs b/resolvers/node/package_json.rs index f8f801cc45..cd98aa4e12 100644 --- a/resolvers/node/package_json.rs +++ b/resolvers/node/package_json.rs @@ -9,7 +9,6 @@ use std::path::PathBuf; use deno_package_json::PackageJson; use deno_package_json::PackageJsonRc; use sys_traits::FsRead; -use url::Url; use crate::errors::ClosestPkgJsonError; use crate::errors::PackageJsonLoadError; @@ -51,17 +50,17 @@ pub struct PackageJsonThreadLocalCache; impl PackageJsonThreadLocalCache { pub fn clear() { - CACHE.with(|cache| cache.borrow_mut().clear()); + CACHE.with_borrow_mut(|cache| cache.clear()); } } impl deno_package_json::PackageJsonCache for PackageJsonThreadLocalCache { fn get(&self, path: &Path) -> Option { - CACHE.with(|cache| cache.borrow().get(path).cloned()) + CACHE.with_borrow(|cache| cache.get(path).cloned()) } fn set(&self, path: PathBuf, package_json: PackageJsonRc) { - CACHE.with(|cache| cache.borrow_mut().insert(path, package_json)); + CACHE.with_borrow_mut(|cache| cache.insert(path, package_json)); } } diff --git a/resolvers/node/path.rs b/resolvers/node/path.rs index 0ad5ceba94..88aba75e86 100644 --- a/resolvers/node/path.rs +++ b/resolvers/node/path.rs @@ -43,6 +43,13 @@ impl UrlOrPath { UrlOrPath::Path(path) => deno_path_util::url_from_file_path(&path), } } + + pub fn to_string_lossy(&self) -> Cow { + match self { + UrlOrPath::Url(url) => Cow::Borrowed(url.as_str()), + UrlOrPath::Path(path) => path.to_string_lossy(), + } + } } impl std::fmt::Display for UrlOrPath { @@ -51,7 +58,7 @@ impl std::fmt::Display for UrlOrPath { UrlOrPath::Url(url) => url.fmt(f), UrlOrPath::Path(path) => { // prefer displaying a url - match deno_path_util::url_from_file_path(&path) { + match deno_path_util::url_from_file_path(path) { Ok(url) => url.fmt(f), Err(_) => { write!(f, "{}", path.display()) @@ -68,14 +75,14 @@ pub struct UrlOrPathRef<'a> { } impl<'a> UrlOrPathRef<'a> { - pub fn new_path(path: &'a Path) -> Self { + pub fn from_path(path: &'a Path) -> Self { Self { url: Default::default(), path: once_cell::unsync::OnceCell::with_value(Cow::Borrowed(path)), } } - pub fn new_url(url: &'a Url) -> Self { + pub fn from_url(url: &'a Url) -> Self { Self { path: Default::default(), url: once_cell::unsync::OnceCell::with_value(Cow::Borrowed(url)), diff --git a/resolvers/node/resolution.rs b/resolvers/node/resolution.rs index e6f8df838d..3bbef283e6 100644 --- a/resolvers/node/resolution.rs +++ b/resolvers/node/resolution.rs @@ -8,7 +8,6 @@ use std::path::PathBuf; use anyhow::bail; use anyhow::Error as AnyError; use deno_package_json::PackageJson; -use deno_path_util::url_from_file_path; use serde_json::Map; use serde_json::Value; use sys_traits::FileType; @@ -17,7 +16,7 @@ use sys_traits::FsMetadata; use sys_traits::FsRead; use url::Url; -use crate::cache::SysCache; +use crate::cache::NodeResolutionSys; use crate::errors; use crate::errors::DataUrlReferrerError; use crate::errors::FinalizeResolutionError; @@ -162,7 +161,7 @@ pub struct NodeResolver< is_built_in_node_module_checker: TIsBuiltInNodeModuleChecker, npm_pkg_folder_resolver: TNpmPackageFolderResolver, pkg_json_resolver: PackageJsonResolverRc, - sys: SysCache, + sys: NodeResolutionSys, conditions_from_resolution_mode: ConditionsFromResolutionMode, } @@ -184,7 +183,7 @@ impl< is_built_in_node_module_checker: TIsBuiltInNodeModuleChecker, npm_pkg_folder_resolver: TNpmPackageFolderResolver, pkg_json_resolver: PackageJsonResolverRc, - sys: SysCache, + sys: NodeResolutionSys, conditions_from_resolution_mode: ConditionsFromResolutionMode, ) -> Self { Self { @@ -254,7 +253,7 @@ impl< let conditions = self .conditions_from_resolution_mode .resolve(resolution_mode); - let referrer = UrlOrPathRef::new_url(referrer); + let referrer = UrlOrPathRef::from_url(referrer); let url = self.module_resolve( specifier, &referrer, @@ -375,7 +374,7 @@ impl< path }; - let maybe_file_type = self.sys.get(&path); + let maybe_file_type = self.sys.get_file_type(&path); match maybe_file_type { Ok(FileType::Dir) => Err( UnsupportedDirImportError { @@ -409,7 +408,7 @@ impl< let package_subpath = package_subpath .map(|s| format!("./{s}")) .unwrap_or_else(|| ".".to_string()); - let maybe_referrer = maybe_referrer.map(|r| UrlOrPathRef::new_url(r)); + let maybe_referrer = maybe_referrer.map(UrlOrPathRef::from_url); let resolved_url = self.resolve_package_dir_subpath( package_dir, &package_subpath, @@ -497,7 +496,7 @@ impl< conditions: &[&str], ) -> Result { fn probe_extensions( - sys: &SysCache, + sys: &NodeResolutionSys, path: &Path, lowercase_path: &str, resolution_mode: ResolutionMode, @@ -579,7 +578,7 @@ impl< return Ok(UrlOrPath::Path(path)); } Err(TypesNotFoundError(Box::new(TypesNotFoundErrorData { - code_specifier: UrlOrPathRef::new_path(&path).display(), + code_specifier: UrlOrPathRef::from_path(&path).display(), maybe_referrer: maybe_referrer.map(|r| r.display()), }))) } @@ -732,7 +731,7 @@ impl< }; let result = match self.package_resolve( &export_target, - &UrlOrPathRef::new_path(package_json_path), + &UrlOrPathRef::from_path(package_json_path), resolution_mode, conditions, resolution_kind, @@ -1545,7 +1544,7 @@ impl< if resolution_kind.is_types() { Err( TypesNotFoundError(Box::new(TypesNotFoundErrorData { - code_specifier: UrlOrPathRef::new_path(&directory.join("index.js")) + code_specifier: UrlOrPathRef::from_path(&directory.join("index.js")) .display(), maybe_referrer: maybe_referrer.map(|r| r.display()), })) @@ -1573,8 +1572,7 @@ impl< { // Specifiers in the node_modules directory are canonicalized // so canoncalize then check if it's in the node_modules directory. - let specifier = - resolve_specifier_into_node_modules(self.sys.sys(), specifier); + let specifier = resolve_specifier_into_node_modules(&self.sys, specifier); return Some(specifier); } @@ -2056,7 +2054,7 @@ mod tests { #[test] fn test_parse_package_name() { let dummy_referrer = Url::parse("http://example.com").unwrap(); - let dummy_referrer = UrlOrPathRef::new_url(&dummy_referrer); + let dummy_referrer = UrlOrPathRef::from_url(&dummy_referrer); assert_eq!( parse_npm_pkg_name("fetch-blob", &dummy_referrer).unwrap(),