Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(unstable/publish): error when a package's module is excluded from publishing #22948

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
82 changes: 53 additions & 29 deletions cli/tools/registry/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ pub enum PublishDiagnostic {
UnsupportedJsxTsx {
specifier: Url,
},
ExcludedModule {
specifier: Url,
},
}

impl PublishDiagnostic {
Expand Down Expand Up @@ -145,6 +148,7 @@ impl Diagnostic for PublishDiagnostic {
UnsupportedFileType { .. } => DiagnosticLevel::Warning,
InvalidExternalImport { .. } => DiagnosticLevel::Error,
UnsupportedJsxTsx { .. } => DiagnosticLevel::Warning,
ExcludedModule { .. } => DiagnosticLevel::Error,
}
}

Expand All @@ -158,6 +162,7 @@ impl Diagnostic for PublishDiagnostic {
UnsupportedFileType { .. } => Cow::Borrowed("unsupported-file-type"),
InvalidExternalImport { .. } => Cow::Borrowed("invalid-external-import"),
UnsupportedJsxTsx { .. } => Cow::Borrowed("unsupported-jsx-tsx"),
ExcludedModule { .. } => Cow::Borrowed("excluded-module"),
}
}

Expand All @@ -175,6 +180,7 @@ impl Diagnostic for PublishDiagnostic {
}
InvalidExternalImport { kind, .. } => Cow::Owned(format!("invalid import to a {kind} specifier")),
UnsupportedJsxTsx { .. } => Cow::Borrowed("JSX and TSX files are currently not supported"),
ExcludedModule { .. } => Cow::Borrowed("module in package's module graph was excluded from publishing"),
}
}

Expand Down Expand Up @@ -217,13 +223,17 @@ impl Diagnostic for PublishDiagnostic {
UnsupportedJsxTsx { specifier } => DiagnosticLocation::Module {
specifier: Cow::Borrowed(specifier),
},
ExcludedModule { specifier } => DiagnosticLocation::Module {
specifier: Cow::Borrowed(specifier),
},
}
}

fn snippet(&self) -> Option<DiagnosticSnippet<'_>> {
use PublishDiagnostic::*;
match &self {
PublishDiagnostic::FastCheck(diagnostic) => diagnostic.snippet(),
PublishDiagnostic::SpecifierUnfurl(diagnostic) => match diagnostic {
FastCheck(diagnostic) => diagnostic.snippet(),
SpecifierUnfurl(diagnostic) => match diagnostic {
SpecifierUnfurlerDiagnostic::UnanalyzableDynamicImport {
text_info,
range,
Expand All @@ -240,10 +250,10 @@ impl Diagnostic for PublishDiagnostic {
},
}),
},
PublishDiagnostic::InvalidPath { .. } => None,
PublishDiagnostic::DuplicatePath { .. } => None,
PublishDiagnostic::UnsupportedFileType { .. } => None,
PublishDiagnostic::InvalidExternalImport {
InvalidPath { .. } => None,
DuplicatePath { .. } => None,
UnsupportedFileType { .. } => None,
InvalidExternalImport {
referrer,
text_info,
..
Expand All @@ -264,31 +274,37 @@ impl Diagnostic for PublishDiagnostic {
description: Some("the specifier".into()),
},
}),
PublishDiagnostic::UnsupportedJsxTsx { .. } => None,
UnsupportedJsxTsx { .. } => None,
ExcludedModule { .. } => None,
}
}

fn hint(&self) -> Option<Cow<'_, str>> {
use PublishDiagnostic::*;
match &self {
PublishDiagnostic::FastCheck(diagnostic) => diagnostic.hint(),
PublishDiagnostic::SpecifierUnfurl(_) => None,
PublishDiagnostic::InvalidPath { .. } => Some(
FastCheck(diagnostic) => diagnostic.hint(),
SpecifierUnfurl(_) => None,
InvalidPath { .. } => Some(
Cow::Borrowed("rename or remove the file, or add it to 'publish.exclude' in the config file"),
),
PublishDiagnostic::DuplicatePath { .. } => Some(
DuplicatePath { .. } => Some(
Cow::Borrowed("rename or remove the file"),
),
PublishDiagnostic::UnsupportedFileType { .. } => Some(
UnsupportedFileType { .. } => Some(
Cow::Borrowed("remove the file, or add it to 'publish.exclude' in the config file"),
),
PublishDiagnostic::InvalidExternalImport { .. } => Some(Cow::Borrowed("replace this import with one from jsr or npm, or vendor the dependency into your package")),
PublishDiagnostic::UnsupportedJsxTsx { .. } => None,
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 or don't reference it in any code used by your package"),
),
}
}

fn snippet_fixed(&self) -> Option<DiagnosticSnippet<'_>> {
use PublishDiagnostic::*;
match &self {
PublishDiagnostic::InvalidExternalImport { imported, .. } => {
InvalidExternalImport { imported, .. } => {
match super::api::get_jsr_alternative(imported) {
Some(replacement) => {
let replacement = SourceTextInfo::new(replacement.into());
Expand All @@ -314,57 +330,65 @@ impl Diagnostic for PublishDiagnostic {
}

fn info(&self) -> Cow<'_, [Cow<'_, str>]> {
use PublishDiagnostic::*;
match &self {
PublishDiagnostic::FastCheck(diagnostic) => {
FastCheck(diagnostic) => {
diagnostic.info()
}
PublishDiagnostic::SpecifierUnfurl(diagnostic) => match diagnostic {
SpecifierUnfurl(diagnostic) => match diagnostic {
SpecifierUnfurlerDiagnostic::UnanalyzableDynamicImport { .. } => Cow::Borrowed(&[
Cow::Borrowed("after publishing this package, imports from the local import map / package.json do not work"),
Cow::Borrowed("dynamic imports that can not be analyzed at publish time will not be rewritten automatically"),
Cow::Borrowed("make sure the dynamic import is resolvable at runtime without an import map / package.json")
]),
},
PublishDiagnostic::InvalidPath { .. } => Cow::Borrowed(&[
InvalidPath { .. } => Cow::Borrowed(&[
Cow::Borrowed("to portably support all platforms, including windows, the allowed characters in package paths are limited"),
]),
PublishDiagnostic::DuplicatePath { .. } => Cow::Borrowed(&[
DuplicatePath { .. } => Cow::Borrowed(&[
Cow::Borrowed("to support case insensitive file systems, no two package paths may differ only by case"),
]),
PublishDiagnostic::UnsupportedFileType { .. } => Cow::Borrowed(&[
UnsupportedFileType { .. } => Cow::Borrowed(&[
Cow::Borrowed("only files and directories are supported"),
Cow::Borrowed("the file was ignored and will not be published")
]),
PublishDiagnostic::InvalidExternalImport { imported, .. } => Cow::Owned(vec![
InvalidExternalImport { imported, .. } => Cow::Owned(vec![
Cow::Owned(format!("the import was resolved to '{}'", imported)),
Cow::Borrowed("this specifier is not allowed to be imported on jsr"),
Cow::Borrowed("jsr only supports importing `jsr:`, `npm:`, and `data:` specifiers"),
]),
PublishDiagnostic::UnsupportedJsxTsx { .. } => Cow::Owned(vec![
UnsupportedJsxTsx { .. } => Cow::Owned(vec![
Cow::Borrowed("follow https://github.com/jsr-io/jsr/issues/24 for updates"),
]),
ExcludedModule { .. } => Cow::Owned(vec![
Cow::Borrowed("excluded modules referenced via a package export will error at runtime due to not existing in the package"),
])
}
}

fn docs_url(&self) -> Option<Cow<'_, str>> {
use PublishDiagnostic::*;
match &self {
PublishDiagnostic::FastCheck(diagnostic) => diagnostic.docs_url(),
PublishDiagnostic::SpecifierUnfurl(diagnostic) => match diagnostic {
FastCheck(diagnostic) => diagnostic.docs_url(),
SpecifierUnfurl(diagnostic) => match diagnostic {
SpecifierUnfurlerDiagnostic::UnanalyzableDynamicImport { .. } => None,
},
PublishDiagnostic::InvalidPath { .. } => {
InvalidPath { .. } => {
Some(Cow::Borrowed("https://jsr.io/go/invalid-path"))
}
PublishDiagnostic::DuplicatePath { .. } => Some(Cow::Borrowed(
DuplicatePath { .. } => Some(Cow::Borrowed(
"https://jsr.io/go/case-insensitive-duplicate-path",
)),
PublishDiagnostic::UnsupportedFileType { .. } => {
UnsupportedFileType { .. } => {
Some(Cow::Borrowed("https://jsr.io/go/unsupported-file-type"))
}
PublishDiagnostic::InvalidExternalImport { .. } => {
InvalidExternalImport { .. } => {
Some(Cow::Borrowed("https://jsr.io/go/invalid-external-import"))
}
PublishDiagnostic::UnsupportedJsxTsx { .. } => None,
UnsupportedJsxTsx { .. } => None,
ExcludedModule { .. } => {
Some(Cow::Borrowed("https://jsr.io/go/excluded-module"))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: add a page for this in the jsr registry

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to address this TODO before landing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, if this url looks ok.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay, but it's redirecting to the main doc page right now. I guess we'll create that page after landing this PR?

I misunderstood. Yeah looks good.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened jsr-io/jsr#318

}
}
}
}
32 changes: 32 additions & 0 deletions cli/tools/registry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ use std::sync::Arc;

use base64::prelude::BASE64_STANDARD;
use base64::Engine;
use deno_ast::ModuleSpecifier;
use deno_config::glob::FilePatterns;
use deno_config::ConfigFile;
use deno_config::WorkspaceMemberConfig;
use deno_core::anyhow::bail;
Expand Down Expand Up @@ -151,6 +153,14 @@ async fn prepare_publish(
sloppy_imports_resolver.as_ref(),
bare_node_builtins,
);
let root_specifier =
ModuleSpecifier::from_directory_path(&dir_path).unwrap();
collect_excluded_module_diagnostics(
&root_specifier,
&graph,
file_patterns.as_ref(),
&diagnostics_collector,
);
tar::create_gzipped_tarball(
&dir_path,
&cli_options,
Expand Down Expand Up @@ -192,6 +202,28 @@ async fn prepare_publish(
}))
}

fn collect_excluded_module_diagnostics(
root: &ModuleSpecifier,
graph: &deno_graph::ModuleGraph,
file_patterns: Option<&FilePatterns>,
diagnostics_collector: &PublishDiagnosticsCollector,
) {
let Some(file_patterns) = file_patterns else {
return;
};
let specifiers = graph
.specifiers()
.map(|(s, _)| s)
.filter(|s| s.as_str().starts_with(root.as_str()));
for specifier in specifiers {
if !file_patterns.matches_specifier(specifier) {
diagnostics_collector.push(PublishDiagnostic::ExcludedModule {
specifier: specifier.clone(),
});
}
}
}

#[derive(Serialize)]
#[serde(tag = "permission")]
pub enum Permission<'s> {
Expand Down
7 changes: 7 additions & 0 deletions tests/specs/jsr/excluded_export_module/__test__.jsonc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// test for a module that's excluded from being
// published, but is found in the module graph
{
"args": "publish --dry-run",
"output": "publish.out",
"exitCode": 1
}
12 changes: 12 additions & 0 deletions tests/specs/jsr/excluded_export_module/deno.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"name": "@denotest/excluded-export-module",
"version": "0.1.0",
"exports": "./main.ts",
"publish": {
"exclude": [
"./excluded_file1.ts",
"./excluded_file2.ts",
"./not_imported_excluded_file.ts"
]
}
}
Empty file.
Empty file.
2 changes: 2 additions & 0 deletions tests/specs/jsr/excluded_export_module/main.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import "./excluded_file1.ts";
import "./excluded_file2.ts";
Empty file.
18 changes: 18 additions & 0 deletions tests/specs/jsr/excluded_export_module/publish.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
Check file:///[WILDLINE]/main.ts
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 or don't reference it in any code used by your package

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 or don't reference it in any code used by your package

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 2 problems