chore: improve unanalyzable dynamic import diagnostic (#22051)

4 files changed, 103 insertions, 45 deletions

diff --git a/cli/tools/registry/diagnostics.rs b/cli/tools/registry/diagnostics.rs
index f6d81f5a8..0a847c46b 100644
--- a/cli/tools/registry/diagnostics.rs
+++ b/cli/tools/registry/diagnostics.rs
@@ -21,6 +21,7 @@ use crate::diagnostics::DiagnosticSnippetSource;
 use crate::diagnostics::DiagnosticSourcePos;
 use crate::diagnostics::DiagnosticSourceRange;
 use crate::diagnostics::SourceTextParsedSourceStore;
+use crate::util::import_map::ImportMapUnfurlDiagnostic;
 
 #[derive(Clone, Default)]
 pub struct PublishDiagnosticsCollector {
@@ -36,7 +37,7 @@ impl PublishDiagnosticsCollector {
     let diagnostics = self.diagnostics.lock().unwrap().take();
     let sources = SourceTextParsedSourceStore(sources);
     for diagnostic in diagnostics {
-      eprintln!("{}", diagnostic.display(&sources));
+      eprint!("{}", diagnostic.display(&sources));
       if matches!(diagnostic.level(), DiagnosticLevel::Error) {
         errors += 1;
       }
@@ -58,34 +59,42 @@ impl PublishDiagnosticsCollector {
 }
 
 pub enum PublishDiagnostic {
-  FastCheck { diagnostic: FastCheckDiagnostic },
+  FastCheck(FastCheckDiagnostic),
+  ImportMapUnfurl(ImportMapUnfurlDiagnostic),
 }
 
 impl Diagnostic for PublishDiagnostic {
   fn level(&self) -> DiagnosticLevel {
     match self {
-      PublishDiagnostic::FastCheck {
-        diagnostic: FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. },
-      } => DiagnosticLevel::Warning,
-      PublishDiagnostic::FastCheck { .. } => DiagnosticLevel::Error,
+      PublishDiagnostic::FastCheck(
+        FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. },
+      ) => DiagnosticLevel::Warning,
+      PublishDiagnostic::FastCheck(_) => DiagnosticLevel::Error,
+      PublishDiagnostic::ImportMapUnfurl(_) => DiagnosticLevel::Warning,
     }
   }
 
   fn code(&self) -> impl Display + '_ {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic, .. } => diagnostic.code(),
+      PublishDiagnostic::FastCheck(diagnostic) => diagnostic.code(),
+      PublishDiagnostic::ImportMapUnfurl(diagnostic) => diagnostic.code(),
     }
   }
 
   fn message(&self) -> impl Display + '_ {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic, .. } => diagnostic.to_string(), // todo
+      PublishDiagnostic::FastCheck(diagnostic) => {
+        Cow::Owned(diagnostic.to_string())
+      }
+      PublishDiagnostic::ImportMapUnfurl(diagnostic) => {
+        Cow::Borrowed(diagnostic.message())
+      }
     }
   }
 
   fn location(&self) -> DiagnosticLocation {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic } => match diagnostic.range() {
+      PublishDiagnostic::FastCheck(diagnostic) => match diagnostic.range() {
         Some(range) => DiagnosticLocation::PositionInFile {
           specifier: Cow::Borrowed(diagnostic.specifier()),
           source_pos: DiagnosticSourcePos::SourcePos(range.range.start),
@@ -94,12 +103,21 @@ impl Diagnostic for PublishDiagnostic {
           specifier: Cow::Borrowed(diagnostic.specifier()),
         },
       },
+      PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic {
+        ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport {
+          specifier,
+          range,
+        } => DiagnosticLocation::PositionInFile {
+          specifier: Cow::Borrowed(specifier),
+          source_pos: DiagnosticSourcePos::SourcePos(range.start),
+        },
+      },
     }
   }
 
   fn snippet(&self) -> Option<DiagnosticSnippet<'_>> {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic } => {
+      PublishDiagnostic::FastCheck(diagnostic) => {
         diagnostic.range().map(|range| DiagnosticSnippet {
           source: DiagnosticSnippetSource::Specifier(Cow::Borrowed(
             diagnostic.specifier(),
@@ -114,14 +132,29 @@ impl Diagnostic for PublishDiagnostic {
           },
         })
       }
+      PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic {
+        ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport {
+          specifier,
+          range,
+        } => Some(DiagnosticSnippet {
+          source: DiagnosticSnippetSource::Specifier(Cow::Borrowed(specifier)),
+          highlight: DiagnosticSnippetHighlight {
+            style: DiagnosticSnippetHighlightStyle::Warning,
+            range: DiagnosticSourceRange {
+              start: DiagnosticSourcePos::SourcePos(range.start),
+              end: DiagnosticSourcePos::SourcePos(range.end),
+            },
+            description: Some("the unanalyzable dynamic import".into()),
+          },
+        }),
+      },
     }
   }
 
   fn hint(&self) -> Option<impl Display + '_> {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic } => {
-        Some(diagnostic.fix_hint())
-      }
+      PublishDiagnostic::FastCheck(diagnostic) => Some(diagnostic.fix_hint()),
+      PublishDiagnostic::ImportMapUnfurl(_) => None,
     }
   }
 
@@ -131,7 +164,7 @@ impl Diagnostic for PublishDiagnostic {
 
   fn info(&self) -> Cow<'_, [Cow<'_, str>]> {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic } => {
+      PublishDiagnostic::FastCheck(diagnostic) => {
         let infos = diagnostic
           .additional_info()
           .iter()
@@ -139,14 +172,24 @@ impl Diagnostic for PublishDiagnostic {
           .collect();
         Cow::Owned(infos)
       }
+      PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic {
+        ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { .. } => Cow::Borrowed(&[
+          Cow::Borrowed("after publishing this package, imports from the local import map 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")
+        ]),
+      },
     }
   }
 
   fn docs_url(&self) -> Option<impl Display + '_> {
     match &self {
-      PublishDiagnostic::FastCheck { diagnostic } => {
+      PublishDiagnostic::FastCheck(diagnostic) => {
         Some(format!("https://jsr.io/go/{}", diagnostic.code()))
       }
+      PublishDiagnostic::ImportMapUnfurl(diagnostic) => match diagnostic {
+        ImportMapUnfurlDiagnostic::UnanalyzableDynamicImport { .. } => None,
+      },
     }
   }
 }
diff --git a/cli/tools/registry/graph.rs b/cli/tools/registry/graph.rs
index 29c825242..4d061e3da 100644
--- a/cli/tools/registry/graph.rs
+++ b/cli/tools/registry/graph.rs
@@ -94,9 +94,8 @@ pub fn collect_fast_check_type_graph_diagnostics(
           {
             continue;
           }
-          diagnostics_collector.push(PublishDiagnostic::FastCheck {
-            diagnostic: diagnostic.clone(),
-          });
+          diagnostics_collector
+            .push(PublishDiagnostic::FastCheck(diagnostic.clone()));
           if matches!(
             diagnostic,
             FastCheckDiagnostic::UnsupportedJavaScriptEntrypoint { .. }
diff --git a/cli/tools/registry/mod.rs b/cli/tools/registry/mod.rs
index 8eaf61258..96f2d0f13 100644
--- a/cli/tools/registry/mod.rs
+++ b/cli/tools/registry/mod.rs
@@ -11,9 +11,9 @@ use deno_config::ConfigFile;
 use deno_core::anyhow::bail;
 use deno_core::anyhow::Context;
 use deno_core::error::AnyError;
+use deno_core::futures::FutureExt;
 use deno_core::serde_json;
 use deno_core::serde_json::json;
-use deno_core::unsync::JoinHandle;
 use deno_core::unsync::JoinSet;
 use deno_runtime::colors;
 use deno_runtime::deno_fetch::reqwest;
@@ -89,6 +89,7 @@ async fn prepare_publish(
   deno_json: &ConfigFile,
   source_cache: Arc<ParsedSourceCache>,
   import_map: Arc<ImportMap>,
+  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();
@@ -134,11 +135,13 @@ async fn prepare_publish(
     .to_files_config()
     .map(|files| files.map(|f| f.exclude).unwrap_or_default())?;
 
+  let diagnostics_collector = diagnostics_collector.clone();
   let tarball = deno_core::unsync::spawn_blocking(move || {
     let unfurler = ImportMapUnfurler::new(&import_map);
     tar::create_gzipped_tarball(
       &dir_path,
       &*source_cache,
+      &diagnostics_collector,
       &unfurler,
       &exclude_patterns,
     )
@@ -666,8 +669,13 @@ async fn prepare_packages_for_publishing(
     )
     .await?;
     let mut prepared_package_by_name = HashMap::with_capacity(1);
-    let package =
-      prepare_publish(&deno_json, source_cache.clone(), import_map).await?;
+    let package = prepare_publish(
+      &deno_json,
+      source_cache.clone(),
+      import_map,
+      diagnostics_collector,
+    )
+    .await?;
     let package_name = format!("@{}/{}", package.scope, package.package);
     let publish_order_graph =
       PublishOrderGraph::new_single(package_name.clone());
@@ -692,29 +700,31 @@ async fn prepare_packages_for_publishing(
   let publish_order_graph =
     publish_order::build_publish_order_graph(&graph, &roots)?;
 
-  let results =
-    workspace_config
-      .members
-      .iter()
-      .cloned()
-      .map(|member| {
-        let import_map = import_map.clone();
-        let source_cache = source_cache.clone();
-        deno_core::unsync::spawn(async move {
-          let package = prepare_publish(&member.config_file, source_cache, import_map)
-            .await
-            .with_context(|| {
-              format!("Failed preparing '{}'.", member.package_name)
-            })?;
-          Ok((member.package_name, package))
-        })
-      })
-      .collect::<Vec<
-        JoinHandle<Result<(String, Rc<PreparedPublishPackage>), AnyError>>,
-      >>();
+  let results = workspace_config
+    .members
+    .iter()
+    .cloned()
+    .map(|member| {
+      let import_map = import_map.clone();
+      async move {
+        let package = prepare_publish(
+          &member.config_file,
+          source_cache.clone(),
+          import_map.clone(),
+          diagnostics_collector,
+        )
+        .await
+        .with_context(|| {
+          format!("Failed preparing '{}'.", member.package_name)
+        })?;
+        Ok::<_, AnyError>((member.package_name, package))
+      }
+      .boxed()
+    })
+    .collect::<Vec<_>>();
   let results = deno_core::futures::future::join_all(results).await;
   for result in results {
-    let (package_name, package) = result??;
+    let (package_name, package) = result?;
     prepared_package_by_name.insert(package_name, package);
   }
   Ok((publish_order_graph, prepared_package_by_name))
diff --git a/cli/tools/registry/tar.rs b/cli/tools/registry/tar.rs
index 0f6edbc3a..9bd7f098e 100644
--- a/cli/tools/registry/tar.rs
+++ b/cli/tools/registry/tar.rs
@@ -15,6 +15,9 @@ use tar::Header;
 use crate::util::import_map::ImportMapUnfurler;
 use deno_config::glob::PathOrPatternSet;
 
+use super::diagnostics::PublishDiagnostic;
+use super::diagnostics::PublishDiagnosticsCollector;
+
 #[derive(Debug, Clone, PartialEq)]
 pub struct PublishableTarballFile {
   pub path: PathBuf,
@@ -32,6 +35,7 @@ pub struct PublishableTarball {
 pub fn create_gzipped_tarball(
   dir: &Path,
   source_cache: &dyn deno_graph::ParsedSourceStore,
+  diagnostics_collector: &PublishDiagnosticsCollector,
   unfurler: &ImportMapUnfurler,
   exclude_patterns: &PathOrPatternSet,
 ) -> Result<PublishableTarball, AnyError> {
@@ -72,9 +76,11 @@ pub fn create_gzipped_tarball(
       });
       let content = match source_cache.get_parsed_source(&url) {
         Some(parsed_source) => {
-          let (content, unfurl_diagnostics) =
-            unfurler.unfurl(&url, &parsed_source);
-          diagnostics.extend_from_slice(&unfurl_diagnostics);
+          let mut reporter = |diagnostic| {
+            diagnostics_collector
+              .push(PublishDiagnostic::ImportMapUnfurl(diagnostic));
+          };
+          let content = unfurler.unfurl(&url, &parsed_source, &mut reporter);
           content.into_bytes()
         }
         None => data,