diff options
| -rw-r--r-- | Cargo.lock | 8 | ||||
| -rw-r--r-- | cli/Cargo.toml | 4 | ||||
| -rw-r--r-- | cli/factory.rs | 5 | ||||
| -rw-r--r-- | cli/graph_util.rs | 25 | ||||
| -rw-r--r-- | cli/lsp/resolver.rs | 9 | ||||
| -rw-r--r-- | cli/module_loader.rs | 1 | ||||
| -rw-r--r-- | cli/npm/managed/installer.rs | 123 | ||||
| -rw-r--r-- | cli/npm/managed/mod.rs | 128 | ||||
| -rw-r--r-- | cli/npm/managed/resolution.rs | 172 | ||||
| -rw-r--r-- | cli/npm/mod.rs | 1 | ||||
| -rw-r--r-- | cli/resolver.rs | 116 | ||||
| -rw-r--r-- | cli/standalone/mod.rs | 83 | ||||
| -rw-r--r-- | cli/tools/task.rs | 1 | ||||
| -rw-r--r-- | tests/integration/lsp_tests.rs | 8 | ||||
| -rw-r--r-- | tests/integration/npm_tests.rs | 1 | ||||
| -rw-r--r-- | tests/registry/npm/@denotest/dep-cannot-parse/1.0.0/package.json | 7 | ||||
| -rw-r--r-- | tests/specs/npm/dynamic_npm_resolution_failure/__test__.jsonc | 4 | ||||
| -rw-r--r-- | tests/specs/npm/dynamic_npm_resolution_failure/main.out | 15 | ||||
| -rw-r--r-- | tests/specs/npm/dynamic_npm_resolution_failure/main.ts | 9 |
19 files changed, 265 insertions, 455 deletions
diff --git a/Cargo.lock b/Cargo.lock index 346651dc5..486d5c2a1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1481,9 +1481,9 @@ dependencies = [ [[package]] name = "deno_graph" -version = "0.78.0" +version = "0.78.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c4ccd2755a805983f96aeccd211c1f7585b6bfec77471f502c47227abe375682" +checksum = "d13080829a06062a14e41e190f64a3407e4a0f63cf7db5dcecbc3cf500445df3" dependencies = [ "anyhow", "async-trait", @@ -1740,9 +1740,9 @@ dependencies = [ [[package]] name = "deno_npm" -version = "0.21.3" +version = "0.21.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a78b95f0daab64d3cfb28b13be2b40d73c8b9563bbce3aa50fc322a7325ce0b9" +checksum = "9812c781ff6b2e0e45c32ccba9983bce84ecccf6f6a7006b750f8c5c9ac15e30" dependencies = [ "anyhow", "async-trait", diff --git a/cli/Cargo.toml b/cli/Cargo.toml index 0292b025d..a523ec657 100644 --- a/cli/Cargo.toml +++ b/cli/Cargo.toml @@ -69,10 +69,10 @@ deno_config = "=0.16.4" deno_core = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_doc = { version = "=0.139.0", features = ["html", "syntect"] } deno_emit = "=0.42.0" -deno_graph = { version = "=0.78.0", features = ["tokio_executor"] } +deno_graph = { version = "=0.78.1", features = ["tokio_executor"] } deno_lint = { version = "=0.60.0", features = ["docs"] } deno_lockfile.workspace = true -deno_npm = "=0.21.3" +deno_npm = "=0.21.4" deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] } deno_semver = "=0.5.4" deno_task_shell = "=0.16.1" diff --git a/cli/factory.rs b/cli/factory.rs index 33786939c..5cae24c7c 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -35,7 +35,6 @@ use crate::npm::CliNpmResolver; use crate::npm::CliNpmResolverByonmCreateOptions; use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedCreateOptions; -use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::resolver::CjsResolutionStore; use crate::resolver::CliGraphResolver; @@ -441,10 +440,8 @@ impl CliFactory { cache_setting: self.options.cache_setting(), text_only_progress_bar: self.text_only_progress_bar().clone(), maybe_node_modules_path: self.options.node_modules_dir_path().cloned(), - package_json_installer: - CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( + package_json_deps_provider: self.package_json_deps_provider().clone(), - ), npm_system_info: self.options.npm_system_info(), npmrc: self.options.npmrc().clone() }) diff --git a/cli/graph_util.rs b/cli/graph_util.rs index 502702b07..67c179293 100644 --- a/cli/graph_util.rs +++ b/cli/graph_util.rs @@ -160,6 +160,10 @@ pub fn graph_valid( if let Some(error) = errors.next() { Err(error) } else { + // finally surface the npm resolution result + if let Err(err) = &graph.npm_dep_graph_result { + return Err(custom_error(get_error_class_name(err), format!("{}", err))); + } Ok(()) } } @@ -562,30 +566,9 @@ impl ModuleGraphBuilder { let initial_redirects_len = graph.redirects.len(); let initial_package_deps_len = graph.packages.package_deps_sum(); let initial_package_mappings_len = graph.packages.mappings().len(); - let initial_npm_packages = graph.npm_packages.len(); graph.build(roots, loader, options).await; - let has_npm_packages_changed = - graph.npm_packages.len() != initial_npm_packages; - // skip installing npm packages if we don't have to - if is_first_execution - && self.npm_resolver.root_node_modules_path().is_some() - || has_npm_packages_changed - { - if let Some(npm_resolver) = self.npm_resolver.as_managed() { - // ensure that the top level package.json is installed if a - // specifier was matched in the package.json - if self.resolver.found_package_json_dep() { - npm_resolver.ensure_top_level_package_json_install().await?; - } - - // resolve the dependencies of any pending dependencies - // that were inserted by building the graph - npm_resolver.resolve_pending().await?; - } - } - let has_redirects_changed = graph.redirects.len() != initial_redirects_len; let has_jsr_package_deps_changed = graph.packages.package_deps_sum() != initial_package_deps_len; diff --git a/cli/lsp/resolver.rs b/cli/lsp/resolver.rs index d0a515063..bd09f0ad1 100644 --- a/cli/lsp/resolver.rs +++ b/cli/lsp/resolver.rs @@ -12,7 +12,6 @@ use crate::npm::CliNpmResolver; use crate::npm::CliNpmResolverByonmCreateOptions; use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedCreateOptions; -use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::npm::ManagedCliNpmResolver; use crate::resolver::CliGraphResolver; @@ -347,9 +346,11 @@ async fn create_npm_resolver( cache_setting: CacheSetting::Only, text_only_progress_bar: ProgressBar::new(ProgressBarStyle::TextOnly), maybe_node_modules_path: config_data.node_modules_dir.clone(), - // do not install while resolving in the lsp—leave that to the cache command - package_json_installer: - CliNpmResolverManagedPackageJsonInstallerOption::NoInstall, + package_json_deps_provider: Arc::new(PackageJsonDepsProvider::new( + config_data.package_json.as_ref().map(|package_json| { + package_json::get_local_package_json_version_reqs(package_json) + }), + )), npmrc: config_data .npmrc .clone() diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 5bcc22c06..1da3f1f2a 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -75,7 +75,6 @@ pub async fn load_top_level_deps(factory: &CliFactory) -> Result<(), AnyError> { let npm_resolver = factory.npm_resolver().await?; if let Some(npm_resolver) = npm_resolver.as_managed() { npm_resolver.ensure_top_level_package_json_install().await?; - npm_resolver.resolve_pending().await?; } // cache as many entries in the import map as we can if let Some(import_map) = factory.maybe_import_map().await? { diff --git a/cli/npm/managed/installer.rs b/cli/npm/managed/installer.rs deleted file mode 100644 index 694e01206..000000000 --- a/cli/npm/managed/installer.rs +++ /dev/null @@ -1,123 +0,0 @@ -// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. - -use std::future::Future; -use std::sync::Arc; - -use deno_core::error::AnyError; -use deno_core::futures::stream::FuturesOrdered; -use deno_core::futures::StreamExt; -use deno_npm::registry::NpmRegistryApi; -use deno_npm::registry::NpmRegistryPackageInfoLoadError; -use deno_semver::package::PackageReq; - -use crate::args::PackageJsonDepsProvider; -use crate::util::sync::AtomicFlag; - -use super::CliNpmRegistryApi; -use super::NpmResolution; - -#[derive(Debug)] -struct PackageJsonDepsInstallerInner { - deps_provider: Arc<PackageJsonDepsProvider>, - has_installed_flag: AtomicFlag, - npm_registry_api: Arc<CliNpmRegistryApi>, - npm_resolution: Arc<NpmResolution>, -} - -impl PackageJsonDepsInstallerInner { - pub fn reqs_with_info_futures<'a>( - &self, - reqs: &'a [&'a PackageReq], - ) -> FuturesOrdered< - impl Future< - Output = Result< - (&'a PackageReq, Arc<deno_npm::registry::NpmPackageInfo>), - NpmRegistryPackageInfoLoadError, - >, - >, - > { - FuturesOrdered::from_iter(reqs.iter().map(|req| { - let api = self.npm_registry_api.clone(); - async move { - let info = api.package_info(&req.name).await?; - Ok::<_, NpmRegistryPackageInfoLoadError>((*req, info)) - } - })) - } -} - -/// Holds and controls installing dependencies from package.json. -#[derive(Debug, Default)] -pub struct PackageJsonDepsInstaller(Option<PackageJsonDepsInstallerInner>); - -impl PackageJsonDepsInstaller { - pub fn new( - deps_provider: Arc<PackageJsonDepsProvider>, - npm_registry_api: Arc<CliNpmRegistryApi>, - npm_resolution: Arc<NpmResolution>, - ) -> Self { - Self(Some(PackageJsonDepsInstallerInner { - deps_provider, - has_installed_flag: Default::default(), - npm_registry_api, - npm_resolution, - })) - } - - /// Creates an installer that never installs local packages during - /// resolution. A top level install will be a no-op. - pub fn no_op() -> Self { - Self(None) - } - - /// Installs the top level dependencies in the package.json file - /// without going through and resolving the descendant dependencies yet. - pub async fn ensure_top_level_install(&self) -> Result<(), AnyError> { - let inner = match &self.0 { - Some(inner) => inner, - None => return Ok(()), - }; - - if !inner.has_installed_flag.raise() { - return Ok(()); // already installed by something else - } - - let package_reqs = inner.deps_provider.reqs().unwrap_or_default(); - - // check if something needs resolving before bothering to load all - // the package information (which is slow) - if package_reqs.iter().all(|req| { - inner - .npm_resolution - .resolve_pkg_id_from_pkg_req(req) - .is_ok() - }) { - log::debug!( - "All package.json deps resolvable. Skipping top level install." - ); - return Ok(()); // everything is already resolvable - } - - let mut reqs_with_info_futures = - inner.reqs_with_info_futures(&package_reqs); - - while let Some(result) = reqs_with_info_futures.next().await { - let (req, info) = result?; - let result = inner - .npm_resolution - .resolve_pkg_req_as_pending_with_info(req, &info) - .await; - if let Err(err) = result { - if inner.npm_registry_api.mark_force_reload() { - log::debug!("Failed to resolve package. Retrying. Error: {err:#}"); - // re-initialize - reqs_with_info_futures = inner.reqs_with_info_futures(&package_reqs); - } else { - return Err(err.into()); - } - } - } - - Ok(()) - } -} diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index d3045a1c9..c086235c3 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -11,7 +11,6 @@ use deno_core::anyhow::Context; use deno_core::error::AnyError; use deno_core::parking_lot::Mutex; use deno_core::serde_json; -use deno_graph::NpmPackageReqsResolution; use deno_npm::npm_rc::ResolvedNpmRc; use deno_npm::registry::NpmPackageInfo; use deno_npm::registry::NpmRegistryApi; @@ -26,6 +25,7 @@ use deno_runtime::deno_node::NodePermissions; use deno_runtime::deno_node::NpmResolver; use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; +use resolution::AddPkgReqsResult; use crate::args::Lockfile; use crate::args::NpmProcessState; @@ -35,9 +35,9 @@ use crate::cache::FastInsecureHasher; use crate::http_util::HttpClientProvider; use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; use crate::util::progress_bar::ProgressBar; +use crate::util::sync::AtomicFlag; use self::cache::NpmCache; -use self::installer::PackageJsonDepsInstaller; use self::registry::CliNpmRegistryApi; use self::resolution::NpmResolution; use self::resolvers::create_npm_fs_resolver; @@ -48,7 +48,6 @@ use super::InnerCliNpmResolverRef; use super::NpmCacheDir; mod cache; -mod installer; mod registry; mod resolution; mod resolvers; @@ -58,11 +57,6 @@ pub enum CliNpmResolverManagedSnapshotOption { Specified(Option<ValidSerializedNpmResolutionSnapshot>), } -pub enum CliNpmResolverManagedPackageJsonInstallerOption { - ConditionalInstall(Arc<PackageJsonDepsProvider>), - NoInstall, -} - pub struct CliNpmResolverManagedCreateOptions { pub snapshot: CliNpmResolverManagedSnapshotOption, pub maybe_lockfile: Option<Arc<Mutex<Lockfile>>>, @@ -73,7 +67,7 @@ pub struct CliNpmResolverManagedCreateOptions { pub text_only_progress_bar: crate::util::progress_bar::ProgressBar, pub maybe_node_modules_path: Option<PathBuf>, pub npm_system_info: NpmSystemInfo, - pub package_json_installer: CliNpmResolverManagedPackageJsonInstallerOption, + pub package_json_deps_provider: Arc<PackageJsonDepsProvider>, pub npmrc: Arc<ResolvedNpmRc>, } @@ -98,7 +92,7 @@ pub async fn create_managed_npm_resolver_for_lsp( npm_api, npm_cache, options.npmrc, - options.package_json_installer, + options.package_json_deps_provider, options.text_only_progress_bar, options.maybe_node_modules_path, options.npm_system_info, @@ -122,7 +116,7 @@ pub async fn create_managed_npm_resolver( npm_api, npm_cache, options.npmrc, - options.package_json_installer, + options.package_json_deps_provider, options.text_only_progress_bar, options.maybe_node_modules_path, options.npm_system_info, @@ -138,7 +132,7 @@ fn create_inner( npm_api: Arc<CliNpmRegistryApi>, npm_cache: Arc<NpmCache>, npm_rc: Arc<ResolvedNpmRc>, - package_json_installer: CliNpmResolverManagedPackageJsonInstallerOption, + package_json_deps_provider: Arc<PackageJsonDepsProvider>, text_only_progress_bar: crate::util::progress_bar::ProgressBar, node_modules_dir_path: Option<PathBuf>, npm_system_info: NpmSystemInfo, @@ -165,25 +159,13 @@ fn create_inner( node_modules_dir_path, npm_system_info.clone(), ); - let package_json_deps_installer = match package_json_installer { - CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( - provider, - ) => Arc::new(PackageJsonDepsInstaller::new( - provider, - npm_api.clone(), - resolution.clone(), - )), - CliNpmResolverManagedPackageJsonInstallerOption::NoInstall => { - Arc::new(PackageJsonDepsInstaller::no_op()) - } - }; Arc::new(ManagedCliNpmResolver::new( fs, fs_resolver, maybe_lockfile, npm_api, npm_cache, - package_json_deps_installer, + package_json_deps_provider, resolution, tarball_cache, text_only_progress_bar, @@ -271,11 +253,12 @@ pub struct ManagedCliNpmResolver { maybe_lockfile: Option<Arc<Mutex<Lockfile>>>, npm_api: Arc<CliNpmRegistryApi>, npm_cache: Arc<NpmCache>, - package_json_deps_installer: Arc<PackageJsonDepsInstaller>, + package_json_deps_provider: Arc<PackageJsonDepsProvider>, resolution: Arc<NpmResolution>, tarball_cache: Arc<TarballCache>, text_only_progress_bar: ProgressBar, npm_system_info: NpmSystemInfo, + top_level_install_flag: AtomicFlag, } impl std::fmt::Debug for ManagedCliNpmResolver { @@ -294,7 +277,7 @@ impl ManagedCliNpmResolver { maybe_lockfile: Option<Arc<Mutex<Lockfile>>>, npm_api: Arc<CliNpmRegistryApi>, npm_cache: Arc<NpmCache>, - package_json_deps_installer: Arc<PackageJsonDepsInstaller>, + package_json_deps_provider: Arc<PackageJsonDepsProvider>, resolution: Arc<NpmResolution>, tarball_cache: Arc<TarballCache>, text_only_progress_bar: ProgressBar, @@ -306,11 +289,12 @@ impl ManagedCliNpmResolver { maybe_lockfile, npm_api, npm_cache, - package_json_deps_installer, + package_json_deps_provider, text_only_progress_bar, resolution, tarball_cache, npm_system_info, + top_level_install_flag: Default::default(), } } @@ -385,14 +369,28 @@ impl ManagedCliNpmResolver { &self, packages: &[PackageReq], ) -> Result<(), AnyError> { + let result = self.add_package_reqs_raw(packages).await; + result.dependencies_result + } + + pub async fn add_package_reqs_raw( + &self, + packages: &[PackageReq], + ) -> AddPkgReqsResult { if packages.is_empty() { - return Ok(()); + return AddPkgReqsResult { + dependencies_result: Ok(()), + results: vec![], + }; } - self.resolution.add_package_reqs(packages).await?; - self.fs_resolver.cache_packages().await?; + let mut result = self.resolution.add_package_reqs(packages).await; + if result.dependencies_result.is_ok() { + result.dependencies_result = + self.cache_packages().await.map_err(AnyError::from); + } - Ok(()) + result } /// Sets package requirements to the resolver, removing old requirements and adding new ones. @@ -422,51 +420,17 @@ impl ManagedCliNpmResolver { &self, ) -> Result<(), AnyError> { // add and ensure this isn't added to the lockfile - let package_reqs = vec![PackageReq::from_str("@types/node").unwrap()]; - self.resolution.add_package_reqs(&package_reqs).await?; - self.cache_packages().await?; + self + .add_package_reqs(&[PackageReq::from_str("@types/node").unwrap()]) + .await?; Ok(()) } - pub async fn resolve_pending(&self) -> Result<(), AnyError> { - self.resolution.resolve_pending().await?; - self.cache_packages().await - } - pub async fn cache_packages(&self) -> Result<(), AnyError> { self.fs_resolver.cache_packages().await } - /// Resolves package requirements for deno graph. - pub async fn resolve_npm_for_deno_graph( - &self, - reqs_with_pkg_infos: &[(&PackageReq, Arc<NpmPackageInfo>)], - ) -> NpmPackageReqsResolution { - let results = self - .resolution - .resolve_pkg_reqs_as_pending_with_info(reqs_with_pkg_infos) - .await; - - let mut resolutions = Vec::with_capacity(results.len()); - for result in results { - match result { - Ok(nv) => { - resolutions.push(Ok(nv)); - } - Err(err) => { - if self.npm_api.mark_force_reload() { - log::debug!("Restarting npm specifier resolution to check for new registry information. Error: {:#}", err); - return NpmPackageReqsResolution::ReloadRegistryInfo; - } else { - resolutions.push(Err(Arc::new(err.into()))); - } - } - } - } - NpmPackageReqsResolution::Resolutions(resolutions) - } - pub fn resolve_pkg_folder_from_deno_module( &self, nv: &PackageNv, @@ -485,10 +449,26 @@ impl ManagedCliNpmResolver { pub async fn ensure_top_level_package_json_install( &self, ) -> Result<(), AnyError> { - self - .package_json_deps_installer - .ensure_top_level_install() - .await + let Some(reqs) = self.package_json_deps_provider.reqs() else { + return Ok(()); + }; + if !self.top_level_install_flag.raise() { + return Ok(()); // already did this + } + // check if something needs resolving before bothering to load all + // the package information (which is slow) + if reqs + .iter() + .all(|req| self.resolution.resolve_pkg_id_from_pkg_req(req).is_ok()) + { + log::debug!( + "All package.json deps resolvable. Skipping top level install." + ); + return Ok(()); // everything is already resolvable + } + + let reqs = reqs.into_iter().cloned().collect::<Vec<_>>(); + self.add_package_reqs(&reqs).await } pub async fn cache_package_info( @@ -582,7 +562,7 @@ impl CliNpmResolver for ManagedCliNpmResolver { self.maybe_lockfile.clone(), self.npm_api.clone(), self.npm_cache.clone(), - self.package_json_deps_installer.clone(), + self.package_json_deps_provider.clone(), npm_resolution, self.tarball_cache.clone(), self.text_only_progress_bar.clone(), diff --git a/cli/npm/managed/resolution.rs b/cli/npm/managed/resolution.rs index 7c2756749..c1d31325d 100644 --- a/cli/npm/managed/resolution.rs +++ b/cli/npm/managed/resolution.rs @@ -8,9 +8,7 @@ use deno_core::error::AnyError; use deno_core::parking_lot::Mutex; use deno_lockfile::NpmPackageDependencyLockfileInfo; use deno_lockfile::NpmPackageLockfileInfo; -use deno_npm::registry::NpmPackageInfo; use deno_npm::registry::NpmRegistryApi; -use deno_npm::resolution::NpmPackageVersionResolutionError; use deno_npm::resolution::NpmPackagesPartitioned; use deno_npm::resolution::NpmResolutionError; use deno_npm::resolution::NpmResolutionSnapshot; @@ -34,6 +32,16 @@ use crate::util::sync::SyncReadAsyncWriteLock; use super::CliNpmRegistryApi; +pub struct AddPkgReqsResult { + /// Results from adding the individual packages. + /// + /// The indexes of the results correspond to the indexes of the provided + /// package requirements. + pub results: Vec<Result<PackageNv, NpmResolutionError>>, + /// The final result of resolving and caching all the package requirements. + pub dependencies_result: Result<(), AnyError>, +} + /// Handles updating and storing npm resolution in memory where the underlying /// snapshot can be updated concurrently. Additionally handles updating the lockfile /// based on changes to the resolution. @@ -80,19 +88,27 @@ impl NpmResolution { pub async fn add_package_reqs( &self, package_reqs: &[PackageReq], - ) -> Result<(), AnyError> { + ) -> AddPkgReqsResult { // only allow one thread in here at a time let snapshot_lock = self.snapshot.acquire().await; - let snapshot = add_package_reqs_to_snapshot( + let result = add_package_reqs_to_snapshot( &self.api, package_reqs, self.maybe_lockfile.clone(), || snapshot_lock.read().clone(), ) - .await?; - - *snapshot_lock.write() = snapshot; - Ok(()) + .await; + + AddPkgReqsResult { + results: result.results, + dependencies_result: match result.dep_graph_result { + Ok(snapshot) => { + *snapshot_lock.write() = snapshot; + Ok(()) + } + Err(err) => Err(err.into()), + }, + } } pub async fn set_package_reqs( @@ -121,24 +137,8 @@ impl NpmResolution { } }, ) - .await?; - - *snapshot_lock.write() = snapshot; - - Ok(()) - } - - pub async fn resolve_pending(&self) -> Result<(), AnyError> { - // only allow one thread in here at a time - let snapshot_lock = self.snapshot.acquire().await; - - let snapshot = add_package_reqs_to_snapshot( - &self.api, - &Vec::new(), - self.maybe_lockfile.clone(), - || snapshot_lock.read().clone(), - ) - .await?; + .await + .into_result()?; *snapshot_lock.write() = snapshot; @@ -217,49 +217,6 @@ impl NpmResolution { .map(|pkg| pkg.id.clone()) } - /// 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 async fn resolve_pkg_req_as_pending_with_info( - &self, - pkg_req: &PackageReq, - pkg_info: &NpmPackageInfo, - ) -> Result<PackageNv, NpmPackageVersionResolutionError> { - debug_assert_eq!(pkg_req.name, pkg_info.name); - // only allow one thread in here at a time - let snapshot_lock = self.snapshot.acquire().await; - - let mut snapshot = snapshot_lock.write(); - let pending_resolver = get_npm_pending_resolver(&self.api); - let nv = pending_resolver.resolve_package_req_as_pending( - &mut snapshot, - pkg_req, - pkg_info, - )?; - Ok(nv) - } - - pub async fn resolve_pkg_reqs_as_pending_with_info( - &self, - reqs_with_pkg_infos: &[(&PackageReq, Arc<NpmPackageInfo>)], - ) -> Vec<Result<PackageNv, NpmPackageVersionResolutionError>> { - // only allow one thread in here at a time - let snapshot_lock = self.snapshot.acquire().await; - - let mut snapshot = snapshot_lock.write(); - let pending_resolver = get_npm_pending_resolver(&self.api); - let mut results = Vec::with_capacity(reqs_with_pkg_infos.len()); - for (pkg_req, pkg_info) in reqs_with_pkg_infos { - debug_assert_eq!(pkg_req.name, pkg_info.name); - results.push(pending_resolver.resolve_package_req_as_pending( - &mut snapshot, - pkg_req, - pkg_info, - )); - } - results - } - pub fn package_reqs(&self) -> HashMap<PackageReq, PackageNv> { self.snapshot.read().package_reqs().clone() } @@ -307,52 +264,50 @@ async fn add_package_reqs_to_snapshot( package_reqs: &[PackageReq], maybe_lockfile: Option<Arc<Mutex<Lockfile>>>, get_new_snapshot: impl Fn() -> NpmResolutionSnapshot, -) -> Result<NpmResolutionSnapshot, AnyError> { +) -> deno_npm::resolution::AddPkgReqsResult { let snapshot = get_new_snapshot(); - let snapshot = if !snapshot.has_pending() - && package_reqs - .iter() - .all(|req| snapshot.package_reqs().contains_key(req)) + if package_reqs + .iter() + .all(|req| snapshot.package_reqs().contains_key(req)) { - log::debug!( - "Snapshot already up to date. Skipping pending npm resolution." - ); - snapshot - } else { - log::debug!( - /* this string is used in tests!! */ - "Running pending npm resolution." - ); - let pending_resolver = get_npm_pending_resolver(api); - let result = pending_resolver - .resolve_pending(snapshot, package_reqs) - .await; - api.clear_memory_cache(); - match result { - Ok(snapshot) => snapshot, - Err(NpmResolutionError::Resolution(err)) if api.mark_force_reload() => { - log::debug!("{err:#}"); - log::debug!("npm resolution failed. Trying again..."); - - // try again - let snapshot = get_new_snapshot(); - let result = pending_resolver - .resolve_pending(snapshot, package_reqs) - .await; - api.clear_memory_cache(); - // now surface the result after clearing the cache - result? - } - Err(err) => return Err(err.into()), + log::debug!("Snapshot already up to date. Skipping npm resolution."); + return deno_npm::resolution::AddPkgReqsResult { + results: package_reqs + .iter() + .map(|req| Ok(snapshot.package_reqs().get(req).unwrap().clone())) + .collect(), + dep_graph_result: Ok(snapshot), + }; + } + log::debug!( + /* this string is used in tests */ + "Running npm resolution." + ); + let pending_resolver = get_npm_pending_resolver(api); + let result = pending_resolver.add_pkg_reqs(snapshot, package_reqs).await; + api.clear_memory_cache(); + let result = match &result.dep_graph_result { + Err(NpmResolutionError::Resolution(err)) if api.mark_force_reload() => { + log::debug!("{err:#}"); + log::debug!("npm resolution failed. Trying again..."); + + // try again + let snapshot = get_new_snapshot(); + let result = pending_resolver.add_pkg_reqs(snapshot, package_reqs).await; + api.clear_memory_cache(); + result } + _ => result, }; - if let Some(lockfile_mutex) = maybe_lockfile { - let mut lockfile = lockfile_mutex.lock(); - populate_lockfile_from_snapshot(&mut lockfile, &snapshot); + if let Ok(snapshot) = &result.dep_graph_result { + if let Some(lockfile_mutex) = maybe_lockfile { + let mut lockfile = lockfile_mutex.lock(); + populate_lockfile_from_snapshot(&mut lockfile, snapshot); + } } - Ok(snapshot) + result } fn get_npm_pending_resolver( @@ -374,7 +329,6 @@ fn populate_lockfile_from_snapshot( lockfile: &mut Lockfile, snapshot: &NpmResolutionSnapshot, ) { - assert!(!snapshot.has_pending()); for (package_req, nv) in snapshot.package_reqs() { lockfile.insert_package_specifier( format!("npm:{}", package_req), diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 8d801744b..8ae81de24 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -25,7 +25,6 @@ pub use self::byonm::ByonmCliNpmResolver; pub use self::byonm::CliNpmResolverByonmCreateOptions; pub use self::cache_dir::NpmCacheDir; pub use self::managed::CliNpmResolverManagedCreateOptions; -pub use self::managed::CliNpmResolverManagedPackageJsonInstallerOption; pub use self::managed::CliNpmResolverManagedSnapshotOption; pub use self::managed::ManagedCliNpmResolver; diff --git a/cli/resolver.rs b/cli/resolver.rs index 994f03f36..87a82dda0 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -7,9 +7,6 @@ use deno_ast::MediaType; use deno_core::anyhow::anyhow; use deno_core::anyhow::Context; use deno_core::error::AnyError; -use deno_core::futures::future; -use deno_core::futures::future::LocalBoxFuture; -use deno_core::futures::FutureExt; use deno_core::ModuleSourceCode; use deno_core::ModuleSpecifier; use deno_graph::source::ResolutionMode; @@ -17,8 +14,9 @@ use deno_graph::source::ResolveError; use deno_graph::source::Resolver; use deno_graph::source::UnknownBuiltInNodeModuleError; use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE; -use deno_graph::NpmPackageReqsResolution; -use deno_npm::registry::NpmPackageInfo; +use deno_graph::NpmLoadError; +use deno_graph::NpmResolvePkgReqsResult; +use deno_npm::resolution::NpmResolutionError; use deno_runtime::deno_fs; use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::is_builtin_node_module; @@ -34,8 +32,6 @@ use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReq; use import_map::ImportMap; use std::borrow::Cow; -use std::cell::RefCell; -use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; use std::rc::Rc; @@ -440,7 +436,7 @@ pub struct CliGraphResolver { maybe_vendor_specifier: Option<ModuleSpecifier>, node_resolver: Option<Arc<CliNodeResolver>>, npm_resolver: Option<Arc<dyn CliNpmResolver>>, - found_package_json_dep_flag: Arc<AtomicFlag>, + found_package_json_dep_flag: AtomicFlag, bare_node_builtins_enabled: bool, } @@ -501,15 +497,11 @@ impl CliGraphResolver { pub fn create_graph_npm_resolver(&self) -> WorkerCliNpmGraphResolver { WorkerCliNpmGraphResolver { npm_resolver: self.npm_resolver.as_ref(), - memory_cache: Default::default(), + found_package_json_dep_flag: &self.found_package_json_dep_flag, bare_node_builtins_enabled: self.bare_node_builtins_enabled, } } - pub fn found_package_json_dep(&self) -> bool { - self.found_package_json_dep_flag.is_raised() - } - fn check_surface_byonm_node_error( &self, specifier: &str, @@ -771,9 +763,7 @@ fn resolve_package_json_dep( #[derive(Debug)] pub struct WorkerCliNpmGraphResolver<'a> { npm_resolver: Option<&'a Arc<dyn CliNpmResolver>>, - // use a cache per worker so that another worker doesn't clear the - // cache of another worker - memory_cache: Rc<RefCell<HashMap<String, Arc<NpmPackageInfo>>>>, + found_package_json_dep_flag: &'a AtomicFlag, bare_node_builtins_enabled: bool, } @@ -810,36 +800,25 @@ impl<'a> deno_graph::source::NpmResolver for WorkerCliNpmGraphResolver<'a> { } } - fn load_and_cache_npm_package_info( - &self, - package_name: &str, - ) -> LocalBoxFuture<'static, Result<(), AnyError>> { + fn load_and_cache_npm_package_info(&self, package_name: &str) { match self.npm_resolver { Some(npm_resolver) if npm_resolver.as_managed().is_some() => { let npm_resolver = npm_resolver.clone(); let package_name = package_name.to_string(); - let memory_cache = self.memory_cache.clone(); - async move { + deno_core::unsync::spawn(async move { if let Some(managed) = npm_resolver.as_managed() { - let package_info = - managed.cache_package_info(&package_name).await?; - memory_cache.borrow_mut().insert(package_name, package_info); + let _ignore = managed.cache_package_info(&package_name).await; } - Ok(()) - } - .boxed_local() - } - _ => { - // return it succeeded and error at the import site below - Box::pin(future::ready(Ok(()))) + }); } + _ => {} } } async fn resolve_pkg_reqs( &self, - package_reqs: &[&PackageReq], - ) -> NpmPackageReqsResolution { + package_reqs: &[PackageReq], + ) -> NpmResolvePkgReqsResult { match &self.npm_resolver { Some(npm_resolver) => { let npm_resolver = match npm_resolver.as_inner() { @@ -849,37 +828,50 @@ impl<'a> deno_graph::source::NpmResolver for WorkerCliNpmGraphResolver<'a> { InnerCliNpmResolverRef::Byonm(_) => unreachable!(), }; - let reqs_with_package_infos = { - let memory_cache = self.memory_cache.borrow(); - package_reqs - .iter() - .map(|package_req| { - let package_info = memory_cache - .get(&package_req.name) - .unwrap() // ok because deno_graph would have called load_and_cache_npm_package_info - .clone(); - (*package_req, package_info) - }) - .collect::<Vec<_>>() + let top_level_result = if self.found_package_json_dep_flag.is_raised() { + npm_resolver.ensure_top_level_package_json_install().await + } else { + Ok(()) }; - let result = npm_resolver - .resolve_npm_for_deno_graph(&reqs_with_package_infos) - .await; - if matches!(result, NpmPackageReqsResolution::ReloadRegistryInfo) { - self.memory_cache.borrow_mut().clear(); + + let result = npm_resolver.add_package_reqs_raw(package_reqs).await; + + NpmResolvePkgReqsResult { + results: result + .results + .into_iter() + .map(|r| { + r.map_err(|err| match err { + NpmResolutionError::Registry(e) => { + NpmLoadError::RegistryInfo(Arc::new(e.into())) + } + NpmResolutionError::Resolution(e) => { + NpmLoadError::PackageReqResolution(Arc::new(e.into())) + } + NpmResolutionError::DependencyEntry(e) => { + NpmLoadError::PackageReqResolution(Arc::new(e.into())) + } + }) + }) + .collect(), + dep_graph_result: match top_level_result { + Ok(()) => result.dependencies_result.map_err(Arc::new), + Err(err) => Err(Arc::new(err)), + }, + } + } + None => { + let err = Arc::new(anyhow!( + "npm specifiers were requested; but --no-npm is specified" + )); + NpmResolvePkgReqsResult { + results: package_reqs + .iter() + .map(|_| Err(NpmLoadError::RegistryInfo(err.clone()))) + .collect(), + dep_graph_result: Err(err), } - result } - None => NpmPackageReqsResolution::Resolutions( - package_reqs - .iter() - .map(|_| { - Err(Arc::new(anyhow!( - "npm specifiers were requested; but --no-npm is specified" - ))) - }) - .collect(), - ), } } diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 91e524dda..b71e47ceb 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -21,7 +21,6 @@ use crate::npm::create_cli_npm_resolver; use crate::npm::CliNpmResolverByonmCreateOptions; use crate::npm::CliNpmResolverCreateOptions; use crate::npm::CliNpmResolverManagedCreateOptions; -use crate::npm::CliNpmResolverManagedPackageJsonInstallerOption; use crate::npm::CliNpmResolverManagedSnapshotOption; use crate::npm::NpmCacheDir; use crate::resolver::CjsResolutionStore; @@ -373,27 +372,27 @@ pub async fn run( )); let fs = Arc::new(DenoCompileFileSystem::new(vfs)) as Arc<dyn deno_fs::FileSystem>; - let npm_resolver = create_cli_npm_resolver( - CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { - snapshot: CliNpmResolverManagedSnapshotOption::Specified(Some(snapshot)), - maybe_lockfile: None, - fs: fs.clone(), - http_client_provider: http_client_provider.clone(), - npm_global_cache_dir, - cache_setting, - text_only_progress_bar: progress_bar, - maybe_node_modules_path, - package_json_installer: - CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( - package_json_deps_provider.clone(), - ), - npm_system_info: Default::default(), - // Packages from different registries are already inlined in the ESZip, - // so no need to create actual `.npmrc` configuration. - npmrc: create_default_npmrc(), - }), - ) - .await?; + let npm_resolver = + create_cli_npm_resolver(CliNpmResolverCreateOptions::Managed( + CliNpmResolverManagedCreateOptions { + snapshot: CliNpmResolverManagedSnapshotOption::Specified(Some( + snapshot, + )), + maybe_lockfile: None, + fs: fs.clone(), + http_client_provider: http_client_provider.clone(), + npm_global_cache_dir, + cache_setting, + text_only_progress_bar: progress_bar, + maybe_node_modules_path, + package_json_deps_provider: package_json_deps_provider.clone(), + npm_system_info: Default::default(), + // Packages from different registries are already inlined in the ESZip, + // so no need to create actual `.npmrc` configuration. + npmrc: create_default_npmrc(), + }, + )) + .await?; ( package_json_deps_provider, fs, @@ -431,27 +430,25 @@ pub async fn run( let package_json_deps_provider = Arc::new(PackageJsonDepsProvider::new(None)); let fs = Arc::new(deno_fs::RealFs) as Arc<dyn deno_fs::FileSystem>; - let npm_resolver = create_cli_npm_resolver( - CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { - snapshot: CliNpmResolverManagedSnapshotOption::Specified(None), - maybe_lockfile: None, - fs: fs.clone(), - http_client_provider: http_client_provider.clone(), - npm_global_cache_dir, - cache_setting, - text_only_progress_bar: progress_bar, - maybe_node_modules_path: None, - package_json_installer: - CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( - package_json_deps_provider.clone(), - ), - npm_system_info: Default::default(), - // Packages from different registries are already inlined in the ESZip, - // so no need to create actual `.npmrc` configuration. - npmrc: create_default_npmrc(), - }), - ) - .await?; + let npm_resolver = + create_cli_npm_resolver(CliNpmResolverCreateOptions::Managed( + CliNpmResolverManagedCreateOptions { + snapshot: CliNpmResolverManagedSnapshotOption::Specified(None), + maybe_lockfile: None, + fs: fs.clone(), + http_client_provider: http_client_provider.clone(), + npm_global_cache_dir, + cache_setting, + text_only_progress_bar: progress_bar, + maybe_node_modules_path: None, + package_json_deps_provider: package_json_deps_provider.clone(), + npm_system_info: Default::default(), + // Packages from different registries are already inlined in the ESZip, + // so no need to create actual `.npmrc` configuration. + npmrc: create_default_npmrc(), + }, + )) + .await?; (package_json_deps_provider, fs, npm_resolver, None) } }; diff --git a/cli/tools/task.rs b/cli/tools/task.rs index 2429e5600..6b18e18bb 100644 --- a/cli/tools/task.rs +++ b/cli/tools/task.rs @@ -122,7 +122,6 @@ pub async fn execute_script( if cli_options.has_node_modules_dir() { if let Some(npm_resolver) = npm_resolver.as_managed() { npm_resolver.ensure_top_level_package_json_install().await?; - npm_resolver.resolve_pending().await?; } } diff --git a/tests/integration/lsp_tests.rs b/tests/integration/lsp_tests.rs index 25fb695b4..62cb84457 100644 --- a/tests/integration/lsp_tests.rs +++ b/tests/integration/lsp_tests.rs @@ -12868,14 +12868,10 @@ fn lsp_uses_lockfile_for_npm_initialization() { client.initialize_default(); let mut skipping_count = 0; client.wait_until_stderr_line(|line| { - if line.contains("Skipping pending npm resolution.") { + if line.contains("Skipping npm resolution.") { skipping_count += 1; } - assert!( - !line.contains("Running pending npm resolution."), - "Line: {}", - line - ); + assert!(!line.contains("Running npm resolution."), "Line: {}", line); line.contains("Server ready.") }); assert_eq!(skipping_count, 1); diff --git a/tests/integration/npm_tests.rs b/tests/integration/npm_tests.rs index 2c074b86f..1d60d9e35 100644 --- a/tests/integration/npm_tests.rs +++ b/tests/integration/npm_tests.rs @@ -1822,6 +1822,7 @@ fn reload_info_not_found_cache_but_exists_remote() { .run(); output.assert_matches_text(concat!( "error: Could not find npm package '@denotest/esm-import-cjs-default' matching '1.0.0'.\n", + " at file:///[WILDCARD]/main.ts:1:8\n", )); output.assert_exit_code(1); diff --git a/tests/registry/npm/@denotest/dep-cannot-parse/1.0.0/package.json b/tests/registry/npm/@denotest/dep-cannot-parse/1.0.0/package.json new file mode 100644 index 000000000..8a069ef72 --- /dev/null +++ b/tests/registry/npm/@denotest/dep-cannot-parse/1.0.0/package.json @@ -0,0 +1,7 @@ +{ + "name": "@denotest/dep-cannot-parse", + "version": "1.0.0", + "dependencies": { + "@denotest/esm-basic": "unknown-scheme:unknown" + } +} diff --git a/tests/specs/npm/dynamic_npm_resolution_failure/__test__.jsonc b/tests/specs/npm/dynamic_npm_resolution_failure/__test__.jsonc new file mode 100644 index 000000000..f816bad86 --- /dev/null +++ b/tests/specs/npm/dynamic_npm_resolution_failure/__test__.jsonc @@ -0,0 +1,4 @@ +{ + "args": "run -A main.ts", + "output": "main.out" +} diff --git a/tests/specs/npm/dynamic_npm_resolution_failure/main.out b/tests/specs/npm/dynamic_npm_resolution_failure/main.out new file mode 100644 index 000000000..03c733567 --- /dev/null +++ b/tests/specs/npm/dynamic_npm_resolution_failure/main.out @@ -0,0 +1,15 @@ +[UNORDERED_START] +Download http://localhost:4260/chalk +Download http://localhost:4260/@denotest/dep-cannot-parse +[UNORDERED_END] +Download http://localhost:4260/chalk/chalk-5.0.1.tgz +Hi +TypeError: Error in @denotest/dep-cannot-parse@1.0.0 parsing version requirement for dependency: @denotest/esm-basic@unknown-scheme:unknown + +Invalid npm version requirement. Unexpected character. + unknown-scheme:unknown + ~ + at async file:///[WILDLINE]main.ts:5:3 { + code: "ERR_MODULE_NOT_FOUND" +} +Bye diff --git a/tests/specs/npm/dynamic_npm_resolution_failure/main.ts b/tests/specs/npm/dynamic_npm_resolution_failure/main.ts new file mode 100644 index 000000000..0096bca48 --- /dev/null +++ b/tests/specs/npm/dynamic_npm_resolution_failure/main.ts @@ -0,0 +1,9 @@ +import chalk from "npm:chalk"; + +console.log(chalk.green("Hi")); +try { + await import("npm:@denotest/dep-cannot-parse"); +} catch (err) { + console.log(err); +} +console.log(chalk.green("Bye")); |