diff --git a/cli/lsp/config.rs b/cli/lsp/config.rs index aad12dec66..4749b25988 100644 --- a/cli/lsp/config.rs +++ b/cli/lsp/config.rs @@ -43,6 +43,7 @@ use deno_npm_installer::NpmInstallerFactoryOptions; use deno_npm_installer::graph::NpmCachingStrategy; use deno_npm_installer::lifecycle_scripts::NullLifecycleScriptsExecutor; use deno_package_json::PackageJsonCache; +use deno_package_json::PackageJsonCacheResult; use deno_path_util::url_to_file_path; use deno_resolver::factory::ConfigDiscoveryOption; use deno_resolver::factory::ResolverFactory; @@ -2036,11 +2037,20 @@ impl deno_config::deno_json::DenoJsonCache for DenoJsonMemCache { struct PackageJsonMemCache(Mutex>>); impl deno_package_json::PackageJsonCache for PackageJsonMemCache { - fn get(&self, path: &Path) -> Option> { - self.0.lock().get(path).cloned() + fn get(&self, path: &Path) -> PackageJsonCacheResult { + self + .0 + .lock() + .get(path) + .cloned() + .map(|value| PackageJsonCacheResult::Hit(Some(value))) + .unwrap_or_else(|| PackageJsonCacheResult::NotCached) } - fn set(&self, path: PathBuf, data: Arc) { + fn set(&self, path: PathBuf, data: Option>) { + let Some(data) = data else { + return; + }; self.0.lock().insert(path, data); } } diff --git a/libs/config/workspace/discovery.rs b/libs/config/workspace/discovery.rs index 7bcc4a1be7..a6d313338b 100644 --- a/libs/config/workspace/discovery.rs +++ b/libs/config/workspace/discovery.rs @@ -278,7 +278,7 @@ fn discover_workspace_config_files_for_single_dir< "package.json file found at '{}'", pkg_json_path.display() ); - Ok(Some(pkg_json)) + Ok(pkg_json) } Err(PackageJsonLoadError::Io { source, .. }) if is_skippable_io_error(&source) => diff --git a/libs/config/workspace/mod.rs b/libs/config/workspace/mod.rs index ca1ff3853a..3761917449 100644 --- a/libs/config/workspace/mod.rs +++ b/libs/config/workspace/mod.rs @@ -2633,6 +2633,7 @@ pub mod test { use std::cell::RefCell; use std::collections::HashMap; + use deno_package_json::PackageJsonCacheResult; use deno_path_util::normalize_path; use deno_path_util::url_from_directory_path; use deno_path_util::url_from_file_path; @@ -5836,11 +5837,18 @@ pub mod test { struct PkgJsonMemCache(RefCell>); impl deno_package_json::PackageJsonCache for PkgJsonMemCache { - fn get(&self, path: &Path) -> Option { - self.0.borrow().get(path).cloned() + fn get(&self, path: &Path) -> PackageJsonCacheResult { + match self.0.borrow().get(path).cloned() { + Some(value) => PackageJsonCacheResult::Hit(Some(value)), + None => PackageJsonCacheResult::NotCached, + } } - fn set(&self, path: PathBuf, value: PackageJsonRc) { + fn set(&self, path: PathBuf, value: Option) { + let Some(value) = value else { + // Don't cache misses (no negative cache). + return; + }; self.0.borrow_mut().insert(path, value); } } diff --git a/libs/node_resolver/package_json.rs b/libs/node_resolver/package_json.rs index 56d32f19b5..84020d4027 100644 --- a/libs/node_resolver/package_json.rs +++ b/libs/node_resolver/package_json.rs @@ -2,11 +2,11 @@ use std::cell::RefCell; use std::collections::HashMap; -use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; use deno_package_json::PackageJson; +use deno_package_json::PackageJsonCacheResult; use deno_package_json::PackageJsonRc; use sys_traits::FsRead; @@ -55,11 +55,18 @@ impl PackageJsonThreadLocalCache { } impl deno_package_json::PackageJsonCache for PackageJsonThreadLocalCache { - fn get(&self, path: &Path) -> Option { - CACHE.with_borrow(|cache| cache.get(path).cloned()) + fn get(&self, path: &Path) -> PackageJsonCacheResult { + CACHE.with_borrow(|cache| match cache.get(path).cloned() { + Some(value) => PackageJsonCacheResult::Hit(Some(value)), + None => PackageJsonCacheResult::NotCached, + }) } - fn set(&self, path: PathBuf, package_json: PackageJsonRc) { + fn set(&self, path: PathBuf, package_json: Option) { + let Some(package_json) = package_json else { + // We don't cache misses. + return; + }; CACHE.with_borrow_mut(|cache| cache.insert(path, package_json)); } } @@ -111,12 +118,7 @@ impl PackageJsonResolver { path, ); match result { - Ok(pkg_json) => Ok(Some(pkg_json)), - Err(deno_package_json::PackageJsonLoadError::Io { source, .. }) - if source.kind() == ErrorKind::NotFound => - { - Ok(None) - } + Ok(pkg_json) => Ok(pkg_json), Err(err) => Err(PackageJsonLoadError(err)), } } diff --git a/libs/package_json/lib.rs b/libs/package_json/lib.rs index 8db92d3705..493c56a244 100644 --- a/libs/package_json/lib.rs +++ b/libs/package_json/lib.rs @@ -5,6 +5,7 @@ #![deny(clippy::unused_async)] #![deny(clippy::unnecessary_wraps)] +use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; @@ -30,9 +31,14 @@ pub type PackageJsonDepsRc = deno_maybe_sync::MaybeArc; #[allow(clippy::disallowed_types)] type PackageJsonDepsRcCell = deno_maybe_sync::MaybeOnceLock; +pub enum PackageJsonCacheResult { + Hit(Option), + NotCached, +} + pub trait PackageJsonCache { - fn get(&self, path: &Path) -> Option; - fn set(&self, path: PathBuf, package_json: PackageJsonRc); + fn get(&self, path: &Path) -> PackageJsonCacheResult; + fn set(&self, path: PathBuf, package_json: Option); } #[derive(Debug, Clone, JsError, PartialEq, Eq, Boxed)] @@ -240,24 +246,36 @@ impl PackageJson { sys: &impl FsRead, maybe_cache: Option<&dyn PackageJsonCache>, path: &Path, - ) -> Result { - match maybe_cache.and_then(|c| c.get(path)) { - Some(item) => Ok(item), - _ => match sys.fs_read_to_string_lossy(path) { - Ok(file_text) => { - let pkg_json = - PackageJson::load_from_string(path.to_path_buf(), &file_text)?; - let pkg_json = deno_maybe_sync::new_rc(pkg_json); - if let Some(cache) = maybe_cache { - cache.set(path.to_path_buf(), pkg_json.clone()); + ) -> Result, PackageJsonLoadError> { + let cache_entry = maybe_cache + .map(|c| c.get(path)) + .unwrap_or(PackageJsonCacheResult::NotCached); + + match cache_entry { + PackageJsonCacheResult::Hit(item) => Ok(item), + PackageJsonCacheResult::NotCached => { + match sys.fs_read_to_string_lossy(path) { + Ok(file_text) => { + let pkg_json = + PackageJson::load_from_string(path.to_path_buf(), &file_text)?; + let pkg_json = deno_maybe_sync::new_rc(pkg_json); + if let Some(cache) = maybe_cache { + cache.set(path.to_path_buf(), Some(pkg_json.clone())); + } + Ok(Some(pkg_json)) } - Ok(pkg_json) + Err(err) if err.kind() == ErrorKind::NotFound => { + if let Some(cache) = maybe_cache { + cache.set(path.to_path_buf(), None); + } + Ok(None) + } + Err(err) => Err(PackageJsonLoadError::Io { + path: path.to_path_buf(), + source: err, + }), } - Err(err) => Err(PackageJsonLoadError::Io { - path: path.to_path_buf(), - source: err, - }), - }, + } } }