fix(publish): unfurling should always be done with the package json (#24435)

Closes https://github.com/denoland/deno/issues/24430

.github/workflows/ci.generate.ts

@@ -5,7 +5,7 @@ import { stringify } from "jsr:@std/yaml@^0.221/stringify";
 // Bump this number when you want to purge the cache.
 // Note: the tools/release/01_bump_crate_versions.ts script will update this version
 // automatically via regex, so ensure that this line maintains this format.
-const cacheVersion = 1;
+const cacheVersion = 2;
 
 const ubuntuX86Runner = "ubuntu-22.04";
 const ubuntuX86XlRunner = "ubuntu-22.04-xl";

.github/workflows/ci.yml

@@ -367,8 +367,8 @@ jobs:
           path: |-
             ~/.cargo/registry/index
             ~/.cargo/registry/cache
-          key: '1-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
-          restore-keys: '1-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
+          key: '2-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}'
+          restore-keys: '2-cargo-home-${{ matrix.os }}-${{ matrix.arch }}'
         if: '!(matrix.skip)'
       - name: Restore cache build output (PR)
         uses: actions/cache/restore@v4
@@ -380,7 +380,7 @@ jobs:
             !./target/*/*.zip
             !./target/*/*.tar.gz
           key: never_saved
-          restore-keys: '1-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-'
+          restore-keys: '2-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-'
       - name: Apply and update mtime cache
         if: '!(matrix.skip) && (!startsWith(github.ref, ''refs/tags/''))'
         uses: ./.github/mtime_cache
@@ -669,7 +669,7 @@ jobs:
             !./target/*/gn_out
             !./target/*/*.zip
             !./target/*/*.tar.gz
-          key: '1-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
+          key: '2-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}'
   publish-canary:
     name: publish canary
     runs-on: ubuntu-22.04

Cargo.lock

@@ -1288,7 +1288,7 @@ dependencies = [
  "indexmap",
  "log",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "serde",
  "serde_json",
  "sha2",
@@ -1351,7 +1351,7 @@ dependencies = [
  "futures",
  "libc",
  "memoffset 0.9.1",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "pin-project",
  "serde",
  "serde_json",
@@ -1531,7 +1531,7 @@ dependencies = [
  "log",
  "monch",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "regex",
  "serde",
  "serde_json",
@@ -1591,7 +1591,7 @@ dependencies = [
  "log",
  "once_cell",
  "os_pipe",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "rand",
  "tokio",
  "winapi",
@@ -1947,10 +1947,11 @@ dependencies = [
 
 [[package]]
 name = "deno_unsync"
-version = "0.3.6"
+version = "0.3.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "10eb3aaf83c3431d4215741140ec3a63b0c0edb972ee898c89bdf8462e9e136b"
+checksum = "c3c8b95582c2023dbb66fccc37421b374026f5915fa507d437cb566904db9a3a"
 dependencies = [
+ "parking_lot 0.12.3",
  "tokio",
 ]
 
@@ -2716,7 +2717,7 @@ dependencies = [
  "anyhow",
  "crossbeam-channel",
  "deno_terminal",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "regex",
  "thiserror",
 ]
@@ -4593,9 +4594,9 @@ dependencies = [
 
 [[package]]
 name = "parking_lot"
-version = "0.12.1"
+version = "0.12.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3742b2c103b9f06bc9fff0a37ff4912935851bee6d36f3c02bcc755bcfec228f"
+checksum = "f1bf18183cf54e8d6059647fc3063646a1801cf30896933ec2311622cc4b9a27"
 dependencies = [
  "lock_api",
  "parking_lot_core 0.9.9",
@@ -6141,7 +6142,7 @@ checksum = "f91138e76242f575eb1d3b38b4f1362f10d3a43f47d182a5b359af488a02293b"
 dependencies = [
  "new_debug_unreachable",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "phf_shared 0.10.0",
  "precomputed-hash",
  "serde",
@@ -6237,7 +6238,7 @@ dependencies = [
  "indexmap",
  "is-macro",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "petgraph",
  "radix_fmt",
  "relative-path",
@@ -6798,7 +6799,7 @@ dependencies = [
  "nix 0.26.2",
  "once_cell",
  "os_pipe",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "pretty_assertions",
  "prost",
  "prost-build",
@@ -6921,7 +6922,7 @@ dependencies = [
  "libc",
  "mio",
  "num_cpus",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "pin-project-lite",
  "signal-hook-registry",
  "socket2",
@@ -7205,7 +7206,7 @@ dependencies = [
  "ipconfig",
  "lru-cache",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "rand",
  "resolv-conf",
  "serde",
@@ -7679,7 +7680,7 @@ dependencies = [
  "log",
  "naga",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "profiling",
  "raw-window-handle",
  "ron",
@@ -7721,7 +7722,7 @@ dependencies = [
  "ndk-sys",
  "objc",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "profiling",
  "range-alloc",
  "raw-window-handle",
@@ -8148,7 +8149,7 @@ dependencies = [
  "log",
  "num-traits",
  "once_cell",
- "parking_lot 0.12.1",
+ "parking_lot 0.12.3",
  "rand",
  "regex",
  "thiserror",

cli/args/mod.rs

@@ -1060,6 +1060,7 @@ impl CliOptions {
   pub async fn create_workspace_resolver(
     &self,
     file_fetcher: &FileFetcher,
+    pkg_json_dep_resolution: PackageJsonDepResolution,
   ) -> Result<WorkspaceResolver, AnyError> {
     let overrode_no_import_map = self
       .overrides
@@ -1102,12 +1103,7 @@ impl CliOptions {
         .workspace
         .create_resolver(
           CreateResolverOptions {
-            // todo(dsherret): this should be false for nodeModulesDir: true
-            pkg_json_dep_resolution: if self.use_byonm() {
-              PackageJsonDepResolution::Disabled
-            } else {
-              PackageJsonDepResolution::Enabled
-            },
+            pkg_json_dep_resolution,
             specified_import_map: cli_arg_specified_import_map,
           },
           |specifier| {

cli/factory.rs

@@ -55,6 +55,7 @@ use std::collections::BTreeSet;
 use std::path::PathBuf;
 
 use deno_config::package_json::PackageJsonDepValue;
+use deno_config::workspace::PackageJsonDepResolution;
 use deno_config::workspace::WorkspaceResolver;
 use deno_config::ConfigFile;
 use deno_core::error::AnyError;
@@ -458,7 +459,15 @@ impl CliFactory {
       .get_or_try_init_async(async {
         let resolver = self
           .options
-          .create_workspace_resolver(self.file_fetcher()?)
+          .create_workspace_resolver(
+            self.file_fetcher()?,
+            if self.options.use_byonm() {
+              PackageJsonDepResolution::Disabled
+            } else {
+              // todo(dsherret): this should be false for nodeModulesDir: true
+              PackageJsonDepResolution::Enabled
+            },
+          )
           .await?;
         if !resolver.diagnostics().is_empty() {
           warn!(
@@ -759,6 +768,7 @@ impl CliFactory {
       self.file_fetcher()?,
       self.http_client_provider(),
       self.npm_resolver().await?.as_ref(),
+      self.workspace_resolver().await?.as_ref(),
       self.options.npm_system_info(),
     ))
   }

cli/standalone/binary.rs

@@ -18,6 +18,7 @@ use std::process::Command;
 use deno_ast::ModuleSpecifier;
 use deno_config::workspace::PackageJsonDepResolution;
 use deno_config::workspace::Workspace;
+use deno_config::workspace::WorkspaceResolver;
 use deno_core::anyhow::bail;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
@@ -376,6 +377,7 @@ pub struct DenoCompileBinaryWriter<'a> {
   file_fetcher: &'a FileFetcher,
   http_client_provider: &'a HttpClientProvider,
   npm_resolver: &'a dyn CliNpmResolver,
+  workspace_resolver: &'a WorkspaceResolver,
   npm_system_info: NpmSystemInfo,
 }
 
@@ -386,6 +388,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
     file_fetcher: &'a FileFetcher,
     http_client_provider: &'a HttpClientProvider,
     npm_resolver: &'a dyn CliNpmResolver,
+    workspace_resolver: &'a WorkspaceResolver,
     npm_system_info: NpmSystemInfo,
   ) -> Self {
     Self {
@@ -393,6 +396,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
       file_fetcher,
       http_client_provider,
       npm_resolver,
+      workspace_resolver,
       npm_system_info,
     }
   }
@@ -419,17 +423,15 @@ impl<'a> DenoCompileBinaryWriter<'a> {
       }
       set_windows_binary_to_gui(&mut original_binary)?;
     }
-    self
-      .write_standalone_binary(
-        writer,
-        original_binary,
-        eszip,
-        root_dir_url,
-        entrypoint,
-        cli_options,
-        compile_flags,
-      )
-      .await
+    self.write_standalone_binary(
+      writer,
+      original_binary,
+      eszip,
+      root_dir_url,
+      entrypoint,
+      cli_options,
+      compile_flags,
+    )
   }
 
   async fn get_base_binary(
@@ -512,7 +514,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
   /// This functions creates a standalone deno binary by appending a bundle
   /// and magic trailer to the currently executing binary.
   #[allow(clippy::too_many_arguments)]
-  async fn write_standalone_binary(
+  fn write_standalone_binary(
     &self,
     writer: &mut impl Write,
     original_bin: Vec<u8>,
@@ -530,9 +532,6 @@ impl<'a> DenoCompileBinaryWriter<'a> {
       Some(CaData::Bytes(bytes)) => Some(bytes.clone()),
       None => None,
     };
-    let workspace_resolver = cli_options
-      .create_workspace_resolver(self.file_fetcher)
-      .await?;
     let root_path = root_dir_url.inner().to_file_path().unwrap();
     let (npm_vfs, npm_files, node_modules) = match self.npm_resolver.as_inner()
     {
@@ -599,7 +598,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
       ca_data,
       entrypoint_key: root_dir_url.specifier_key(entrypoint).into_owned(),
       workspace_resolver: SerializedWorkspaceResolver {
-        import_map: workspace_resolver.maybe_import_map().map(|i| {
+        import_map: self.workspace_resolver.maybe_import_map().map(|i| {
           SerializedWorkspaceResolverImportMap {
             specifier: if i.base_url().scheme() == "file" {
               root_dir_url.specifier_key(i.base_url()).into_owned()
@@ -610,7 +609,8 @@ impl<'a> DenoCompileBinaryWriter<'a> {
             json: i.to_json(),
           }
         }),
-        package_jsons: workspace_resolver
+        package_jsons: self
+          .workspace_resolver
           .package_jsons()
           .map(|pkg_json| {
             (
@@ -621,7 +621,7 @@ impl<'a> DenoCompileBinaryWriter<'a> {
             )
           })
           .collect(),
-        pkg_json_resolution: workspace_resolver.pkg_json_dep_resolution(),
+        pkg_json_resolution: self.workspace_resolver.pkg_json_dep_resolution(),
       },
       node_modules,
       disable_deprecated_api_warning: cli_options

cli/tools/registry/mod.rs

@@ -12,7 +12,7 @@ use base64::prelude::BASE64_STANDARD;
 use base64::Engine;
 use deno_ast::ModuleSpecifier;
 use deno_config::workspace::JsrPackageConfig;
-use deno_config::workspace::WorkspaceResolver;
+use deno_config::workspace::PackageJsonDepResolution;
 use deno_core::anyhow::bail;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
@@ -24,7 +24,6 @@ use deno_core::serde_json;
 use deno_core::serde_json::json;
 use deno_core::serde_json::Value;
 use deno_runtime::deno_fetch::reqwest;
-use deno_runtime::deno_fs::FileSystem;
 use deno_terminal::colors;
 use lsp_types::Url;
 use serde::Deserialize;
@@ -81,8 +80,6 @@ pub async fn publish(
   let auth_method =
     get_auth_method(publish_flags.token, publish_flags.dry_run)?;
 
-  let workspace_resolver = cli_factory.workspace_resolver().await?.clone();
-
   let directory_path = cli_factory.cli_options().initial_cwd();
   let cli_options = cli_factory.cli_options();
   let publish_configs = cli_options.workspace.jsr_packages_for_publish();
@@ -103,6 +100,20 @@ pub async fn publish(
       }
     }
   }
+  let specifier_unfurler = Arc::new(SpecifierUnfurler::new(
+    if cli_options.unstable_sloppy_imports() {
+      Some(SloppyImportsResolver::new(cli_factory.fs().clone()))
+    } else {
+      None
+    },
+    cli_options
+      .create_workspace_resolver(
+        cli_factory.file_fetcher()?,
+        PackageJsonDepResolution::Enabled,
+      )
+      .await?,
+    cli_options.unstable_bare_node_builtins(),
+  ));
 
   let diagnostics_collector = PublishDiagnosticsCollector::default();
   let publish_preparer = PublishPreparer::new(
@@ -110,9 +121,8 @@ pub async fn publish(
     cli_factory.module_graph_creator().await?.clone(),
     cli_factory.parsed_source_cache().clone(),
     cli_factory.type_checker().await?.clone(),
-    cli_factory.fs().clone(),
     cli_factory.cli_options().clone(),
-    workspace_resolver,
+    specifier_unfurler,
   );
 
   let prepared_data = publish_preparer
@@ -191,8 +201,7 @@ struct PublishPreparer {
   source_cache: Arc<ParsedSourceCache>,
   type_checker: Arc<TypeChecker>,
   cli_options: Arc<CliOptions>,
-  sloppy_imports_resolver: Option<Arc<SloppyImportsResolver>>,
-  workspace_resolver: Arc<WorkspaceResolver>,
+  specifier_unfurler: Arc<SpecifierUnfurler>,
 }
 
 impl PublishPreparer {
@@ -201,23 +210,16 @@ impl PublishPreparer {
     module_graph_creator: Arc<ModuleGraphCreator>,
     source_cache: Arc<ParsedSourceCache>,
     type_checker: Arc<TypeChecker>,
-    fs: Arc<dyn FileSystem>,
     cli_options: Arc<CliOptions>,
-    workspace_resolver: Arc<WorkspaceResolver>,
+    specifier_unfurler: Arc<SpecifierUnfurler>,
   ) -> Self {
-    let sloppy_imports_resolver = if cli_options.unstable_sloppy_imports() {
-      Some(Arc::new(SloppyImportsResolver::new(fs.clone())))
-    } else {
-      None
-    };
     Self {
       graph_diagnostics_collector,
       module_graph_creator,
       source_cache,
       type_checker,
       cli_options,
-      sloppy_imports_resolver,
-      workspace_resolver,
+      specifier_unfurler,
     }
   }
 
@@ -432,18 +434,11 @@ impl PublishPreparer {
 
     let tarball = deno_core::unsync::spawn_blocking({
       let diagnostics_collector = diagnostics_collector.clone();
-      let workspace_resolver = self.workspace_resolver.clone();
-      let sloppy_imports_resolver = self.sloppy_imports_resolver.clone();
+      let unfurler = self.specifier_unfurler.clone();
       let cli_options = self.cli_options.clone();
       let source_cache = self.source_cache.clone();
       let config_path = config_path.clone();
       move || {
-        let bare_node_builtins = cli_options.unstable_bare_node_builtins();
-        let unfurler = SpecifierUnfurler::new(
-          sloppy_imports_resolver.as_deref(),
-          &workspace_resolver,
-          bare_node_builtins,
-        );
         let root_specifier =
           ModuleSpecifier::from_directory_path(&root_dir).unwrap();
         let publish_paths =

cli/tools/registry/unfurl.rs

@@ -5,6 +5,7 @@ use deno_ast::SourceRange;
 use deno_ast::SourceTextInfo;
 use deno_config::package_json::PackageJsonDepValue;
 use deno_config::workspace::MappedResolution;
+use deno_config::workspace::PackageJsonDepResolution;
 use deno_config::workspace::WorkspaceResolver;
 use deno_core::ModuleSpecifier;
 use deno_graph::DependencyDescriptor;
@@ -40,18 +41,22 @@ impl SpecifierUnfurlerDiagnostic {
   }
 }
 
-pub struct SpecifierUnfurler<'a> {
-  sloppy_imports_resolver: Option<&'a SloppyImportsResolver>,
-  workspace_resolver: &'a WorkspaceResolver,
+pub struct SpecifierUnfurler {
+  sloppy_imports_resolver: Option<SloppyImportsResolver>,
+  workspace_resolver: WorkspaceResolver,
   bare_node_builtins: bool,
 }
 
-impl<'a> SpecifierUnfurler<'a> {
+impl SpecifierUnfurler {
   pub fn new(
-    sloppy_imports_resolver: Option<&'a SloppyImportsResolver>,
-    workspace_resolver: &'a WorkspaceResolver,
+    sloppy_imports_resolver: Option<SloppyImportsResolver>,
+    workspace_resolver: WorkspaceResolver,
     bare_node_builtins: bool,
   ) -> Self {
+    debug_assert_eq!(
+      workspace_resolver.pkg_json_dep_resolution(),
+      PackageJsonDepResolution::Enabled
+    );
     Self {
       sloppy_imports_resolver,
       workspace_resolver,
@@ -136,7 +141,7 @@ impl<'a> SpecifierUnfurler<'a> {
     //   resolved
     // };
     let resolved =
-      if let Some(sloppy_imports_resolver) = self.sloppy_imports_resolver {
+      if let Some(sloppy_imports_resolver) = &self.sloppy_imports_resolver {
         sloppy_imports_resolver
           .resolve(&resolved, deno_graph::source::ResolutionMode::Execution)
           .as_specifier()
@@ -148,6 +153,12 @@ impl<'a> SpecifierUnfurler<'a> {
     if relative_resolved == specifier {
       None // nothing to unfurl
     } else {
+      log::debug!(
+        "Unfurled specifier: {} from {} -> {}",
+        specifier,
+        referrer,
+        relative_resolved
+      );
       Some(relative_resolved)
     }
   }
@@ -395,11 +406,9 @@ mod tests {
       deno_config::workspace::PackageJsonDepResolution::Enabled,
     );
     let fs = Arc::new(RealFs);
-    let sloppy_imports_resolver = SloppyImportsResolver::new(fs);
-
     let unfurler = SpecifierUnfurler::new(
-      Some(&sloppy_imports_resolver),
-      &workspace_resolver,
+      Some(SloppyImportsResolver::new(fs)),
+      workspace_resolver,
       true,
     );
 

cli/util/logger.rs

@@ -41,6 +41,7 @@ pub fn init(maybe_level: Option<log::Level>) {
   // wgpu crates (gfx_backend), have a lot of useless INFO and WARN logs
   .filter_module("wgpu", log::LevelFilter::Error)
   .filter_module("gfx", log::LevelFilter::Error)
+  .filter_module("globset", log::LevelFilter::Error)
   // used to make available the lsp_debug which is then filtered out at runtime
   // in the cli logger
   .filter_module("deno::lsp::performance", log::LevelFilter::Debug)

tests/specs/publish/byonm_with_package_json/__test__.jsonc

@@ -0,0 +1,13 @@
+{
+  "envs": {
+    "DENO_FUTURE": "1"
+  },
+  "tempDir": true,
+  "steps": [{
+    "args": "install",
+    "output": "[WILDCARD]"
+  }, {
+    "args": "publish --log-level=debug --dry-run --allow-dirty",
+    "output": "publish.out"
+  }]
+}

tests/specs/publish/byonm_with_package_json/jsr.json

@@ -0,0 +1,12 @@
+{
+  "name": "@scope/package",
+  "version": "0.0.0",
+  "exports": {
+    ".": "./src/index.ts"
+  },
+  "publish": {
+    "include": [
+      "src/**/*"
+    ]
+  }
+}

tests/specs/publish/byonm_with_package_json/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "@scope/pkg",
+  "module": "src/index.ts",
+  "type": "module",
+  "dependencies": {
+    "@denotest/add": "1"
+  }
+}

tests/specs/publish/byonm_with_package_json/publish.out

@@ -0,0 +1,3 @@
+[WILDCARD]Unfurled specifier: @denotest/add from [WILDLINE]/src/index.ts -> npm:@denotest/add@1
+[WILDCARD]
+Warning Aborting due to --dry-run

tests/specs/publish/byonm_with_package_json/src/index.ts

@@ -0,0 +1,3 @@
+import { add } from "@denotest/add";
+
+console.log(add(1, 2));