From 8c1677ecbcbb474fc6a5ac9b5f73b562677bb829 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Tue, 3 Oct 2023 19:05:06 -0400 Subject: [PATCH] refactor(npm): break up `NpmModuleLoader` and move more methods into the managed `CliNpmResolver` (#20777) Part of https://github.com/denoland/deno/issues/18967 --- cli/factory.rs | 81 ++++++++------ cli/lsp/diagnostics.rs | 16 ++- cli/lsp/documents.rs | 20 ++-- cli/module_loader.rs | 160 +++++++++++++++++----------- cli/npm/common.rs | 23 ++++ cli/npm/managed/mod.rs | 119 ++++++++++----------- cli/npm/managed/resolvers/common.rs | 22 ---- cli/npm/managed/resolvers/global.rs | 2 +- cli/npm/managed/resolvers/local.rs | 2 +- cli/npm/mod.rs | 47 +++----- cli/resolver.rs | 25 +++-- cli/standalone/binary.rs | 4 +- cli/standalone/mod.rs | 32 +++--- cli/tools/check.rs | 48 ++++----- cli/tools/coverage/mod.rs | 14 ++- cli/tools/task.rs | 2 +- cli/tsc/mod.rs | 6 +- cli/worker.rs | 55 ++++++---- ext/node/package_json.rs | 13 +-- 19 files changed, 385 insertions(+), 306 deletions(-) create mode 100644 cli/npm/common.rs diff --git a/cli/factory.rs b/cli/factory.rs index 648d4a87d1..14f448b19f 100644 --- a/cli/factory.rs +++ b/cli/factory.rs @@ -25,6 +25,7 @@ use crate::graph_util::ModuleGraphContainer; use crate::http_util::HttpClient; use crate::module_loader::CjsResolutionStore; use crate::module_loader::CliModuleLoaderFactory; +use crate::module_loader::CliNodeResolver; use crate::module_loader::ModuleLoadPreparer; use crate::module_loader::NpmModuleLoader; use crate::node::CliCjsCodeAnalyzer; @@ -159,6 +160,7 @@ struct CliFactoryServices { text_only_progress_bar: Deferred, type_checker: Deferred>, cjs_resolutions: Deferred>, + cli_node_resolver: Deferred>, } pub struct CliFactory { @@ -294,35 +296,37 @@ impl CliFactory { .services .npm_resolver .get_or_try_init_async(async { - create_cli_npm_resolver(CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { - snapshot: match self.options.resolve_npm_resolution_snapshot()? { - Some(snapshot) => { - CliNpmResolverManagedSnapshotOption::Specified(Some(snapshot)) - } - None => match self.maybe_lockfile() { - Some(lockfile) => { - CliNpmResolverManagedSnapshotOption::ResolveFromLockfile( - lockfile.clone(), - ) + let fs = self.fs(); + create_cli_npm_resolver( + CliNpmResolverCreateOptions::Managed(CliNpmResolverManagedCreateOptions { + snapshot: match self.options.resolve_npm_resolution_snapshot()? { + Some(snapshot) => { + CliNpmResolverManagedSnapshotOption::Specified(Some(snapshot)) } - None => CliNpmResolverManagedSnapshotOption::Specified(None), + None => match self.maybe_lockfile() { + Some(lockfile) => { + CliNpmResolverManagedSnapshotOption::ResolveFromLockfile( + lockfile.clone(), + ) + } + None => CliNpmResolverManagedSnapshotOption::Specified(None), + }, }, - }, - maybe_lockfile: self.maybe_lockfile().as_ref().cloned(), - fs: self.fs().clone(), - http_client: self.http_client().clone(), - npm_global_cache_dir: self.deno_dir()?.npm_folder_path(), - 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(), - package_json_installer: - CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( - self.package_json_deps_provider().clone(), - ), - npm_system_info: self.options.npm_system_info(), - npm_registry_url: crate::args::npm_registry_default_url().to_owned(), - })) - .await + maybe_lockfile: self.maybe_lockfile().as_ref().cloned(), + fs: fs.clone(), + http_client: self.http_client().clone(), + npm_global_cache_dir: self.deno_dir()?.npm_folder_path(), + 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(), + package_json_installer: + CliNpmResolverManagedPackageJsonInstallerOption::ConditionalInstall( + self.package_json_deps_provider().clone(), + ), + npm_system_info: self.options.npm_system_info(), + npm_registry_url: crate::args::npm_registry_default_url().to_owned(), + }) + ).await }) .await } @@ -538,6 +542,22 @@ impl CliFactory { self.services.cjs_resolutions.get_or_init(Default::default) } + pub async fn cli_node_resolver( + &self, + ) -> Result<&Arc, AnyError> { + self + .services + .cli_node_resolver + .get_or_try_init_async(async { + Ok(Arc::new(CliNodeResolver::new( + self.cjs_resolutions().clone(), + self.node_resolver().await?.clone(), + self.npm_resolver().await?.clone(), + ))) + }) + .await + } + pub async fn create_compile_binary_writer( &self, ) -> Result { @@ -557,6 +577,7 @@ impl CliFactory { let node_resolver = self.node_resolver().await?; let npm_resolver = self.npm_resolver().await?; let fs = self.fs(); + let cli_node_resolver = self.cli_node_resolver().await?; Ok(CliMainWorkerFactory::new( StorageKeyResolver::from_options(&self.options), npm_resolver.clone(), @@ -569,12 +590,12 @@ impl CliFactory { self.module_load_preparer().await?.clone(), self.parsed_source_cache()?.clone(), self.resolver().await?.clone(), + cli_node_resolver.clone(), NpmModuleLoader::new( self.cjs_resolutions().clone(), self.node_code_translator().await?.clone(), fs.clone(), - node_resolver.clone(), - npm_resolver.clone(), + cli_node_resolver.clone(), ), )), self.root_cert_store_provider().clone(), @@ -618,7 +639,7 @@ impl CliFactory { .unsafely_ignore_certificate_errors() .clone(), unstable: self.options.unstable(), - maybe_package_json_deps: self.options.maybe_package_json_deps(), + maybe_root_package_json_deps: self.options.maybe_package_json_deps(), }) } } diff --git a/cli/lsp/diagnostics.rs b/cli/lsp/diagnostics.rs index 9ec273d7f2..cb368b1f25 100644 --- a/cli/lsp/diagnostics.rs +++ b/cli/lsp/diagnostics.rs @@ -1236,10 +1236,14 @@ fn diagnose_resolution( } else if let Ok(pkg_ref) = NpmPackageReqReference::from_specifier(specifier) { - if let Some(npm) = &snapshot.npm { + if let Some(npm_resolver) = snapshot + .npm + .as_ref() + .and_then(|n| n.npm_resolver.as_managed()) + { // show diagnostics for npm package references that aren't cached let req = pkg_ref.into_inner().req; - if !npm.npm_resolver.is_pkg_req_folder_cached(&req) { + if !npm_resolver.is_pkg_req_folder_cached(&req) { diagnostics .push(DenoDiagnostic::NoCacheNpm(req, specifier.clone())); } @@ -1249,10 +1253,14 @@ fn diagnose_resolution( if !deno_node::is_builtin_node_module(module_name) { diagnostics .push(DenoDiagnostic::InvalidNodeSpecifier(specifier.clone())); - } else if let Some(npm) = &snapshot.npm { + } else if let Some(npm_resolver) = snapshot + .npm + .as_ref() + .and_then(|n| n.npm_resolver.as_managed()) + { // check that a @types/node package exists in the resolver let types_node_req = PackageReq::from_str("@types/node").unwrap(); - if !npm.npm_resolver.is_pkg_req_folder_cached(&types_node_req) { + if !npm_resolver.is_pkg_req_folder_cached(&types_node_req) { diagnostics.push(DenoDiagnostic::NoCacheNpm( types_node_req, ModuleSpecifier::parse("npm:@types/node").unwrap(), diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 30f419249c..92c2e0a3bc 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1183,9 +1183,9 @@ impl Documents { dependencies.as_ref().and_then(|d| d.deps.get(&specifier)) { if let Some(specifier) = dep.maybe_type.maybe_specifier() { - results.push(self.resolve_dependency(specifier, maybe_npm)); + results.push(self.resolve_dependency(specifier, maybe_npm, referrer)); } else if let Some(specifier) = dep.maybe_code.maybe_specifier() { - results.push(self.resolve_dependency(specifier, maybe_npm)); + results.push(self.resolve_dependency(specifier, maybe_npm, referrer)); } else { results.push(None); } @@ -1193,11 +1193,15 @@ impl Documents { .resolve_imports_dependency(&specifier) .and_then(|r| r.maybe_specifier()) { - results.push(self.resolve_dependency(specifier, maybe_npm)); + results.push(self.resolve_dependency(specifier, maybe_npm, referrer)); } else if let Ok(npm_req_ref) = NpmPackageReqReference::from_str(&specifier) { - results.push(node_resolve_npm_req_ref(npm_req_ref, maybe_npm)); + results.push(node_resolve_npm_req_ref( + npm_req_ref, + maybe_npm, + referrer, + )); } else { results.push(None); } @@ -1528,6 +1532,7 @@ impl Documents { &self, specifier: &ModuleSpecifier, maybe_npm: Option<&StateNpmSnapshot>, + referrer: &ModuleSpecifier, ) -> Option<(ModuleSpecifier, MediaType)> { if let Some(module_name) = specifier.as_str().strip_prefix("node:") { if deno_node::is_builtin_node_module(module_name) { @@ -1539,7 +1544,7 @@ impl Documents { } if let Ok(npm_ref) = NpmPackageReqReference::from_specifier(specifier) { - return node_resolve_npm_req_ref(npm_ref, maybe_npm); + return node_resolve_npm_req_ref(npm_ref, maybe_npm, referrer); } let doc = self.get(specifier)?; let maybe_module = doc.maybe_esm_module().and_then(|r| r.as_ref().ok()); @@ -1548,7 +1553,7 @@ impl Documents { if let Some(specifier) = maybe_types_dependency.and_then(|d| d.maybe_specifier()) { - self.resolve_dependency(specifier, maybe_npm) + self.resolve_dependency(specifier, maybe_npm, referrer) } else { let media_type = doc.media_type(); Some((doc.specifier().clone(), media_type)) @@ -1572,12 +1577,13 @@ impl Documents { fn node_resolve_npm_req_ref( npm_req_ref: NpmPackageReqReference, maybe_npm: Option<&StateNpmSnapshot>, + referrer: &ModuleSpecifier, ) -> Option<(ModuleSpecifier, MediaType)> { maybe_npm.map(|npm| { NodeResolution::into_specifier_and_media_type( npm .npm_resolver - .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req()) + .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req(), referrer) .ok() .and_then(|package_folder| { npm diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 4a694e6155..51b9ea598f 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -50,10 +50,10 @@ use deno_runtime::deno_node::NodeResolution; use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::NodeResolver; use deno_runtime::permissions::PermissionsContainer; -use deno_semver::npm::NpmPackageNvReference; use deno_semver::npm::NpmPackageReqReference; use std::borrow::Cow; use std::collections::HashSet; +use std::path::Path; use std::pin::Pin; use std::rc::Rc; use std::str; @@ -309,6 +309,7 @@ struct SharedCliModuleLoaderState { module_load_preparer: Arc, prepared_module_loader: PreparedModuleLoader, resolver: Arc, + node_resolver: Arc, npm_module_loader: NpmModuleLoader, } @@ -317,6 +318,7 @@ pub struct CliModuleLoaderFactory { } impl CliModuleLoaderFactory { + #[allow(clippy::too_many_arguments)] pub fn new( options: &CliOptions, emitter: Arc, @@ -324,6 +326,7 @@ impl CliModuleLoaderFactory { module_load_preparer: Arc, parsed_source_cache: Arc, resolver: Arc, + node_resolver: Arc, npm_module_loader: NpmModuleLoader, ) -> Self { Self { @@ -343,6 +346,7 @@ impl CliModuleLoaderFactory { graph_container, module_load_preparer, resolver, + node_resolver, npm_module_loader, }), } @@ -471,11 +475,11 @@ impl ModuleLoader for CliModuleLoader { let referrer_result = deno_core::resolve_url_or_path(referrer, &cwd); if let Ok(referrer) = referrer_result.as_ref() { - if let Some(result) = self - .shared - .npm_module_loader - .resolve_if_in_npm_package(specifier, referrer, permissions) - { + if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( + specifier, + referrer, + permissions, + ) { return result; } @@ -492,10 +496,28 @@ impl ModuleLoader for CliModuleLoader { let specifier = &resolved.specifier; return match graph.get(specifier) { - Some(Module::Npm(module)) => self - .shared - .npm_module_loader - .resolve_nv_ref(&module.nv_reference, permissions), + Some(Module::Npm(module)) => { + let package_folder = self + .shared + .node_resolver + .npm_resolver + .as_managed() + .unwrap() // byonm won't create a Module::Npm + .resolve_pkg_folder_from_deno_module( + module.nv_reference.nv(), + )?; + self + .shared + .node_resolver + .resolve_package_sub_path( + &package_folder, + module.nv_reference.sub_path(), + permissions, + ) + .with_context(|| { + format!("Could not resolve '{}'.", module.nv_reference) + }) + } Some(Module::Node(module)) => Ok(module.specifier.clone()), Some(Module::Esm(module)) => Ok(module.specifier.clone()), Some(Module::Json(module)) => Ok(module.specifier.clone()), @@ -538,10 +560,11 @@ impl ModuleLoader for CliModuleLoader { if let Ok(reference) = NpmPackageReqReference::from_specifier(&specifier) { - return self - .shared - .npm_module_loader - .resolve_req_reference(&reference, permissions); + return self.shared.node_resolver.resolve_req_reference( + &reference, + permissions, + &referrer, + ); } } } @@ -642,38 +665,36 @@ impl SourceMapGetter for CliSourceMapGetter { } } -pub struct NpmModuleLoader { +pub struct CliNodeResolver { cjs_resolutions: Arc, - node_code_translator: Arc, - fs: Arc, node_resolver: Arc, npm_resolver: Arc, } -impl NpmModuleLoader { +impl CliNodeResolver { pub fn new( cjs_resolutions: Arc, - node_code_translator: Arc, - fs: Arc, node_resolver: Arc, npm_resolver: Arc, ) -> Self { Self { cjs_resolutions, - node_code_translator, - fs, node_resolver, npm_resolver, } } + pub fn in_npm_package(&self, referrer: &ModuleSpecifier) -> bool { + self.npm_resolver.in_npm_package(referrer) + } + pub fn resolve_if_in_npm_package( &self, specifier: &str, referrer: &ModuleSpecifier, permissions: &PermissionsContainer, ) -> Option> { - if self.node_resolver.in_npm_package(referrer) { + if self.in_npm_package(referrer) { // we're in an npm package, so use node resolution Some( self @@ -692,42 +713,76 @@ impl NpmModuleLoader { } } - pub fn resolve_nv_ref( - &self, - nv_ref: &NpmPackageNvReference, - permissions: &PermissionsContainer, - ) -> Result { - let package_folder = self - .npm_resolver - .resolve_pkg_folder_from_deno_module(nv_ref.nv())?; - self - .handle_node_resolve_result(self.node_resolver.resolve_npm_reference( - &package_folder, - nv_ref.sub_path(), - NodeResolutionMode::Execution, - permissions, - )) - .with_context(|| format!("Could not resolve '{}'.", nv_ref)) - } - pub fn resolve_req_reference( &self, req_ref: &NpmPackageReqReference, permissions: &PermissionsContainer, + referrer: &ModuleSpecifier, ) -> Result { let package_folder = self .npm_resolver - .resolve_pkg_folder_from_deno_module_req(req_ref.req())?; + .resolve_pkg_folder_from_deno_module_req(req_ref.req(), referrer)?; self - .handle_node_resolve_result(self.node_resolver.resolve_npm_reference( + .resolve_package_sub_path( &package_folder, req_ref.sub_path(), - NodeResolutionMode::Execution, permissions, - )) + ) .with_context(|| format!("Could not resolve '{}'.", req_ref)) } + fn resolve_package_sub_path( + &self, + package_folder: &Path, + sub_path: Option<&str>, + permissions: &PermissionsContainer, + ) -> Result { + self.handle_node_resolve_result(self.node_resolver.resolve_npm_reference( + package_folder, + sub_path, + NodeResolutionMode::Execution, + permissions, + )) + } + + fn handle_node_resolve_result( + &self, + result: Result, AnyError>, + ) -> Result { + let response = match result? { + Some(response) => response, + None => return Err(generic_error("not found")), + }; + if let NodeResolution::CommonJs(specifier) = &response { + // remember that this was a common js resolution + self.cjs_resolutions.insert(specifier.clone()); + } + Ok(response.into_url()) + } +} + +pub struct NpmModuleLoader { + cjs_resolutions: Arc, + node_code_translator: Arc, + fs: Arc, + node_resolver: Arc, +} + +impl NpmModuleLoader { + pub fn new( + cjs_resolutions: Arc, + node_code_translator: Arc, + fs: Arc, + node_resolver: Arc, + ) -> Self { + Self { + cjs_resolutions, + node_code_translator, + fs, + node_resolver, + } + } + pub fn maybe_prepare_load( &self, specifier: &ModuleSpecifier, @@ -812,21 +867,6 @@ impl NpmModuleLoader { media_type: MediaType::from_specifier(specifier), }) } - - fn handle_node_resolve_result( - &self, - result: Result, AnyError>, - ) -> Result { - let response = match result? { - Some(response) => response, - None => return Err(generic_error("not found")), - }; - if let NodeResolution::CommonJs(specifier) = &response { - // remember that this was a common js resolution - self.cjs_resolutions.insert(specifier.clone()); - } - Ok(response.into_url()) - } } /// Keeps track of what module specifiers were resolved as CJS. diff --git a/cli/npm/common.rs b/cli/npm/common.rs new file mode 100644 index 0000000000..c7ae55f8f7 --- /dev/null +++ b/cli/npm/common.rs @@ -0,0 +1,23 @@ +// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. + +/// Gets the corresponding @types package for the provided package name. +pub fn types_package_name(package_name: &str) -> String { + debug_assert!(!package_name.starts_with("@types/")); + // Scoped packages will get two underscores for each slash + // https://github.com/DefinitelyTyped/DefinitelyTyped/tree/15f1ece08f7b498f4b9a2147c2a46e94416ca777#what-about-scoped-packages + format!("@types/{}", package_name.replace('/', "__")) +} + +#[cfg(test)] +mod test { + use super::types_package_name; + + #[test] + fn test_types_package_name() { + assert_eq!(types_package_name("name"), "@types/name"); + assert_eq!( + types_package_name("@scoped/package"), + "@types/@scoped__package" + ); + } +} diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs index df9ad59acd..b85f1130f6 100644 --- a/cli/npm/managed/mod.rs +++ b/cli/npm/managed/mod.rs @@ -1,6 +1,5 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -23,15 +22,13 @@ use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::NodePermissions; use deno_runtime::deno_node::NodeResolutionMode; use deno_runtime::deno_node::NpmResolver; -use deno_semver::npm::NpmPackageNvReference; -use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageNv; -use deno_semver::package::PackageNvReference; use deno_semver::package::PackageReq; use crate::args::Lockfile; use crate::args::NpmProcessState; use crate::args::PackageJsonDepsProvider; +use crate::cache::FastInsecureHasher; use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs; use crate::util::progress_bar::ProgressBar; @@ -345,6 +342,16 @@ impl ManagedCliNpmResolver { self.resolution.all_system_packages(system_info) } + /// Checks if the provided package req's folder is cached. + pub fn is_pkg_req_folder_cached(&self, req: &PackageReq) -> bool { + self + .resolve_pkg_id_from_pkg_req(req) + .ok() + .and_then(|id| self.fs_resolver.package_folder(&id).ok()) + .map(|folder| folder.exists()) + .unwrap_or(false) + } + /// Adds package requirements to the resolver and ensures everything is setup. pub async fn add_package_reqs( &self, @@ -413,6 +420,35 @@ impl ManagedCliNpmResolver { self.fs_resolver.cache_packages().await } + /// Resolves a package requirement for deno graph. This should only be + /// called by deno_graph's NpmResolver or for resolving packages in + /// a package.json + pub fn resolve_npm_for_deno_graph( + &self, + pkg_req: &PackageReq, + ) -> NpmPackageReqResolution { + let result = self.resolution.resolve_pkg_req_as_pending(pkg_req); + match result { + Ok(nv) => NpmPackageReqResolution::Ok(nv), + Err(err) => { + if self.api.mark_force_reload() { + log::debug!("Restarting npm specifier resolution to check for new registry information. Error: {:#}", err); + NpmPackageReqResolution::ReloadRegistryInfo(err.into()) + } else { + NpmPackageReqResolution::Err(err.into()) + } + } + } + } + + pub fn resolve_pkg_folder_from_deno_module( + &self, + nv: &PackageNv, + ) -> Result { + let pkg_id = self.resolution.resolve_pkg_id_from_deno_module(nv)?; + self.resolve_pkg_folder_from_pkg_id(&pkg_id) + } + fn resolve_pkg_id_from_pkg_req( &self, req: &PackageReq, @@ -513,7 +549,7 @@ impl CliNpmResolver for ManagedCliNpmResolver { &self.progress_bar, self.api.base_url().clone(), npm_resolution, - self.node_modules_path(), + self.root_node_modules_path(), self.npm_system_info.clone(), ), self.global_npm_cache.clone(), @@ -524,59 +560,14 @@ impl CliNpmResolver for ManagedCliNpmResolver { )) } - fn root_dir_url(&self) -> &Url { - self.fs_resolver.root_dir_url() - } - fn as_inner(&self) -> InnerCliNpmResolverRef { InnerCliNpmResolverRef::Managed(self) } - fn node_modules_path(&self) -> Option { + fn root_node_modules_path(&self) -> Option { self.fs_resolver.node_modules_path() } - /// Checks if the provided package req's folder is cached. - fn is_pkg_req_folder_cached(&self, req: &PackageReq) -> bool { - self - .resolve_pkg_id_from_pkg_req(req) - .ok() - .and_then(|id| self.fs_resolver.package_folder(&id).ok()) - .map(|folder| folder.exists()) - .unwrap_or(false) - } - - fn resolve_npm_for_deno_graph( - &self, - pkg_req: &PackageReq, - ) -> NpmPackageReqResolution { - let result = self.resolution.resolve_pkg_req_as_pending(pkg_req); - match result { - Ok(nv) => NpmPackageReqResolution::Ok(nv), - Err(err) => { - if self.api.mark_force_reload() { - log::debug!("Restarting npm specifier resolution to check for new registry information. Error: {:#}", err); - NpmPackageReqResolution::ReloadRegistryInfo(err.into()) - } else { - NpmPackageReqResolution::Err(err.into()) - } - } - } - } - - fn resolve_pkg_nv_ref_from_pkg_req_ref( - &self, - req_ref: &NpmPackageReqReference, - ) -> Result { - let pkg_nv = self - .resolve_pkg_id_from_pkg_req(req_ref.req()) - .map(|id| id.nv)?; - Ok(NpmPackageNvReference::new(PackageNvReference { - nv: pkg_nv, - sub_path: req_ref.sub_path().map(|s| s.to_string()), - })) - } - /// Resolve the root folder of the package the provided specifier is in. /// /// This will error when the provided specifier is not in an npm package. @@ -601,19 +592,12 @@ impl CliNpmResolver for ManagedCliNpmResolver { fn resolve_pkg_folder_from_deno_module_req( &self, req: &PackageReq, + _referrer: &ModuleSpecifier, ) -> Result { let pkg_id = self.resolve_pkg_id_from_pkg_req(req)?; self.resolve_pkg_folder_from_pkg_id(&pkg_id) } - fn resolve_pkg_folder_from_deno_module( - &self, - nv: &PackageNv, - ) -> Result { - let pkg_id = self.resolution.resolve_pkg_id_from_deno_module(nv)?; - self.resolve_pkg_folder_from_pkg_id(&pkg_id) - } - /// Gets the state of npm for the process. fn get_npm_process_state(&self) -> String { serde_json::to_string(&NpmProcessState { @@ -629,7 +613,20 @@ impl CliNpmResolver for ManagedCliNpmResolver { .unwrap() } - fn package_reqs(&self) -> HashMap { - self.resolution.package_reqs() + fn check_state_hash(&self) -> Option { + // We could go further and check all the individual + // npm packages, but that's probably overkill. + let mut package_reqs = self + .resolution + .package_reqs() + .into_iter() + .collect::>(); + package_reqs.sort_by(|a, b| a.0.cmp(&b.0)); // determinism + let mut hasher = FastInsecureHasher::new(); + for (pkg_req, pkg_nv) in package_reqs { + hasher.write_hashable(&pkg_req); + hasher.write_hashable(&pkg_nv); + } + Some(hasher.finish()) } } diff --git a/cli/npm/managed/resolvers/common.rs b/cli/npm/managed/resolvers/common.rs index b0f375779d..41b5d8a963 100644 --- a/cli/npm/managed/resolvers/common.rs +++ b/cli/npm/managed/resolvers/common.rs @@ -149,25 +149,3 @@ pub async fn cache_packages( } Ok(()) } - -/// Gets the corresponding @types package for the provided package name. -pub fn types_package_name(package_name: &str) -> String { - debug_assert!(!package_name.starts_with("@types/")); - // Scoped packages will get two underscores for each slash - // https://github.com/DefinitelyTyped/DefinitelyTyped/tree/15f1ece08f7b498f4b9a2147c2a46e94416ca777#what-about-scoped-packages - format!("@types/{}", package_name.replace('/', "__")) -} - -#[cfg(test)] -mod test { - use super::types_package_name; - - #[test] - fn test_types_package_name() { - assert_eq!(types_package_name("name"), "@types/name"); - assert_eq!( - types_package_name("@scoped/package"), - "@types/@scoped__package" - ); - } -} diff --git a/cli/npm/managed/resolvers/global.rs b/cli/npm/managed/resolvers/global.rs index 3f042a38b6..7a51cead25 100644 --- a/cli/npm/managed/resolvers/global.rs +++ b/cli/npm/managed/resolvers/global.rs @@ -20,10 +20,10 @@ use deno_runtime::deno_fs::FileSystem; use deno_runtime::deno_node::NodePermissions; use deno_runtime::deno_node::NodeResolutionMode; +use super::super::super::common::types_package_name; use super::super::cache::NpmCache; use super::super::resolution::NpmResolution; use super::common::cache_packages; -use super::common::types_package_name; use super::common::NpmPackageFsResolver; use super::common::RegistryReadPermissionChecker; diff --git a/cli/npm/managed/resolvers/local.rs b/cli/npm/managed/resolvers/local.rs index 8e4d72f269..2d774518a7 100644 --- a/cli/npm/managed/resolvers/local.rs +++ b/cli/npm/managed/resolvers/local.rs @@ -45,9 +45,9 @@ use crate::npm::cache_dir::mixed_case_package_name_encode; use crate::util::fs::copy_dir_recursive; use crate::util::fs::hard_link_dir_recursive; +use super::super::super::common::types_package_name; use super::super::cache::NpmCache; use super::super::resolution::NpmResolution; -use super::common::types_package_name; use super::common::NpmPackageFsResolver; use super::common::RegistryReadPermissionChecker; diff --git a/cli/npm/mod.rs b/cli/npm/mod.rs index 22997a8b25..81f46419ed 100644 --- a/cli/npm/mod.rs +++ b/cli/npm/mod.rs @@ -1,21 +1,15 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. mod cache_dir; +mod common; mod managed; -use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; use deno_ast::ModuleSpecifier; use deno_core::error::AnyError; -use deno_core::url::Url; -use deno_graph::NpmPackageReqResolution; -use deno_npm::resolution::PackageReqNotFoundError; use deno_runtime::deno_node::NpmResolver; -use deno_semver::npm::NpmPackageNvReference; -use deno_semver::npm::NpmPackageReqReference; -use deno_semver::package::PackageNv; use deno_semver::package::PackageReq; pub use self::cache_dir::NpmCacheDir; @@ -64,8 +58,6 @@ pub trait CliNpmResolver: NpmResolver { fn clone_snapshotted(&self) -> Arc; - fn root_dir_url(&self) -> &Url; - fn as_inner(&self) -> InnerCliNpmResolverRef; fn as_managed(&self) -> Option<&ManagedCliNpmResolver> { @@ -75,27 +67,16 @@ pub trait CliNpmResolver: NpmResolver { } } - fn node_modules_path(&self) -> Option; + fn as_byonm(&self) -> Option<&ByonmCliNpmResolver> { + match self.as_inner() { + InnerCliNpmResolverRef::Managed(_) => None, + InnerCliNpmResolverRef::Byonm(inner) => Some(inner), + } + } - /// Checks if the provided package req's folder is cached. - fn is_pkg_req_folder_cached(&self, req: &PackageReq) -> bool; - - /// 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 - fn resolve_npm_for_deno_graph( - &self, - pkg_req: &PackageReq, - ) -> NpmPackageReqResolution; - - fn resolve_pkg_nv_ref_from_pkg_req_ref( - &self, - req_ref: &NpmPackageReqReference, - ) -> Result; + fn root_node_modules_path(&self) -> Option; /// Resolve the root folder of the package the provided specifier is in. - /// - /// This will error when the provided specifier is not in an npm package. fn resolve_pkg_folder_from_specifier( &self, specifier: &ModuleSpecifier, @@ -104,19 +85,15 @@ pub trait CliNpmResolver: NpmResolver { fn resolve_pkg_folder_from_deno_module_req( &self, req: &PackageReq, - ) -> Result; - - fn resolve_pkg_folder_from_deno_module( - &self, - nv: &PackageNv, + referrer: &ModuleSpecifier, ) -> Result; /// Gets the state of npm for the process. fn get_npm_process_state(&self) -> String; - // todo(#18967): should instead return a hash state of the resolver - // or perhaps this could be non-BYONM only and byonm always runs deno check - fn package_reqs(&self) -> HashMap; + /// Returns a hash returning the state of the npm resolver + /// or `None` if the state currently can't be determined. + fn check_state_hash(&self) -> Option; } // todo(#18967): implement this diff --git a/cli/resolver.rs b/cli/resolver.rs index cc1059aed6..ebb808c6bc 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -22,6 +22,7 @@ use crate::args::package_json::PackageJsonDeps; use crate::args::JsxImportSourceConfig; use crate::args::PackageJsonDepsProvider; use crate::npm::CliNpmResolver; +use crate::npm::InnerCliNpmResolverRef; use crate::util::sync::AtomicFlag; /// Result of checking if a specifier is mapped via @@ -118,10 +119,10 @@ impl CliGraphResolver { options: CliGraphResolverOptions, ) -> Self { Self { - mapped_specifier_resolver: MappedSpecifierResolver { - maybe_import_map: options.maybe_import_map, + mapped_specifier_resolver: MappedSpecifierResolver::new( + options.maybe_import_map, package_json_deps_provider, - }, + ), maybe_default_jsx_import_source: options .maybe_jsx_import_source_config .as_ref() @@ -167,19 +168,18 @@ impl Resolver for CliGraphResolver { specifier: &str, referrer: &ModuleSpecifier, ) -> Result { - use MappedResolution::*; let result = match self .mapped_specifier_resolver .resolve(specifier, referrer)? { - ImportMap(specifier) => Ok(specifier), - PackageJson(specifier) => { + MappedResolution::ImportMap(specifier) => Ok(specifier), + MappedResolution::PackageJson(specifier) => { // found a specifier in the package.json, so mark that // we need to do an "npm install" later self.found_package_json_dep_flag.raise(); Ok(specifier) } - None => deno_graph::resolve_import(specifier, referrer) + MappedResolution::None => deno_graph::resolve_import(specifier, referrer) .map_err(|err| err.into()), }; @@ -263,9 +263,14 @@ impl NpmResolver for CliGraphResolver { fn resolve_npm(&self, package_req: &PackageReq) -> NpmPackageReqResolution { match &self.npm_resolver { - Some(npm_resolver) => { - npm_resolver.resolve_npm_for_deno_graph(package_req) - } + Some(npm_resolver) => match npm_resolver.as_inner() { + InnerCliNpmResolverRef::Managed(npm_resolver) => { + npm_resolver.resolve_npm_for_deno_graph(package_req) + } + // if we are using byonm, then this should never be called because + // we don't use deno_graph's npm resolution in this case + InnerCliNpmResolverRef::Byonm(_) => unreachable!(), + }, None => NpmPackageReqResolution::Err(anyhow!( "npm specifiers were requested; but --no-npm is specified" )), diff --git a/cli/standalone/binary.rs b/cli/standalone/binary.rs index 412cea12c1..38ef3648e4 100644 --- a/cli/standalone/binary.rs +++ b/cli/standalone/binary.rs @@ -523,7 +523,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { ca_data, entrypoint: entrypoint.clone(), maybe_import_map, - node_modules_dir: self.npm_resolver.node_modules_path().is_some(), + node_modules_dir: self.npm_resolver.root_node_modules_path().is_some(), package_json_deps: self .package_json_deps_provider .deps() @@ -543,7 +543,7 @@ impl<'a> DenoCompileBinaryWriter<'a> { fn build_vfs(&self) -> Result { match self.npm_resolver.as_inner() { InnerCliNpmResolverRef::Managed(npm_resolver) => { - if let Some(node_modules_path) = npm_resolver.node_modules_path() { + if let Some(node_modules_path) = npm_resolver.root_node_modules_path() { let mut builder = VfsBuilder::new(node_modules_path.clone())?; builder.add_dir_recursive(&node_modules_path)?; Ok(builder) diff --git a/cli/standalone/mod.rs b/cli/standalone/mod.rs index 64d56e6a51..e537f9e0c6 100644 --- a/cli/standalone/mod.rs +++ b/cli/standalone/mod.rs @@ -12,6 +12,7 @@ use crate::cache::NodeAnalysisCache; use crate::file_fetcher::get_source_from_data_url; use crate::http_util::HttpClient; use crate::module_loader::CjsResolutionStore; +use crate::module_loader::CliNodeResolver; use crate::module_loader::NpmModuleLoader; use crate::node::CliCjsCodeAnalyzer; use crate::npm::create_cli_npm_resolver; @@ -67,6 +68,7 @@ use self::file_system::DenoCompileFileSystem; struct SharedModuleLoaderState { eszip: eszip::EszipV2, mapped_specifier_resolver: MappedSpecifierResolver, + node_resolver: Arc, npm_module_loader: Arc, } @@ -104,11 +106,11 @@ impl ModuleLoader for EmbeddedModuleLoader { } else { &self.root_permissions }; - if let Some(result) = self - .shared - .npm_module_loader - .resolve_if_in_npm_package(specifier, &referrer, permissions) - { + if let Some(result) = self.shared.node_resolver.resolve_if_in_npm_package( + specifier, + &referrer, + permissions, + ) { return result; } @@ -124,10 +126,11 @@ impl ModuleLoader for EmbeddedModuleLoader { .map(|r| r.as_str()) .unwrap_or(specifier); if let Ok(reference) = NpmPackageReqReference::from_str(specifier_text) { - return self - .shared - .npm_module_loader - .resolve_req_reference(&reference, permissions); + return self.shared.node_resolver.resolve_req_reference( + &reference, + permissions, + &referrer, + ); } match maybe_mapped { @@ -380,6 +383,11 @@ pub async fn run( let maybe_import_map = metadata.maybe_import_map.map(|(base, source)| { Arc::new(parse_from_json(&base, &source).unwrap().import_map) }); + let cli_node_resolver = Arc::new(CliNodeResolver::new( + cjs_resolutions.clone(), + node_resolver.clone(), + npm_resolver.clone(), + )); let module_loader_factory = StandaloneModuleLoaderFactory { shared: Arc::new(SharedModuleLoaderState { eszip, @@ -387,12 +395,12 @@ pub async fn run( maybe_import_map.clone(), package_json_deps_provider.clone(), ), + node_resolver: cli_node_resolver.clone(), npm_module_loader: Arc::new(NpmModuleLoader::new( cjs_resolutions, node_code_translator, fs.clone(), - node_resolver.clone(), - npm_resolver.clone(), + cli_node_resolver, )), }), }; @@ -447,7 +455,7 @@ pub async fn run( unsafely_ignore_certificate_errors: metadata .unsafely_ignore_certificate_errors, unstable: metadata.unstable, - maybe_package_json_deps: package_json_deps_provider.deps().cloned(), + maybe_root_package_json_deps: package_json_deps_provider.deps().cloned(), }, ); diff --git a/cli/tools/check.rs b/cli/tools/check.rs index 0a25518e45..e960439a6e 100644 --- a/cli/tools/check.rs +++ b/cli/tools/check.rs @@ -1,6 +1,5 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. -use std::collections::HashMap; use std::collections::HashSet; use std::sync::Arc; @@ -11,8 +10,6 @@ use deno_graph::Module; use deno_graph::ModuleGraph; use deno_runtime::colors; use deno_runtime::deno_node::NodeResolver; -use deno_semver::package::PackageNv; -use deno_semver::package::PackageReq; use once_cell::sync::Lazy; use regex::Regex; @@ -95,19 +92,28 @@ impl TypeChecker { let debug = self.cli_options.log_level() == Some(log::Level::Debug); let cache = TypeCheckCache::new(self.caches.type_checking_cache_db()); let check_js = ts_config.get_check_js(); - let check_hash = match get_check_hash( - &graph, - self.npm_resolver.package_reqs(), - type_check_mode, - &ts_config, - ) { - CheckHashResult::NoFiles => return Ok(()), - CheckHashResult::Hash(hash) => hash, + let maybe_check_hash = match self.npm_resolver.check_state_hash() { + Some(npm_check_hash) => { + match get_check_hash( + &graph, + npm_check_hash, + type_check_mode, + &ts_config, + ) { + CheckHashResult::NoFiles => return Ok(()), + CheckHashResult::Hash(hash) => Some(hash), + } + } + None => None, // we can't determine a check hash }; // do not type check if we know this is type checked - if !options.reload && cache.has_check_hash(check_hash) { - return Ok(()); + if !options.reload { + if let Some(check_hash) = maybe_check_hash { + if cache.has_check_hash(check_hash) { + return Ok(()); + } + } } for root in &graph.roots { @@ -174,7 +180,9 @@ impl TypeChecker { } if diagnostics.is_empty() { - cache.add_check_hash(check_hash); + if let Some(check_hash) = maybe_check_hash { + cache.add_check_hash(check_hash); + } } log::debug!("{}", response.stats); @@ -196,7 +204,7 @@ enum CheckHashResult { /// be used to tell fn get_check_hash( graph: &ModuleGraph, - package_reqs: HashMap, + package_reqs_hash: u64, type_check_mode: TypeCheckMode, ts_config: &TsConfig, ) -> CheckHashResult { @@ -270,15 +278,7 @@ fn get_check_hash( } } - // Check if any of the top level npm packages have changed. We could go - // further and check all the individual npm packages, but that's - // probably overkill. - let mut package_reqs = package_reqs.into_iter().collect::>(); - package_reqs.sort_by(|a, b| a.0.cmp(&b.0)); // determinism - for (pkg_req, pkg_nv) in package_reqs { - hasher.write_hashable(&pkg_req); - hasher.write_hashable(&pkg_nv); - } + hasher.write_hashable(package_reqs_hash); if !has_file || !check_js && !has_file_to_type_check { // no files to type check diff --git a/cli/tools/coverage/mod.rs b/cli/tools/coverage/mod.rs index 2b8e67c822..23aef89fbc 100644 --- a/cli/tools/coverage/mod.rs +++ b/cli/tools/coverage/mod.rs @@ -5,6 +5,7 @@ use crate::args::FileFlags; use crate::args::Flags; use crate::colors; use crate::factory::CliFactory; +use crate::npm::CliNpmResolver; use crate::tools::fmt::format_json; use crate::tools::test::is_supported_test_path; use crate::util::fs::FileCollector; @@ -601,7 +602,7 @@ fn filter_coverages( coverages: Vec, include: Vec, exclude: Vec, - npm_root_dir: &str, + npm_resolver: &dyn CliNpmResolver, ) -> Vec { let include: Vec = include.iter().map(|e| Regex::new(e).unwrap()).collect(); @@ -613,11 +614,14 @@ fn filter_coverages( .into_iter() .filter(|e| { let is_internal = e.url.starts_with("ext:") - || e.url.starts_with(npm_root_dir) || e.url.ends_with("__anonymous__") || e.url.ends_with("$deno$test.js") || e.url.ends_with(".snap") - || is_supported_test_path(Path::new(e.url.as_str())); + || is_supported_test_path(Path::new(e.url.as_str())) + || Url::parse(&e.url) + .ok() + .map(|url| npm_resolver.in_npm_package(&url)) + .unwrap_or(false); let is_included = include.iter().any(|p| p.is_match(&e.url)); let is_excluded = exclude.iter().any(|p| p.is_match(&e.url)); @@ -636,7 +640,7 @@ pub async fn cover_files( } let factory = CliFactory::from_flags(flags).await?; - let root_dir_url = factory.npm_resolver().await?.root_dir_url(); + let npm_resolver = factory.npm_resolver().await?; let file_fetcher = factory.file_fetcher()?; let cli_options = factory.cli_options(); let emitter = factory.emitter()?; @@ -646,7 +650,7 @@ pub async fn cover_files( script_coverages, coverage_flags.include, coverage_flags.exclude, - root_dir_url.as_str(), + npm_resolver.as_ref(), ); let proc_coverages: Vec<_> = script_coverages diff --git a/cli/tools/task.rs b/cli/tools/task.rs index 93e78dc568..f80a32577d 100644 --- a/cli/tools/task.rs +++ b/cli/tools/task.rs @@ -126,7 +126,7 @@ pub async fn execute_script( } None => Default::default(), }; - let env_vars = match npm_resolver.node_modules_path() { + let env_vars = match npm_resolver.root_node_modules_path() { Some(dir_path) => collect_env_vars_with_node_modules_dir(&dir_path), None => collect_env_vars(), }; diff --git a/cli/tsc/mod.rs b/cli/tsc/mod.rs index cd3d9ecae4..c7371e0c54 100644 --- a/cli/tsc/mod.rs +++ b/cli/tsc/mod.rs @@ -657,9 +657,11 @@ fn resolve_graph_specifier_types( Ok(Some((module.specifier.clone(), module.media_type))) } Some(Module::Npm(module)) => { - if let Some(npm) = &state.maybe_npm { + if let Some(npm) = &state.maybe_npm.as_ref() { let package_folder = npm .npm_resolver + .as_managed() + .unwrap() // should never be byonm because it won't create Module::Npm .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?; let maybe_resolution = npm.node_resolver.resolve_npm_reference( &package_folder, @@ -718,7 +720,7 @@ fn resolve_non_graph_specifier_types( // injected and not part of the graph let package_folder = npm .npm_resolver - .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req())?; + .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req(), referrer)?; let maybe_resolution = node_resolver.resolve_npm_reference( &package_folder, npm_req_ref.sub_path(), diff --git a/cli/worker.rs b/cli/worker.rs index d4277c618b..7a0cb53280 100644 --- a/cli/worker.rs +++ b/cli/worker.rs @@ -1,5 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. +use std::path::Path; use std::path::PathBuf; use std::rc::Rc; use std::sync::Arc; @@ -39,7 +40,6 @@ use deno_runtime::worker::MainWorker; use deno_runtime::worker::WorkerOptions; use deno_runtime::BootstrapOptions; use deno_runtime::WorkerLogLevel; -use deno_semver::npm::NpmPackageNvReference; use deno_semver::npm::NpmPackageReqReference; use deno_semver::package::PackageReqReference; @@ -92,7 +92,7 @@ pub struct CliMainWorkerOptions { pub seed: Option, pub unsafely_ignore_certificate_errors: Option>, pub unstable: bool, - pub maybe_package_json_deps: Option, + pub maybe_root_package_json_deps: Option, } struct SharedWorkerState { @@ -365,7 +365,7 @@ impl CliMainWorkerFactory { // package.json deps in order to prevent adding new dependency version shared .options - .maybe_package_json_deps + .maybe_root_package_json_deps .as_ref() .and_then(|deps| { deps @@ -388,11 +388,23 @@ impl CliMainWorkerFactory { .add_package_reqs(&[package_ref.req().clone()]) .await?; } - let package_ref = shared + + // use a fake referrer that can be used to discover the package.json if necessary + let referrer = + ModuleSpecifier::from_directory_path(self.shared.fs.cwd()?) + .unwrap() + .join("package.json")?; + let package_folder = shared .npm_resolver - .resolve_pkg_nv_ref_from_pkg_req_ref(&package_ref)?; - let node_resolution = - self.resolve_binary_entrypoint(&package_ref, &permissions)?; + .resolve_pkg_folder_from_deno_module_req( + package_ref.req(), + &referrer, + )?; + let node_resolution = self.resolve_binary_entrypoint( + &package_folder, + package_ref.sub_path(), + &permissions, + )?; let is_main_cjs = matches!(node_resolution, NodeResolution::CommonJs(_)); if let Some(lockfile) = &shared.maybe_lockfile { @@ -516,23 +528,23 @@ impl CliMainWorkerFactory { fn resolve_binary_entrypoint( &self, - package_ref: &NpmPackageNvReference, + package_folder: &Path, + sub_path: Option<&str>, permissions: &PermissionsContainer, ) -> Result { - let package_folder = self - .shared - .npm_resolver - .resolve_pkg_folder_from_deno_module(package_ref.nv())?; match self .shared .node_resolver - .resolve_binary_export(&package_folder, package_ref.sub_path()) + .resolve_binary_export(package_folder, sub_path) { Ok(node_resolution) => Ok(node_resolution), Err(original_err) => { // if the binary entrypoint was not found, fallback to regular node resolution - let result = - self.resolve_binary_entrypoint_fallback(package_ref, permissions); + let result = self.resolve_binary_entrypoint_fallback( + package_folder, + sub_path, + permissions, + ); match result { Ok(Some(resolution)) => Ok(resolution), Ok(None) => Err(original_err), @@ -547,24 +559,21 @@ impl CliMainWorkerFactory { /// resolve the binary entrypoint using regular node resolution fn resolve_binary_entrypoint_fallback( &self, - package_ref: &NpmPackageNvReference, + package_folder: &Path, + sub_path: Option<&str>, permissions: &PermissionsContainer, ) -> Result, AnyError> { // only fallback if the user specified a sub path - if package_ref.sub_path().is_none() { + if sub_path.is_none() { // it's confusing to users if the package doesn't have any binary // entrypoint and we just execute the main script which will likely // have blank output, so do not resolve the entrypoint in this case return Ok(None); } - let package_folder = self - .shared - .npm_resolver - .resolve_pkg_folder_from_deno_module(package_ref.nv())?; let Some(resolution) = self.shared.node_resolver.resolve_npm_reference( - &package_folder, - package_ref.sub_path(), + package_folder, + sub_path, NodeResolutionMode::Execution, permissions, )? diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs index 0f7cc5bb1f..b24bfdef3e 100644 --- a/ext/node/package_json.rs +++ b/ext/node/package_json.rs @@ -97,7 +97,13 @@ impl PackageJson { return Ok(PackageJson::empty(path)); } - Self::load_from_string(path, source) + let package_json = Self::load_from_string(path, source)?; + CACHE.with(|cache| { + cache + .borrow_mut() + .insert(package_json.path.clone(), package_json.clone()); + }); + Ok(package_json) } pub fn load_from_string( @@ -199,11 +205,6 @@ impl PackageJson { scripts, }; - CACHE.with(|cache| { - cache - .borrow_mut() - .insert(package_json.path.clone(), package_json.clone()); - }); Ok(package_json) }