From 8cd257de3dbdb94f0ddabcc262ff335c98ca314c Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 3 Dec 2024 19:44:56 -0500 Subject: [PATCH] refactor: remove `CliNpmRegistryApi` (#27222) Extracts more code out of the CLI. --- cli/lsp/diagnostics.rs | 2 +- cli/npm/managed/mod.rs | 54 +++-- cli/npm/managed/registry.rs | 201 ------------------ cli/npm/managed/resolution.rs | 39 ++-- resolvers/npm_cache/lib.rs | 2 +- resolvers/npm_cache/registry_info.rs | 188 +++++++++++++--- resolvers/npm_cache/tarball.rs | 12 +- tests/integration/lsp_tests.rs | 12 +- tests/integration/npm_tests.rs | 2 +- .../npm/cached_only/cached_only/main.out | 3 +- .../npm/npmrc_bad_registry_config/main.out | 6 +- tests/specs/npm/npmrc_bad_token/main.out | 6 +- .../npmrc_password_no_username/install.out | 7 +- .../npmrc_username_no_password/install.out | 7 +- tests/testdata/npm/cached_only/main.out | 2 +- 15 files changed, 242 insertions(+), 301 deletions(-) delete mode 100644 cli/npm/managed/registry.rs diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 1b72953c1b..ac4d8c01e4 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1262,7 +1262,7 @@ impl DenoDiagnostic { Self::NoAttributeType => (lsp::DiagnosticSeverity::ERROR, "The module is a JSON module and not being imported with an import attribute. Consider adding `with { type: \"json\" }` to the import statement.".to_string(), None), Self::NoCache(specifier) => (lsp::DiagnosticSeverity::ERROR, format!("Uncached or missing remote URL: {specifier}"), Some(json!({ "specifier": specifier }))), Self::NotInstalledJsr(pkg_req, specifier) => (lsp::DiagnosticSeverity::ERROR, format!("JSR package \"{pkg_req}\" is not installed or doesn't exist."), Some(json!({ "specifier": specifier }))), - Self::NotInstalledNpm(pkg_req, specifier) => (lsp::DiagnosticSeverity::ERROR, format!("NPM package \"{pkg_req}\" is not installed or doesn't exist."), Some(json!({ "specifier": specifier }))), + Self::NotInstalledNpm(pkg_req, specifier) => (lsp::DiagnosticSeverity::ERROR, format!("npm package \"{pkg_req}\" is not installed or doesn't exist."), Some(json!({ "specifier": specifier }))), Self::NoLocal(specifier) => { let maybe_sloppy_resolution = CliSloppyImportsResolver::new( SloppyImportsCachedFs::new(Arc::new(deno_fs::RealFs)) diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index da39f55e85..5ed25f8272 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -44,7 +44,6 @@ use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; use crate::util::progress_bar::ProgressBar; use crate::util::sync::AtomicFlag; -use self::registry::CliNpmRegistryApi; use self::resolution::NpmResolution; use self::resolvers::create_npm_fs_resolver; use self::resolvers::NpmPackageFsResolver; @@ -57,7 +56,6 @@ use super::CliNpmTarballCache; use super::InnerCliNpmResolverRef; use super::ResolvePkgFolderFromDenoReqError; -mod registry; mod resolution; mod resolvers; @@ -120,13 +118,13 @@ pub async fn create_managed_npm_resolver( ) -> Result, AnyError> { let npm_cache_env = create_cache_env(&options); let npm_cache = create_cache(npm_cache_env.clone(), &options); - let npm_api = create_api(npm_cache.clone(), npm_cache_env.clone(), &options); - let snapshot = resolve_snapshot(&npm_api, options.snapshot).await?; + let api = create_api(npm_cache.clone(), npm_cache_env.clone(), &options); + let snapshot = resolve_snapshot(&api, options.snapshot).await?; Ok(create_inner( npm_cache_env, options.fs, options.maybe_lockfile, - npm_api, + api, npm_cache, options.npmrc, options.npm_install_deps_provider, @@ -143,7 +141,7 @@ fn create_inner( env: Arc, fs: Arc, maybe_lockfile: Option>, - npm_api: Arc, + registry_info_provider: Arc, npm_cache: Arc, npm_rc: Arc, npm_install_deps_provider: Arc, @@ -154,7 +152,7 @@ fn create_inner( lifecycle_scripts: LifecycleScriptsConfig, ) -> Arc { let resolution = Arc::new(NpmResolution::from_serialized( - npm_api.clone(), + registry_info_provider.clone(), snapshot, maybe_lockfile.clone(), )); @@ -178,7 +176,7 @@ fn create_inner( fs, fs_resolver, maybe_lockfile, - npm_api, + registry_info_provider, npm_cache, npm_install_deps_provider, resolution, @@ -215,29 +213,29 @@ fn create_api( cache: Arc, env: Arc, options: &CliManagedNpmResolverCreateOptions, -) -> Arc { - Arc::new(CliNpmRegistryApi::new( - cache.clone(), - Arc::new(CliNpmRegistryInfoProvider::new( - cache, - env, - options.npmrc.clone(), - )), +) -> Arc { + Arc::new(CliNpmRegistryInfoProvider::new( + cache, + env, + options.npmrc.clone(), )) } async fn resolve_snapshot( - api: &CliNpmRegistryApi, + registry_info_provider: &Arc, snapshot: CliNpmResolverManagedSnapshotOption, ) -> Result, AnyError> { match snapshot { CliNpmResolverManagedSnapshotOption::ResolveFromLockfile(lockfile) => { if !lockfile.overwrite() { - let snapshot = snapshot_from_lockfile(lockfile.clone(), api) - .await - .with_context(|| { - format!("failed reading lockfile '{}'", lockfile.filename.display()) - })?; + let snapshot = snapshot_from_lockfile( + lockfile.clone(), + ®istry_info_provider.as_npm_registry_api(), + ) + .await + .with_context(|| { + format!("failed reading lockfile '{}'", lockfile.filename.display()) + })?; Ok(Some(snapshot)) } else { Ok(None) @@ -304,7 +302,7 @@ pub struct ManagedCliNpmResolver { fs: Arc, fs_resolver: Arc, maybe_lockfile: Option>, - npm_api: Arc, + registry_info_provider: Arc, npm_cache: Arc, npm_install_deps_provider: Arc, resolution: Arc, @@ -329,7 +327,7 @@ impl ManagedCliNpmResolver { fs: Arc, fs_resolver: Arc, maybe_lockfile: Option>, - npm_api: Arc, + registry_info_provider: Arc, npm_cache: Arc, npm_install_deps_provider: Arc, resolution: Arc, @@ -342,7 +340,7 @@ impl ManagedCliNpmResolver { fs, fs_resolver, maybe_lockfile, - npm_api, + registry_info_provider, npm_cache, npm_install_deps_provider, text_only_progress_bar, @@ -588,7 +586,7 @@ impl ManagedCliNpmResolver { ) -> Result, AnyError> { // this will internally cache the package information self - .npm_api + .registry_info_provider .package_info(package_name) .await .map_err(|err| err.into()) @@ -684,7 +682,7 @@ impl CliNpmResolver for ManagedCliNpmResolver { fn clone_snapshotted(&self) -> Arc { // create a new snapshotted npm resolution and resolver let npm_resolution = Arc::new(NpmResolution::new( - self.npm_api.clone(), + self.registry_info_provider.clone(), self.resolution.snapshot(), self.maybe_lockfile.clone(), )); @@ -703,7 +701,7 @@ impl CliNpmResolver for ManagedCliNpmResolver { self.lifecycle_scripts.clone(), ), self.maybe_lockfile.clone(), - self.npm_api.clone(), + self.registry_info_provider.clone(), self.npm_cache.clone(), self.npm_install_deps_provider.clone(), npm_resolution, diff --git a/cli/npm/managed/registry.rs b/cli/npm/managed/registry.rs deleted file mode 100644 index b431c77c5d..0000000000 --- a/cli/npm/managed/registry.rs +++ /dev/null @@ -1,201 +0,0 @@ -// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. - -use std::collections::HashMap; -use std::collections::HashSet; -use std::sync::Arc; - -use async_trait::async_trait; -use deno_core::anyhow::anyhow; -use deno_core::error::AnyError; -use deno_core::futures::future::BoxFuture; -use deno_core::futures::future::Shared; -use deno_core::futures::FutureExt; -use deno_core::parking_lot::Mutex; -use deno_npm::registry::NpmPackageInfo; -use deno_npm::registry::NpmRegistryApi; -use deno_npm::registry::NpmRegistryPackageInfoLoadError; -use deno_npm_cache::NpmCacheSetting; - -use crate::npm::CliNpmCache; -use crate::npm::CliNpmRegistryInfoProvider; -use crate::util::sync::AtomicFlag; - -// todo(#27198): Remove this and move functionality down into -// RegistryInfoProvider, which already does most of this. -#[derive(Debug)] -pub struct CliNpmRegistryApi(Option>); - -impl CliNpmRegistryApi { - pub fn new( - cache: Arc, - registry_info_provider: Arc, - ) -> Self { - Self(Some(Arc::new(CliNpmRegistryApiInner { - cache, - force_reload_flag: Default::default(), - mem_cache: Default::default(), - previously_reloaded_packages: Default::default(), - registry_info_provider, - }))) - } - - /// Clears the internal memory cache. - pub fn clear_memory_cache(&self) { - self.inner().clear_memory_cache(); - } - - fn inner(&self) -> &Arc { - // this panicking indicates a bug in the code where this - // wasn't initialized - self.0.as_ref().unwrap() - } -} - -#[async_trait(?Send)] -impl NpmRegistryApi for CliNpmRegistryApi { - async fn package_info( - &self, - name: &str, - ) -> Result, NpmRegistryPackageInfoLoadError> { - match self.inner().maybe_package_info(name).await { - Ok(Some(info)) => Ok(info), - Ok(None) => Err(NpmRegistryPackageInfoLoadError::PackageNotExists { - package_name: name.to_string(), - }), - Err(err) => { - Err(NpmRegistryPackageInfoLoadError::LoadError(Arc::new(err))) - } - } - } - - fn mark_force_reload(&self) -> bool { - self.inner().mark_force_reload() - } -} - -type CacheItemPendingResult = - Result>, Arc>; - -#[derive(Debug)] -enum CacheItem { - Pending(Shared>), - Resolved(Option>), -} - -#[derive(Debug)] -struct CliNpmRegistryApiInner { - cache: Arc, - force_reload_flag: AtomicFlag, - mem_cache: Mutex>, - previously_reloaded_packages: Mutex>, - registry_info_provider: Arc, -} - -impl CliNpmRegistryApiInner { - pub async fn maybe_package_info( - self: &Arc, - name: &str, - ) -> Result>, AnyError> { - let (created, future) = { - let mut mem_cache = self.mem_cache.lock(); - match mem_cache.get(name) { - Some(CacheItem::Resolved(maybe_info)) => { - return Ok(maybe_info.clone()); - } - Some(CacheItem::Pending(future)) => (false, future.clone()), - None => { - let future = { - let api = self.clone(); - let name = name.to_string(); - async move { - if (api.cache.cache_setting().should_use_for_npm_package(&name) && !api.force_reload_flag.is_raised()) - // if this has been previously reloaded, then try loading from the - // file system cache - || !api.previously_reloaded_packages.lock().insert(name.to_string()) - { - // attempt to load from the file cache - if let Some(info) = api.load_file_cached_package_info(&name).await { - let result = Some(Arc::new(info)); - return Ok(result); - } - } - api.registry_info_provider - .load_package_info(&name) - .await - .map_err(Arc::new) - } - .boxed() - .shared() - }; - mem_cache - .insert(name.to_string(), CacheItem::Pending(future.clone())); - (true, future) - } - } - }; - - if created { - match future.await { - Ok(maybe_info) => { - // replace the cache item to say it's resolved now - self - .mem_cache - .lock() - .insert(name.to_string(), CacheItem::Resolved(maybe_info.clone())); - Ok(maybe_info) - } - Err(err) => { - // purge the item from the cache so it loads next time - self.mem_cache.lock().remove(name); - Err(anyhow!("{:#}", err)) - } - } - } else { - Ok(future.await.map_err(|err| anyhow!("{:#}", err))?) - } - } - - fn mark_force_reload(&self) -> bool { - // never force reload the registry information if reloading - // is disabled or if we're already reloading - if matches!( - self.cache.cache_setting(), - NpmCacheSetting::Only | NpmCacheSetting::ReloadAll - ) { - return false; - } - if self.force_reload_flag.raise() { - self.clear_memory_cache(); - true - } else { - false - } - } - - async fn load_file_cached_package_info( - &self, - name: &str, - ) -> Option { - let result = deno_core::unsync::spawn_blocking({ - let cache = self.cache.clone(); - let name = name.to_string(); - move || cache.load_package_info(&name) - }) - .await - .unwrap(); - match result { - Ok(value) => value, - Err(err) => { - if cfg!(debug_assertions) { - panic!("error loading cached npm package info for {name}: {err:#}"); - } else { - None - } - } - } - } - - fn clear_memory_cache(&self) { - self.mem_cache.lock().clear(); - } -} diff --git a/cli/npm/managed/resolution.rs b/cli/npm/managed/resolution.rs index 033c853233..66cc6a7428 100644 --- a/cli/npm/managed/resolution.rs +++ b/cli/npm/managed/resolution.rs @@ -27,10 +27,9 @@ use deno_semver::package::PackageReq; use deno_semver::VersionReq; use crate::args::CliLockfile; +use crate::npm::CliNpmRegistryInfoProvider; use crate::util::sync::SyncReadAsyncWriteLock; -use super::CliNpmRegistryApi; - pub struct AddPkgReqsResult { /// Results from adding the individual packages. /// @@ -47,7 +46,7 @@ pub struct AddPkgReqsResult { /// /// This does not interact with the file system. pub struct NpmResolution { - api: Arc, + registry_info_provider: Arc, snapshot: SyncReadAsyncWriteLock, maybe_lockfile: Option>, } @@ -63,22 +62,22 @@ impl std::fmt::Debug for NpmResolution { impl NpmResolution { pub fn from_serialized( - api: Arc, + registry_info_provider: Arc, initial_snapshot: Option, maybe_lockfile: Option>, ) -> Self { let snapshot = NpmResolutionSnapshot::new(initial_snapshot.unwrap_or_default()); - Self::new(api, snapshot, maybe_lockfile) + Self::new(registry_info_provider, snapshot, maybe_lockfile) } pub fn new( - api: Arc, + registry_info_provider: Arc, initial_snapshot: NpmResolutionSnapshot, maybe_lockfile: Option>, ) -> Self { Self { - api, + registry_info_provider, snapshot: SyncReadAsyncWriteLock::new(initial_snapshot), maybe_lockfile, } @@ -91,7 +90,7 @@ impl NpmResolution { // only allow one thread in here at a time let snapshot_lock = self.snapshot.acquire().await; let result = add_package_reqs_to_snapshot( - &self.api, + &self.registry_info_provider, package_reqs, self.maybe_lockfile.clone(), || snapshot_lock.read().clone(), @@ -119,7 +118,7 @@ impl NpmResolution { let reqs_set = package_reqs.iter().collect::>(); let snapshot = add_package_reqs_to_snapshot( - &self.api, + &self.registry_info_provider, package_reqs, self.maybe_lockfile.clone(), || { @@ -259,7 +258,7 @@ impl NpmResolution { } async fn add_package_reqs_to_snapshot( - api: &CliNpmRegistryApi, + registry_info_provider: &Arc, package_reqs: &[PackageReq], maybe_lockfile: Option>, get_new_snapshot: impl Fn() -> NpmResolutionSnapshot, @@ -282,26 +281,28 @@ async fn add_package_reqs_to_snapshot( /* this string is used in tests */ "Running npm resolution." ); + let npm_registry_api = registry_info_provider.as_npm_registry_api(); let result = snapshot - .add_pkg_reqs(api, get_add_pkg_reqs_options(package_reqs)) + .add_pkg_reqs(&npm_registry_api, get_add_pkg_reqs_options(package_reqs)) .await; - api.clear_memory_cache(); let result = match &result.dep_graph_result { - Err(NpmResolutionError::Resolution(err)) if api.mark_force_reload() => { + Err(NpmResolutionError::Resolution(err)) + if npm_registry_api.mark_force_reload() => + { log::debug!("{err:#}"); log::debug!("npm resolution failed. Trying again..."); - // try again + // try again with forced reloading let snapshot = get_new_snapshot(); - let result = snapshot - .add_pkg_reqs(api, get_add_pkg_reqs_options(package_reqs)) - .await; - api.clear_memory_cache(); - result + snapshot + .add_pkg_reqs(&npm_registry_api, get_add_pkg_reqs_options(package_reqs)) + .await } _ => result, }; + registry_info_provider.clear_memory_cache(); + if let Ok(snapshot) = &result.dep_graph_result { if let Some(lockfile) = maybe_lockfile { populate_lockfile_from_snapshot(&lockfile, snapshot); diff --git a/resolvers/npm_cache/lib.rs b/resolvers/npm_cache/lib.rs index 4e8966a4e1..9f5424dc46 100644 --- a/resolvers/npm_cache/lib.rs +++ b/resolvers/npm_cache/lib.rs @@ -42,7 +42,7 @@ pub struct DownloadError { impl std::error::Error for DownloadError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - Some(self.error.as_ref()) + self.error.source() } } diff --git a/resolvers/npm_cache/registry_info.rs b/resolvers/npm_cache/registry_info.rs index 7ab50f0495..543ddadc5a 100644 --- a/resolvers/npm_cache/registry_info.rs +++ b/resolvers/npm_cache/registry_info.rs @@ -1,14 +1,19 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. use std::collections::HashMap; +use std::collections::HashSet; use std::sync::Arc; use anyhow::anyhow; use anyhow::bail; use anyhow::Context; use anyhow::Error as AnyError; +use async_trait::async_trait; use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; +use deno_npm::registry::NpmRegistryApi; +use deno_npm::registry::NpmRegistryPackageInfoLoadError; +use deno_unsync::sync::AtomicFlag; use deno_unsync::sync::MultiRuntimeAsyncValueCreator; use futures::future::LocalBoxFuture; use futures::FutureExt; @@ -43,8 +48,49 @@ enum MemoryCacheItem { MemoryCached(Result>, Arc>), } -// todo(#27198): refactor to store this only in the http cache and also -// consolidate with CliNpmRegistryApi. +#[derive(Debug, Default)] +struct MemoryCache { + clear_id: usize, + items: HashMap, +} + +impl MemoryCache { + #[inline(always)] + pub fn clear(&mut self) { + self.clear_id += 1; + self.items.clear(); + } + + #[inline(always)] + pub fn get(&self, key: &str) -> Option<&MemoryCacheItem> { + self.items.get(key) + } + + #[inline(always)] + pub fn insert(&mut self, key: String, value: MemoryCacheItem) { + self.items.insert(key, value); + } + + #[inline(always)] + pub fn try_insert( + &mut self, + clear_id: usize, + key: &str, + value: MemoryCacheItem, + ) -> bool { + if clear_id != self.clear_id { + return false; + } + // if the clear_id is the same then the item should exist + debug_assert!(self.items.contains_key(key)); + if let Some(item) = self.items.get_mut(key) { + *item = value; + } + true + } +} + +// todo(#27198): refactor to store this only in the http cache /// Downloads packuments from the npm registry. /// @@ -55,7 +101,9 @@ pub struct RegistryInfoProvider { cache: Arc>, env: Arc, npmrc: Arc, - memory_cache: Mutex>, + force_reload_flag: AtomicFlag, + memory_cache: Mutex, + previously_loaded_packages: Mutex>, } impl RegistryInfoProvider { @@ -68,17 +116,60 @@ impl RegistryInfoProvider { cache, env, npmrc, + force_reload_flag: AtomicFlag::lowered(), memory_cache: Default::default(), + previously_loaded_packages: Default::default(), } } - pub async fn load_package_info( + /// Clears the internal memory cache. + pub fn clear_memory_cache(&self) { + self.memory_cache.lock().clear(); + } + + fn mark_force_reload(&self) -> bool { + // never force reload the registry information if reloading + // is disabled or if we're already reloading + if matches!( + self.cache.cache_setting(), + NpmCacheSetting::Only | NpmCacheSetting::ReloadAll + ) { + return false; + } + if self.force_reload_flag.raise() { + self.clear_memory_cache(); + true + } else { + false + } + } + + pub fn as_npm_registry_api(self: &Arc) -> NpmRegistryApiAdapter { + NpmRegistryApiAdapter(self.clone()) + } + + pub async fn package_info( + self: &Arc, + name: &str, + ) -> Result, NpmRegistryPackageInfoLoadError> { + match self.maybe_package_info(name).await { + Ok(Some(info)) => Ok(info), + Ok(None) => Err(NpmRegistryPackageInfoLoadError::PackageNotExists { + package_name: name.to_string(), + }), + Err(err) => { + Err(NpmRegistryPackageInfoLoadError::LoadError(Arc::new(err))) + } + } + } + + pub async fn maybe_package_info( self: &Arc, name: &str, ) -> Result>, AnyError> { self.load_package_info_inner(name).await.with_context(|| { format!( - "Error getting response at {} for package \"{}\"", + "Failed loading {} for package \"{}\"", get_package_url(&self.npmrc, name), name ) @@ -89,18 +180,9 @@ impl RegistryInfoProvider { self: &Arc, name: &str, ) -> Result>, AnyError> { - if *self.cache.cache_setting() == NpmCacheSetting::Only { - return Err(deno_core::error::custom_error( - "NotCached", - format!( - "An npm specifier not found in cache: \"{name}\", --cached-only is specified." - ) - )); - } - - let cache_item = { + let (cache_item, clear_id) = { let mut mem_cache = self.memory_cache.lock(); - if let Some(cache_item) = mem_cache.get(name) { + let cache_item = if let Some(cache_item) = mem_cache.get(name) { cache_item.clone() } else { let value_creator = MultiRuntimeAsyncValueCreator::new({ @@ -111,7 +193,8 @@ impl RegistryInfoProvider { let cache_item = MemoryCacheItem::Pending(Arc::new(value_creator)); mem_cache.insert(name.to_string(), cache_item.clone()); cache_item - } + }; + (cache_item, mem_cache.clear_id) }; match cache_item { @@ -130,25 +213,37 @@ impl RegistryInfoProvider { Ok(FutureResult::SavedFsCache(info)) => { // return back the future and mark this package as having // been saved in the cache for next time it's requested - *self.memory_cache.lock().get_mut(name).unwrap() = - MemoryCacheItem::FsCached; + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::FsCached, + ); Ok(Some(info)) } Ok(FutureResult::ErroredFsCache(info)) => { // since saving to the fs cache failed, keep the package information in memory - *self.memory_cache.lock().get_mut(name).unwrap() = - MemoryCacheItem::MemoryCached(Ok(Some(info.clone()))); + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::MemoryCached(Ok(Some(info.clone()))), + ); Ok(Some(info)) } Ok(FutureResult::PackageNotExists) => { - *self.memory_cache.lock().get_mut(name).unwrap() = - MemoryCacheItem::MemoryCached(Ok(None)); + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::MemoryCached(Ok(None)), + ); Ok(None) } Err(err) => { - let return_err = anyhow!("{}", err); - *self.memory_cache.lock().get_mut(name).unwrap() = - MemoryCacheItem::MemoryCached(Err(err)); + let return_err = anyhow!("{:#}", err); + self.memory_cache.lock().try_insert( + clear_id, + name, + MemoryCacheItem::MemoryCached(Err(err)), + ); Err(return_err) } } @@ -196,6 +291,29 @@ impl RegistryInfoProvider { }; let name = name.to_string(); async move { + if (downloader.cache.cache_setting().should_use_for_npm_package(&name) && !downloader.force_reload_flag.is_raised()) + // if this has been previously reloaded, then try loading from the + // file system cache + || downloader.previously_loaded_packages.lock().contains(&name) + { + // attempt to load from the file cache + if let Some(info) = downloader.cache.load_package_info(&name)? { + let result = Arc::new(info); + return Ok(FutureResult::SavedFsCache(result)); + } + } + + if *downloader.cache.cache_setting() == NpmCacheSetting::Only { + return Err(deno_core::error::custom_error( + "NotCached", + format!( + "npm package not found in cache: \"{name}\", --cached-only is specified." + ) + )); + } + + downloader.previously_loaded_packages.lock().insert(name.to_string()); + let maybe_bytes = downloader .env .download_with_retries_on_any_tokio_runtime( @@ -234,6 +352,24 @@ impl RegistryInfoProvider { } } +pub struct NpmRegistryApiAdapter( + Arc>, +); + +#[async_trait(?Send)] +impl NpmRegistryApi for NpmRegistryApiAdapter { + async fn package_info( + &self, + name: &str, + ) -> Result, NpmRegistryPackageInfoLoadError> { + self.0.package_info(name).await + } + + fn mark_force_reload(&self) -> bool { + self.0.mark_force_reload() + } +} + // todo(#27198): make this private and only use RegistryInfoProvider in the rest of // the code pub fn get_package_url(npmrc: &ResolvedNpmRc, name: &str) -> Url { diff --git a/resolvers/npm_cache/tarball.rs b/resolvers/npm_cache/tarball.rs index 3102d811d1..5c8e460fd6 100644 --- a/resolvers/npm_cache/tarball.rs +++ b/resolvers/npm_cache/tarball.rs @@ -65,13 +65,13 @@ impl TarballCache { pub async fn ensure_package( self: &Arc, - package: &PackageNv, + package_nv: &PackageNv, dist: &NpmPackageVersionDistInfo, ) -> Result<(), AnyError> { self - .ensure_package_inner(package, dist) + .ensure_package_inner(package_nv, dist) .await - .with_context(|| format!("Failed caching npm package '{}'.", package)) + .with_context(|| format!("Failed caching npm package '{}'.", package_nv)) } async fn ensure_package_inner( @@ -100,7 +100,7 @@ impl TarballCache { match cache_item { MemoryCacheItem::Cached => Ok(()), - MemoryCacheItem::Errored(err) => Err(anyhow!("{}", err)), + MemoryCacheItem::Errored(err) => Err(anyhow!("{:#}", err)), MemoryCacheItem::Pending(creator) => { let result = creator.get().await; match result { @@ -110,7 +110,7 @@ impl TarballCache { Ok(()) } Err(err) => { - let result_err = anyhow!("{}", err); + let result_err = anyhow!("{:#}", err); *self.memory_cache.lock().get_mut(package_nv).unwrap() = MemoryCacheItem::Errored(err); Err(result_err) @@ -138,7 +138,7 @@ impl TarballCache { return Err(deno_core::error::custom_error( "NotCached", format!( - "An npm specifier not found in cache: \"{}\", --cached-only is specified.", + "npm package not found in cache: \"{}\", --cached-only is specified.", &package_nv.name ) ) diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 9ccd33c995..296da75315 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -6009,7 +6009,7 @@ fn lsp_code_actions_deno_cache_npm() { "severity": 1, "code": "not-installed-npm", "source": "deno", - "message": "NPM package \"chalk\" is not installed or doesn't exist.", + "message": "npm package \"chalk\" is not installed or doesn't exist.", "data": { "specifier": "npm:chalk" } }], "version": 1 @@ -6036,7 +6036,7 @@ fn lsp_code_actions_deno_cache_npm() { "severity": 1, "code": "not-installed-npm", "source": "deno", - "message": "NPM package \"chalk\" is not installed or doesn't exist.", + "message": "npm package \"chalk\" is not installed or doesn't exist.", "data": { "specifier": "npm:chalk" } }], "only": ["quickfix"] @@ -6056,7 +6056,7 @@ fn lsp_code_actions_deno_cache_npm() { "severity": 1, "code": "not-installed-npm", "source": "deno", - "message": "NPM package \"chalk\" is not installed or doesn't exist.", + "message": "npm package \"chalk\" is not installed or doesn't exist.", "data": { "specifier": "npm:chalk" } }], "command": { @@ -6111,7 +6111,7 @@ fn lsp_code_actions_deno_cache_all() { "severity": 1, "code": "not-installed-npm", "source": "deno", - "message": "NPM package \"chalk\" is not installed or doesn't exist.", + "message": "npm package \"chalk\" is not installed or doesn't exist.", "data": { "specifier": "npm:chalk" }, }, ], @@ -6199,7 +6199,7 @@ fn lsp_code_actions_deno_cache_all() { "severity": 1, "code": "not-installed-npm", "source": "deno", - "message": "NPM package \"chalk\" is not installed or doesn't exist.", + "message": "npm package \"chalk\" is not installed or doesn't exist.", "data": { "specifier": "npm:chalk" }, }, ], @@ -9860,7 +9860,7 @@ fn lsp_completions_node_builtin() { "severity": 1, "code": "not-installed-npm", "source": "deno", - "message": "NPM package \"@types/node\" is not installed or doesn't exist." + "message": "npm package \"@types/node\" is not installed or doesn't exist." } ]) ); diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index f8c6eebf39..ffd3b817d4 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -102,7 +102,7 @@ fn cached_only_after_first_run() { let stdout = String::from_utf8_lossy(&output.stdout); assert_contains!( stderr, - "An npm specifier not found in cache: \"ansi-styles\", --cached-only is specified." + "npm package not found in cache: \"ansi-styles\", --cached-only is specified." ); assert!(stdout.is_empty()); assert!(!output.status.success()); diff --git a/tests/specs/npm/cached_only/cached_only/main.out b/tests/specs/npm/cached_only/cached_only/main.out index 0d0cdad094..ea17167b14 100644 --- a/tests/specs/npm/cached_only/cached_only/main.out +++ b/tests/specs/npm/cached_only/cached_only/main.out @@ -1,2 +1,3 @@ -error: Error getting response at http://localhost:4260/chalk for package "chalk": An npm specifier not found in cache: "chalk", --cached-only is specified. +error: Failed loading http://localhost:4260/chalk for package "chalk" + 0: npm package not found in cache: "chalk", --cached-only is specified. at file:///[WILDCARD]/main.ts:1:19 diff --git a/tests/specs/npm/npmrc_bad_registry_config/main.out b/tests/specs/npm/npmrc_bad_registry_config/main.out index 5d778d32e9..d616829604 100644 --- a/tests/specs/npm/npmrc_bad_registry_config/main.out +++ b/tests/specs/npm/npmrc_bad_registry_config/main.out @@ -1,3 +1,5 @@ Download http://localhost:4261/@denotest%2fbasic -error: Error getting response at http://localhost:4261/@denotest%2fbasic for package "@denotest/basic": Bad response: 401 -[WILDCARD] \ No newline at end of file +error: Failed loading http://localhost:4261/@denotest%2fbasic for package "@denotest/basic" + +Caused by: + Bad response: 401 diff --git a/tests/specs/npm/npmrc_bad_token/main.out b/tests/specs/npm/npmrc_bad_token/main.out index 5d778d32e9..d616829604 100644 --- a/tests/specs/npm/npmrc_bad_token/main.out +++ b/tests/specs/npm/npmrc_bad_token/main.out @@ -1,3 +1,5 @@ Download http://localhost:4261/@denotest%2fbasic -error: Error getting response at http://localhost:4261/@denotest%2fbasic for package "@denotest/basic": Bad response: 401 -[WILDCARD] \ No newline at end of file +error: Failed loading http://localhost:4261/@denotest%2fbasic for package "@denotest/basic" + +Caused by: + Bad response: 401 diff --git a/tests/specs/npm/npmrc_password_no_username/install.out b/tests/specs/npm/npmrc_password_no_username/install.out index b198bcd27e..ceff6d5c97 100644 --- a/tests/specs/npm/npmrc_password_no_username/install.out +++ b/tests/specs/npm/npmrc_password_no_username/install.out @@ -1,3 +1,4 @@ -[UNORDERED_START] -error: Error getting response at http://localhost:4261/@denotest%2fbasic for package "@denotest/basic": Both the username and password must be provided for basic auth -[UNORDERED_END] +error: Failed loading http://localhost:4261/@denotest%2fbasic for package "@denotest/basic" + +Caused by: + Both the username and password must be provided for basic auth diff --git a/tests/specs/npm/npmrc_username_no_password/install.out b/tests/specs/npm/npmrc_username_no_password/install.out index b198bcd27e..ceff6d5c97 100644 --- a/tests/specs/npm/npmrc_username_no_password/install.out +++ b/tests/specs/npm/npmrc_username_no_password/install.out @@ -1,3 +1,4 @@ -[UNORDERED_START] -error: Error getting response at http://localhost:4261/@denotest%2fbasic for package "@denotest/basic": Both the username and password must be provided for basic auth -[UNORDERED_END] +error: Failed loading http://localhost:4261/@denotest%2fbasic for package "@denotest/basic" + +Caused by: + Both the username and password must be provided for basic auth diff --git a/tests/testdata/npm/cached_only/main.out b/tests/testdata/npm/cached_only/main.out index c4bfc1fc43..99ded2ec76 100644 --- a/tests/testdata/npm/cached_only/main.out +++ b/tests/testdata/npm/cached_only/main.out @@ -1,2 +1,2 @@ -error: Error getting response at http://localhost:4260/chalk for package "chalk": An npm specifier not found in cache: "chalk", --cached-only is specified. +error: Failed loading http://localhost:4260/chalk for package "chalk": npm package not found in cache: "chalk", --cached-only is specified. at file:///[WILDCARD]/testdata/npm/cached_only/main.ts:1:19