From 48da3c17ea905f50b82948e6f94795e1589f852e Mon Sep 17 00:00:00 2001 From: Nathan Whitaker <17734409+nathanwhit@users.noreply.github.com> Date: Wed, 21 Aug 2024 15:23:32 -0700 Subject: [PATCH] fix(add): Handle packages without root exports (#25102) Fixes #24607. This PR makes the logic that caches top level dependencies (things present in import map) smarter, so we handle JSR dependencies without root exports. --- cli/module_loader.rs | 49 -------- cli/tools/installer.rs | 2 +- cli/tools/registry/mod.rs | 2 + cli/tools/registry/pm.rs | 25 ++-- cli/tools/registry/pm/cache_deps.rs | 115 ++++++++++++++++++ tests/registry/jsr/@std/testing/1.0.0/bdd.ts | 2 + .../registry/jsr/@std/testing/1.0.0/types.ts | 1 + .../registry/jsr/@std/testing/1.0.0_meta.json | 6 + tests/registry/jsr/@std/testing/meta.json | 8 ++ tests/specs/add/no_root_export/__test__.jsonc | 9 ++ tests/specs/add/no_root_export/add.out | 5 + tests/specs/add/no_root_export/deno.json | 0 tests/specs/add/no_root_export/main.ts | 3 + .../future_install_local_deno/deno.json | 4 +- .../future_install_local_deno/deno.lock.out | 12 +- .../future_install_local_deno/install.out | 4 + tests/specs/remove/basic/add.out | 8 +- 17 files changed, 190 insertions(+), 65 deletions(-) create mode 100644 cli/tools/registry/pm/cache_deps.rs create mode 100644 tests/registry/jsr/@std/testing/1.0.0/bdd.ts create mode 100644 tests/registry/jsr/@std/testing/1.0.0/types.ts create mode 100644 tests/registry/jsr/@std/testing/1.0.0_meta.json create mode 100644 tests/registry/jsr/@std/testing/meta.json create mode 100644 tests/specs/add/no_root_export/__test__.jsonc create mode 100644 tests/specs/add/no_root_export/add.out create mode 100644 tests/specs/add/no_root_export/deno.json create mode 100644 tests/specs/add/no_root_export/main.ts diff --git a/cli/module_loader.rs b/cli/module_loader.rs index 9f52089363..3208d13657 100644 --- a/cli/module_loader.rs +++ b/cli/module_loader.rs @@ -18,7 +18,6 @@ use crate::cache::CodeCache; use crate::cache::FastInsecureHasher; use crate::cache::ParsedSourceCache; use crate::emit::Emitter; -use crate::factory::CliFactory; use crate::graph_container::MainModuleGraphContainer; use crate::graph_container::ModuleGraphContainer; use crate::graph_container::ModuleGraphUpdatePermit; @@ -70,54 +69,6 @@ use deno_runtime::deno_permissions::PermissionsContainer; use deno_semver::npm::NpmPackageReqReference; use node_resolver::NodeResolutionMode; -pub async fn load_top_level_deps(factory: &CliFactory) -> Result<(), AnyError> { - let npm_resolver = factory.npm_resolver().await?; - let cli_options = factory.cli_options()?; - if let Some(npm_resolver) = npm_resolver.as_managed() { - if !npm_resolver.ensure_top_level_package_json_install().await? { - if let Some(lockfile) = cli_options.maybe_lockfile() { - lockfile.error_if_changed()?; - } - - npm_resolver.cache_packages().await?; - } - } - // cache as many entries in the import map as we can - let resolver = factory.workspace_resolver().await?; - if let Some(import_map) = resolver.maybe_import_map() { - let roots = import_map - .imports() - .entries() - .filter_map(|entry| { - if entry.key.ends_with('/') { - None - } else { - entry.value.cloned() - } - }) - .collect::>(); - let mut graph_permit = factory - .main_module_graph_container() - .await? - .acquire_update_permit() - .await; - let graph = graph_permit.graph_mut(); - factory - .module_load_preparer() - .await? - .prepare_module_load( - graph, - &roots, - false, - factory.cli_options()?.ts_type_lib_window(), - deno_runtime::deno_permissions::PermissionsContainer::allow_all(), - ) - .await?; - } - - Ok(()) -} - pub struct ModuleLoadPreparer { options: Arc, lockfile: Option>, diff --git a/cli/tools/installer.rs b/cli/tools/installer.rs index 456e5c1a68..1809e1f16a 100644 --- a/cli/tools/installer.rs +++ b/cli/tools/installer.rs @@ -275,7 +275,7 @@ async fn install_local( } let factory = CliFactory::from_flags(flags); - crate::module_loader::load_top_level_deps(&factory).await?; + crate::tools::registry::cache_top_level_deps(&factory, None).await?; if let Some(lockfile) = factory.cli_options()?.maybe_lockfile() { lockfile.write_if_changed()?; diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs index d6e06fb58a..ee3204dc74 100644 --- a/cli/tools/registry/mod.rs +++ b/cli/tools/registry/mod.rs @@ -52,6 +52,7 @@ use crate::util::display::human_size; mod api; mod auth; + mod diagnostics; mod graph; mod paths; @@ -64,6 +65,7 @@ mod unfurl; use auth::get_auth_method; use auth::AuthMethod; pub use pm::add; +pub use pm::cache_top_level_deps; pub use pm::remove; pub use pm::AddCommandName; use publish_order::PublishOrderGraph; diff --git a/cli/tools/registry/pm.rs b/cli/tools/registry/pm.rs index 3cdef071f9..87a5ea69a9 100644 --- a/cli/tools/registry/pm.rs +++ b/cli/tools/registry/pm.rs @@ -1,5 +1,9 @@ // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. +mod cache_deps; + +pub use cache_deps::cache_top_level_deps; + use std::borrow::Cow; use std::path::Path; use std::path::PathBuf; @@ -236,13 +240,16 @@ pub async fn add( let package_futures = package_reqs .into_iter() - .map(move |package_req| { - find_package_and_select_version_for_req( - jsr_resolver.clone(), - npm_resolver.clone(), - package_req, - ) - .boxed_local() + .map({ + let jsr_resolver = jsr_resolver.clone(); + move |package_req| { + find_package_and_select_version_for_req( + jsr_resolver.clone(), + npm_resolver.clone(), + package_req, + ) + .boxed_local() + } }) .collect::>(); @@ -350,7 +357,7 @@ pub async fn add( // make a new CliFactory to pick up the updated config file let cli_factory = CliFactory::from_flags(flags); // cache deps - crate::module_loader::load_top_level_deps(&cli_factory).await?; + cache_deps::cache_top_level_deps(&cli_factory, Some(jsr_resolver)).await?; Ok(()) } @@ -597,7 +604,7 @@ pub async fn remove( // Update deno.lock node_resolver::PackageJsonThreadLocalCache::clear(); let cli_factory = CliFactory::from_flags(flags); - crate::module_loader::load_top_level_deps(&cli_factory).await?; + cache_deps::cache_top_level_deps(&cli_factory, None).await?; } Ok(()) diff --git a/cli/tools/registry/pm/cache_deps.rs b/cli/tools/registry/pm/cache_deps.rs new file mode 100644 index 0000000000..d292c32f58 --- /dev/null +++ b/cli/tools/registry/pm/cache_deps.rs @@ -0,0 +1,115 @@ +// Copyright 2018-2024 the Deno authors. All rights reserved. MIT license. + +use std::sync::Arc; + +use crate::factory::CliFactory; +use crate::graph_container::ModuleGraphContainer; +use crate::graph_container::ModuleGraphUpdatePermit; +use deno_core::error::AnyError; +use deno_core::futures::stream::FuturesUnordered; +use deno_core::futures::StreamExt; +use deno_semver::package::PackageReq; + +pub async fn cache_top_level_deps( + factory: &CliFactory, + jsr_resolver: Option>, +) -> Result<(), AnyError> { + let npm_resolver = factory.npm_resolver().await?; + let cli_options = factory.cli_options()?; + if let Some(npm_resolver) = npm_resolver.as_managed() { + if !npm_resolver.ensure_top_level_package_json_install().await? { + if let Some(lockfile) = cli_options.maybe_lockfile() { + lockfile.error_if_changed()?; + } + + npm_resolver.cache_packages().await?; + } + } + // cache as many entries in the import map as we can + let resolver = factory.workspace_resolver().await?; + if let Some(import_map) = resolver.maybe_import_map() { + let jsr_resolver = if let Some(resolver) = jsr_resolver { + resolver + } else { + Arc::new(crate::jsr::JsrFetchResolver::new( + factory.file_fetcher()?.clone(), + )) + }; + + let mut roots = Vec::new(); + + let mut info_futures = FuturesUnordered::new(); + + let mut seen_reqs = std::collections::HashSet::new(); + + for entry in import_map.imports().entries() { + let Some(specifier) = entry.value else { + continue; + }; + + match specifier.scheme() { + "jsr" => { + let specifier_str = specifier.as_str(); + let specifier_str = + specifier_str.strip_prefix("jsr:").unwrap_or(specifier_str); + if let Ok(req) = PackageReq::from_str(specifier_str) { + if !seen_reqs.insert(req.clone()) { + continue; + } + let jsr_resolver = jsr_resolver.clone(); + info_futures.push(async move { + if let Some(nv) = jsr_resolver.req_to_nv(&req).await { + if let Some(info) = jsr_resolver.package_version_info(&nv).await + { + return Some((specifier.clone(), info)); + } + } + None + }); + } + } + "npm" => roots.push(specifier.clone()), + _ => { + if entry.key.ends_with('/') && specifier.as_str().ends_with('/') { + continue; + } + roots.push(specifier.clone()); + } + } + } + + while let Some(info_future) = info_futures.next().await { + if let Some((specifier, info)) = info_future { + if info.export(".").is_some() { + roots.push(specifier.clone()); + continue; + } + let exports = info.exports(); + for (k, _) in exports { + if let Ok(spec) = specifier.join(k) { + roots.push(spec); + } + } + } + } + let mut graph_permit = factory + .main_module_graph_container() + .await? + .acquire_update_permit() + .await; + let graph = graph_permit.graph_mut(); + factory + .module_load_preparer() + .await? + .prepare_module_load( + graph, + &roots, + false, + deno_config::deno_json::TsTypeLib::DenoWorker, + deno_runtime::deno_permissions::PermissionsContainer::allow_all(), + ) + .await?; + } + + Ok(()) +} diff --git a/tests/registry/jsr/@std/testing/1.0.0/bdd.ts b/tests/registry/jsr/@std/testing/1.0.0/bdd.ts new file mode 100644 index 0000000000..a665d06034 --- /dev/null +++ b/tests/registry/jsr/@std/testing/1.0.0/bdd.ts @@ -0,0 +1,2 @@ +export function it(_name: string, _fn: () => void) { +} \ No newline at end of file diff --git a/tests/registry/jsr/@std/testing/1.0.0/types.ts b/tests/registry/jsr/@std/testing/1.0.0/types.ts new file mode 100644 index 0000000000..feebd26032 --- /dev/null +++ b/tests/registry/jsr/@std/testing/1.0.0/types.ts @@ -0,0 +1 @@ +export type AssertType = A extends B ? true : never; \ No newline at end of file diff --git a/tests/registry/jsr/@std/testing/1.0.0_meta.json b/tests/registry/jsr/@std/testing/1.0.0_meta.json new file mode 100644 index 0000000000..5c1bdd8072 --- /dev/null +++ b/tests/registry/jsr/@std/testing/1.0.0_meta.json @@ -0,0 +1,6 @@ +{ + "exports": { + "./bdd": "./bdd.ts", + "./types": "./types.ts" + } +} \ No newline at end of file diff --git a/tests/registry/jsr/@std/testing/meta.json b/tests/registry/jsr/@std/testing/meta.json new file mode 100644 index 0000000000..d8037eabb8 --- /dev/null +++ b/tests/registry/jsr/@std/testing/meta.json @@ -0,0 +1,8 @@ +{ + "scope": "std", + "name": "path", + "latest": "1.0.0", + "versions": { + "1.0.0": {} + } +} diff --git a/tests/specs/add/no_root_export/__test__.jsonc b/tests/specs/add/no_root_export/__test__.jsonc new file mode 100644 index 0000000000..2adfbd8de2 --- /dev/null +++ b/tests/specs/add/no_root_export/__test__.jsonc @@ -0,0 +1,9 @@ +{ + "tempDir": true, + "steps": [ + { + "args": "add @std/testing", + "output": "add.out" + } + ] +} diff --git a/tests/specs/add/no_root_export/add.out b/tests/specs/add/no_root_export/add.out new file mode 100644 index 0000000000..4bd9da7be2 --- /dev/null +++ b/tests/specs/add/no_root_export/add.out @@ -0,0 +1,5 @@ +Add jsr:@std/testing@1.0.0 +[UNORDERED_START] +Download http://127.0.0.1:4250/@std/testing/1.0.0/bdd.ts +Download http://127.0.0.1:4250/@std/testing/1.0.0/types.ts +[UNORDERED_END] diff --git a/tests/specs/add/no_root_export/deno.json b/tests/specs/add/no_root_export/deno.json new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/specs/add/no_root_export/main.ts b/tests/specs/add/no_root_export/main.ts new file mode 100644 index 0000000000..0c0d4107fe --- /dev/null +++ b/tests/specs/add/no_root_export/main.ts @@ -0,0 +1,3 @@ +import { it } from "@std/testing/bdd"; + +const _it = it; diff --git a/tests/specs/install/future_install_local_deno/deno.json b/tests/specs/install/future_install_local_deno/deno.json index dbcf1c2203..9213ce8347 100644 --- a/tests/specs/install/future_install_local_deno/deno.json +++ b/tests/specs/install/future_install_local_deno/deno.json @@ -3,6 +3,8 @@ "@std/fs/": "https://deno.land/std@0.224.0/fs/", "@denotest/esm-basic": "npm:@denotest/esm-basic@^1.0.0", "@denotest/add": "jsr:@denotest/add", - "test-http": "http://localhost:4545/v1/extensionless" + "test-http": "http://localhost:4545/v1/extensionless", + "@std/testing": "jsr:@std/testing", + "@std/testing/": "jsr:/@std/testing/" } } diff --git a/tests/specs/install/future_install_local_deno/deno.lock.out b/tests/specs/install/future_install_local_deno/deno.lock.out index 188de5de90..04bfc9b3a7 100644 --- a/tests/specs/install/future_install_local_deno/deno.lock.out +++ b/tests/specs/install/future_install_local_deno/deno.lock.out @@ -2,11 +2,15 @@ "version": "4", "specifiers": { "jsr:@denotest/add": "jsr:@denotest/add@1.0.0", + "jsr:@std/testing": "jsr:@std/testing@1.0.0", "npm:@denotest/esm-basic@^1.0.0": "npm:@denotest/esm-basic@1.0.0" }, "jsr": { "@denotest/add@1.0.0": { "integrity": "[WILDCARD]" + }, + "@std/testing@1.0.0": { + "integrity": "[WILDCARD]" } }, "npm": { @@ -14,10 +18,16 @@ "integrity": "[WILDCARD]" } }, - "remote": [WILDCARD], + "remote": { + "http://localhost:4545/subdir/mod1.ts": "[WILDCARD]", + "http://localhost:4545/subdir/print_hello.ts": "[WILDCARD]", + "http://localhost:4545/subdir/subdir2/mod2.ts": "[WILDCARD]", + "http://localhost:4545/v1/extensionless": "[WILDCARD]" + }, "workspace": { "dependencies": [ "jsr:@denotest/add", + "jsr:@std/testing", "npm:@denotest/esm-basic@^1.0.0" ] } diff --git a/tests/specs/install/future_install_local_deno/install.out b/tests/specs/install/future_install_local_deno/install.out index eecba12991..15263a37b5 100644 --- a/tests/specs/install/future_install_local_deno/install.out +++ b/tests/specs/install/future_install_local_deno/install.out @@ -1,4 +1,6 @@ [UNORDERED_START] +Download http://127.0.0.1:4250/@std/testing/meta.json +Download http://127.0.0.1:4250/@std/testing/1.0.0_meta.json Download http://localhost:4545/v1/extensionless Download http://localhost:4545/subdir/mod1.ts Download http://localhost:4545/subdir/subdir2/mod2.ts @@ -8,4 +10,6 @@ Download http://127.0.0.1:4250/@denotest/add/1.0.0_meta.json Download http://127.0.0.1:4250/@denotest/add/1.0.0/mod.ts Download http://localhost:4260/@denotest/esm-basic Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz +Download http://127.0.0.1:4250/@std/testing/1.0.0/bdd.ts +Download http://127.0.0.1:4250/@std/testing/1.0.0/types.ts [UNORDERED_END] diff --git a/tests/specs/remove/basic/add.out b/tests/specs/remove/basic/add.out index a93b0ab528..75848b7c6f 100644 --- a/tests/specs/remove/basic/add.out +++ b/tests/specs/remove/basic/add.out @@ -1,9 +1,9 @@ Created deno.json configuration file. -Add jsr:@std/assert@1.0.0 -Add jsr:@std/http@1.0.0 [UNORDERED_START] -Download http://127.0.0.1:4250/@std/http/1.0.0_meta.json -Download http://127.0.0.1:4250/@std/assert/1.0.0_meta.json +Add jsr:@std/http@1.0.0 +Add jsr:@std/assert@1.0.0 +[UNORDERED_END] +[UNORDERED_START] Download http://127.0.0.1:4250/@std/http/1.0.0/mod.ts Download http://127.0.0.1:4250/@std/assert/1.0.0/mod.ts Download http://127.0.0.1:4250/@std/assert/1.0.0/assert_equals.ts