fix(publish): --dry-publish should error for gitignored excluded files (#23540)

Files that were gitignored only were not included in the diagnostic.

cli/tools/registry/diagnostics.rs

@@ -296,7 +296,7 @@ impl Diagnostic for PublishDiagnostic {
       InvalidExternalImport { .. } => Some(Cow::Borrowed("replace this import with one from jsr or npm, or vendor the dependency into your package")),
       UnsupportedJsxTsx { .. } => None,
       ExcludedModule { .. } => Some(
-        Cow::Borrowed("remove the module from 'exclude' and/or 'publish.exclude' in the config file"),
+        Cow::Borrowed("remove the module from 'exclude' and/or 'publish.exclude' in the config file or use 'publish.exclude' with a negative glob to unexclude from gitignore"),
       ),
     }
   }

cli/tools/registry/mod.rs

@@ -1,6 +1,7 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
 use std::collections::HashMap;
+use std::collections::HashSet;
 use std::io::IsTerminal;
 use std::path::Path;
 use std::process::Stdio;
@@ -68,6 +69,7 @@ use unfurl::SpecifierUnfurler;
 
 use super::check::TypeChecker;
 
+use self::paths::CollectedPublishPath;
 use self::tar::PublishableTarball;
 
 fn ring_bell() {
@@ -105,7 +107,7 @@ async fn prepare_publish(
   diagnostics_collector: &PublishDiagnosticsCollector,
 ) -> Result<Rc<PreparedPublishPackage>, AnyError> {
   let config_path = deno_json.specifier.to_file_path().unwrap();
-  let dir_path = config_path.parent().unwrap().to_path_buf();
+  let root_dir = config_path.parent().unwrap().to_path_buf();
   let Some(version) = deno_json.json.version.clone() else {
     bail!("{} is missing 'version' field", deno_json.specifier);
   };
@@ -113,7 +115,7 @@ async fn prepare_publish(
     let mut suggested_entrypoint = None;
 
     for entrypoint in SUGGESTED_ENTRYPOINTS {
-      if dir_path.join(entrypoint).exists() {
+      if root_dir.join(entrypoint).exists() {
         suggested_entrypoint = Some(entrypoint);
         break;
       }
@@ -143,7 +145,10 @@ async fn prepare_publish(
   let Some((scope, name_no_scope)) = name_no_at.split_once('/') else {
     bail!("Invalid package name, use '@<scope_name>/<package_name> format");
   };
-  let file_patterns = deno_json.to_publish_config()?.map(|c| c.files);
+  let file_patterns = deno_json
+    .to_publish_config()?
+    .map(|c| c.files)
+    .unwrap_or_else(|| FilePatterns::new_with_base(root_dir.to_path_buf()));
 
   let diagnostics_collector = diagnostics_collector.clone();
   let tarball = deno_core::unsync::spawn_blocking(move || {
@@ -154,20 +159,24 @@ async fn prepare_publish(
       bare_node_builtins,
     );
     let root_specifier =
-      ModuleSpecifier::from_directory_path(&dir_path).unwrap();
+      ModuleSpecifier::from_directory_path(&root_dir).unwrap();
+    let publish_paths = paths::collect_publish_paths(
+      &root_dir,
+      &cli_options,
+      &diagnostics_collector,
+      file_patterns,
+    )?;
     collect_excluded_module_diagnostics(
       &root_specifier,
       &graph,
-      file_patterns.as_ref(),
+      &publish_paths,
       &diagnostics_collector,
     );
     tar::create_gzipped_tarball(
-      &dir_path,
-      &cli_options,
+      &publish_paths,
       LazyGraphSourceParser::new(&source_cache, &graph),
       &diagnostics_collector,
       &unfurler,
-      file_patterns,
     )
     .context("Failed to create a tarball")
   })
@@ -205,13 +214,14 @@ async fn prepare_publish(
 fn collect_excluded_module_diagnostics(
   root: &ModuleSpecifier,
   graph: &deno_graph::ModuleGraph,
-  file_patterns: Option<&FilePatterns>,
+  publish_paths: &[CollectedPublishPath],
   diagnostics_collector: &PublishDiagnosticsCollector,
 ) {
-  let Some(file_patterns) = file_patterns else {
-    return;
-  };
-  let specifiers = graph
+  let publish_specifiers = publish_paths
+    .iter()
+    .map(|path| &path.specifier)
+    .collect::<HashSet<_>>();
+  let graph_specifiers = graph
     .modules()
     .filter_map(|m| match m {
       deno_graph::Module::Js(_) | deno_graph::Module::Json(_) => {
@@ -222,8 +232,8 @@ fn collect_excluded_module_diagnostics(
       | deno_graph::Module::External(_) => None,
     })
     .filter(|s| s.as_str().starts_with(root.as_str()));
-  for specifier in specifiers {
-    if !file_patterns.matches_specifier(specifier) {
+  for specifier in graph_specifiers {
+    if !publish_specifiers.contains(specifier) {
       diagnostics_collector.push(PublishDiagnostic::ExcludedModule {
         specifier: specifier.clone(),
       });

cli/tools/registry/paths.rs

@@ -2,8 +2,22 @@
 
 // Validation logic in this file is shared with registry/api/src/ids.rs
 
+use std::collections::HashSet;
+use std::path::Path;
+use std::path::PathBuf;
+
+use deno_ast::MediaType;
+use deno_ast::ModuleSpecifier;
+use deno_config::glob::FilePatterns;
+use deno_core::error::AnyError;
 use thiserror::Error;
 
+use crate::args::CliOptions;
+use crate::util::fs::FileCollector;
+
+use super::diagnostics::PublishDiagnostic;
+use super::diagnostics::PublishDiagnosticsCollector;
+
 /// A package path, like '/foo' or '/foo/bar'. The path is prefixed with a slash
 /// and does not end with a slash.
 ///
@@ -196,3 +210,124 @@ pub enum PackagePathValidationError {
   #[error("package path must not contain invalid characters (found '{}')", .0.escape_debug())]
   InvalidOtherChar(char),
 }
+
+pub struct CollectedPublishPath {
+  pub specifier: ModuleSpecifier,
+  pub path: PathBuf,
+  pub relative_path: String,
+}
+
+pub fn collect_publish_paths(
+  root_dir: &Path,
+  cli_options: &CliOptions,
+  diagnostics_collector: &PublishDiagnosticsCollector,
+  file_patterns: FilePatterns,
+) -> Result<Vec<CollectedPublishPath>, AnyError> {
+  let publish_paths =
+    collect_paths(cli_options, diagnostics_collector, file_patterns)?;
+  let mut paths = HashSet::with_capacity(publish_paths.len());
+  let mut result = Vec::with_capacity(publish_paths.len());
+  for path in publish_paths {
+    let Ok(specifier) = ModuleSpecifier::from_file_path(&path) else {
+      diagnostics_collector
+        .to_owned()
+        .push(PublishDiagnostic::InvalidPath {
+          path: path.to_path_buf(),
+          message: "unable to convert path to url".to_string(),
+        });
+      continue;
+    };
+
+    let Ok(relative_path) = path.strip_prefix(root_dir) else {
+      diagnostics_collector
+        .to_owned()
+        .push(PublishDiagnostic::InvalidPath {
+          path: path.to_path_buf(),
+          message: "path is not in publish directory".to_string(),
+        });
+      continue;
+    };
+
+    let relative_path =
+      relative_path
+        .components()
+        .fold("".to_string(), |mut path, component| {
+          path.push('/');
+          match component {
+            std::path::Component::Normal(normal) => {
+              path.push_str(&normal.to_string_lossy())
+            }
+            std::path::Component::CurDir => path.push('.'),
+            std::path::Component::ParentDir => path.push_str(".."),
+            _ => unreachable!(),
+          }
+          path
+        });
+
+    match PackagePath::new(relative_path.clone()) {
+      Ok(package_path) => {
+        if !paths.insert(package_path) {
+          diagnostics_collector.to_owned().push(
+            PublishDiagnostic::DuplicatePath {
+              path: path.to_path_buf(),
+            },
+          );
+        }
+      }
+      Err(err) => {
+        diagnostics_collector
+          .to_owned()
+          .push(PublishDiagnostic::InvalidPath {
+            path: path.to_path_buf(),
+            message: err.to_string(),
+          });
+      }
+    }
+
+    let media_type = MediaType::from_specifier(&specifier);
+    if matches!(media_type, MediaType::Jsx | MediaType::Tsx) {
+      diagnostics_collector.push(PublishDiagnostic::UnsupportedJsxTsx {
+        specifier: specifier.clone(),
+      });
+    }
+
+    result.push(CollectedPublishPath {
+      specifier,
+      path,
+      relative_path,
+    });
+  }
+
+  Ok(result)
+}
+
+fn collect_paths(
+  cli_options: &CliOptions,
+  diagnostics_collector: &PublishDiagnosticsCollector,
+  file_patterns: FilePatterns,
+) -> Result<Vec<PathBuf>, AnyError> {
+  FileCollector::new(|e| {
+    if !e.file_type.is_file() {
+      if let Ok(specifier) = ModuleSpecifier::from_file_path(e.path) {
+        diagnostics_collector.push(PublishDiagnostic::UnsupportedFileType {
+          specifier,
+          kind: if e.file_type.is_symlink() {
+            "symlink".to_owned()
+          } else {
+            format!("{:?}", e.file_type)
+          },
+        });
+      }
+      return false;
+    }
+    e.path
+      .file_name()
+      .map(|s| s != ".DS_Store" && s != ".gitignore")
+      .unwrap_or(true)
+  })
+  .ignore_git_folder()
+  .ignore_node_modules()
+  .set_vendor_folder(cli_options.vendor_dir_path().map(ToOwned::to_owned))
+  .use_gitignore()
+  .collect_file_patterns(file_patterns)
+}

cli/tools/registry/tar.rs

@@ -2,25 +2,20 @@
 
 use bytes::Bytes;
 use deno_ast::MediaType;
-use deno_ast::ModuleSpecifier;
-use deno_config::glob::FilePatterns;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
 use deno_core::url::Url;
 use sha2::Digest;
-use std::collections::HashSet;
 use std::fmt::Write as FmtWrite;
 use std::io::Write;
 use std::path::Path;
 use tar::Header;
 
-use crate::args::CliOptions;
 use crate::cache::LazyGraphSourceParser;
-use crate::tools::registry::paths::PackagePath;
-use crate::util::fs::FileCollector;
 
 use super::diagnostics::PublishDiagnostic;
 use super::diagnostics::PublishDiagnosticsCollector;
+use super::paths::CollectedPublishPath;
 use super::unfurl::SpecifierUnfurler;
 
 #[derive(Debug, Clone, PartialEq)]
@@ -39,117 +34,27 @@ pub struct PublishableTarball {
 }
 
 pub fn create_gzipped_tarball(
-  dir: &Path,
-  cli_options: &CliOptions,
+  publish_paths: &[CollectedPublishPath],
   source_parser: LazyGraphSourceParser,
   diagnostics_collector: &PublishDiagnosticsCollector,
   unfurler: &SpecifierUnfurler,
-  file_patterns: Option<FilePatterns>,
 ) -> Result<PublishableTarball, AnyError> {
-  let file_patterns = file_patterns
-    .unwrap_or_else(|| FilePatterns::new_with_base(dir.to_path_buf()));
   let mut tar = TarGzArchive::new();
   let mut files = vec![];
 
-  let iter_paths = FileCollector::new(|e| {
-    if !e.file_type.is_file() {
-      if let Ok(specifier) = ModuleSpecifier::from_file_path(e.path) {
-        diagnostics_collector.push(PublishDiagnostic::UnsupportedFileType {
-          specifier,
-          kind: if e.file_type.is_symlink() {
-            "symlink".to_owned()
-          } else {
-            format!("{:?}", e.file_type)
-          },
-        });
-      }
-      return false;
-    }
-    e.path
-      .file_name()
-      .map(|s| s != ".DS_Store" && s != ".gitignore")
-      .unwrap_or(true)
-  })
-  .ignore_git_folder()
-  .ignore_node_modules()
-  .set_vendor_folder(cli_options.vendor_dir_path().map(ToOwned::to_owned))
-  .use_gitignore()
-  .collect_file_patterns(file_patterns)?;
-
-  let mut paths = HashSet::with_capacity(iter_paths.len());
-
-  for path in iter_paths {
-    let Ok(specifier) = Url::from_file_path(&path) else {
-      diagnostics_collector
-        .to_owned()
-        .push(PublishDiagnostic::InvalidPath {
-          path: path.to_path_buf(),
-          message: "unable to convert path to url".to_string(),
-        });
-      continue;
-    };
-
-    let Ok(relative_path) = path.strip_prefix(dir) else {
-      diagnostics_collector
-        .to_owned()
-        .push(PublishDiagnostic::InvalidPath {
-          path: path.to_path_buf(),
-          message: "path is not in publish directory".to_string(),
-        });
-      continue;
-    };
-
-    let path_str =
-      relative_path
-        .components()
-        .fold("".to_string(), |mut path, component| {
-          path.push('/');
-          match component {
-            std::path::Component::Normal(normal) => {
-              path.push_str(&normal.to_string_lossy())
-            }
-            std::path::Component::CurDir => path.push('.'),
-            std::path::Component::ParentDir => path.push_str(".."),
-            _ => unreachable!(),
-          }
-          path
-        });
-
-    match PackagePath::new(path_str.clone()) {
-      Ok(package_path) => {
-        if !paths.insert(package_path) {
-          diagnostics_collector.to_owned().push(
-            PublishDiagnostic::DuplicatePath {
-              path: path.to_path_buf(),
-            },
-          );
-        }
-      }
-      Err(err) => {
-        diagnostics_collector
-          .to_owned()
-          .push(PublishDiagnostic::InvalidPath {
-            path: path.to_path_buf(),
-            message: err.to_string(),
-          });
-      }
-    }
+  for path in publish_paths {
+    let path_str = &path.relative_path;
+    let specifier = &path.specifier;
+    let path = &path.path;
 
     let content = resolve_content_maybe_unfurling(
-      &path,
-      &specifier,
+      path,
+      specifier,
       unfurler,
       source_parser,
       diagnostics_collector,
     )?;
 
-    let media_type = MediaType::from_specifier(&specifier);
-    if matches!(media_type, MediaType::Jsx | MediaType::Tsx) {
-      diagnostics_collector.push(PublishDiagnostic::UnsupportedJsxTsx {
-        specifier: specifier.clone(),
-      });
-    }
-
     files.push(PublishableTarballFile {
       path_str: path_str.clone(),
       specifier: specifier.clone(),

tests/specs/jsr/excluded_export_module/publish.out

@@ -3,14 +3,14 @@ Checking for slow types in the public API...
 Check file:///[WILDLINE]/main.ts
 error[excluded-module]: module in package's module graph was excluded from publishing
  --> [WILDLINE]excluded_file1.ts
-  = hint: remove the module from 'exclude' and/or 'publish.exclude' in the config file
+  = hint: remove the module from 'exclude' and/or 'publish.exclude' in the config file or use 'publish.exclude' with a negative glob to unexclude from gitignore
 
   info: excluded modules referenced via a package export will error at runtime due to not existing in the package
   docs: https://jsr.io/go/excluded-module
 
 error[excluded-module]: module in package's module graph was excluded from publishing
  --> [WILDLINE]excluded_file2.ts
-  = hint: remove the module from 'exclude' and/or 'publish.exclude' in the config file
+  = hint: remove the module from 'exclude' and/or 'publish.exclude' in the config file or use 'publish.exclude' with a negative glob to unexclude from gitignore
 
   info: excluded modules referenced via a package export will error at runtime due to not existing in the package
   docs: https://jsr.io/go/excluded-module

tests/specs/publish/dry_run_gitignored/__test__.jsonc

@@ -0,0 +1,14 @@
+{
+  "tempDir": true,
+  "steps": [{
+    "args": [
+      "eval",
+      "Deno.writeTextFileSync('.gitignore', 'gitignored.ts')"
+    ],
+    "output": "[WILDCARD]"
+  }, {
+    "args": "publish --dry-run",
+    "output": "mod.out",
+    "exitCode": 1
+  }]
+}

tests/specs/publish/dry_run_gitignored/deno.json

@@ -0,0 +1,5 @@
+{
+  "name": "@scope/pkg",
+  "version": "1.0.0",
+  "exports": "./mod.ts"
+}

tests/specs/publish/dry_run_gitignored/mod.out

@@ -0,0 +1,11 @@
+Check file:///[WILDLINE]/mod.ts
+Checking for slow types in the public API...
+Check file:///[WILDLINE]/mod.ts
+error[excluded-module]: module in package's module graph was excluded from publishing
+ --> [WILDLINE]gitignored.ts
+  = hint: remove the module from 'exclude' and/or 'publish.exclude' in the config file or use 'publish.exclude' with a negative glob to unexclude from gitignore
+
+  info: excluded modules referenced via a package export will error at runtime due to not existing in the package
+  docs: https://jsr.io/go/excluded-module
+
+error: Found 1 problem

tests/specs/publish/dry_run_gitignored/mod.ts

@@ -0,0 +1 @@
+import "./gitignored.ts";