From 191f4f16ad0cf612c5fa18009441d266b14e1c14 Mon Sep 17 00:00:00 2001
From: David Sherret <dsherret@users.noreply.github.com>
Date: Mon, 14 Nov 2022 19:55:08 -0500
Subject: [PATCH] fix(bundle): explicit error when using an npm specifier with
 deno bundle (#16637)

---
 cli/main.rs                        | 62 +++++++++++++++++++-----------
 cli/tests/integration/npm_tests.rs | 12 +++++-
 2 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/cli/main.rs b/cli/main.rs
index 31ac4b3314..01ef738a31 100644
--- a/cli/main.rs
+++ b/cli/main.rs
@@ -68,7 +68,6 @@ use crate::emit::TsConfigType;
 use crate::file_fetcher::File;
 use crate::file_watcher::ResolutionResult;
 use crate::graph_util::graph_lock_or_exit;
-use crate::graph_util::graph_valid;
 use crate::proc_state::ProcState;
 use crate::resolver::CliResolver;
 use crate::tools::check;
@@ -89,6 +88,7 @@ use deno_runtime::colors;
 use deno_runtime::fmt_errors::format_js_error;
 use deno_runtime::permissions::Permissions;
 use deno_runtime::tokio_util::run_local;
+use graph_util::GraphData;
 use log::debug;
 use log::info;
 use npm::NpmPackageReference;
@@ -150,22 +150,10 @@ async fn compile_command(
     generic_error("There should only be one reference to ModuleGraph")
   })?;
 
-  graph.valid().unwrap();
-
   // at the moment, we don't support npm specifiers in deno_compile, so show an error
-  let first_npm_specifier = graph
-    .specifiers()
-    .values()
-    .filter_map(|r| match r {
-      Ok((specifier, kind, _)) if *kind == deno_graph::ModuleKind::External => {
-        Some(specifier.clone())
-      }
-      _ => None,
-    })
-    .next();
-  if let Some(npm_specifier) = first_npm_specifier {
-    bail!("npm specifiers have not yet been implemented for deno compile (https://github.com/denoland/deno/issues/15960). Found: {}", npm_specifier)
-  }
+  error_for_any_npm_specifier(&graph)?;
+
+  graph.valid().unwrap();
 
   let parser = ps.parsed_source_cache.as_capturing_parser();
   let eszip = eszip::EszipV2::from_graph(graph, &parser, Default::default())?;
@@ -384,11 +372,18 @@ async fn create_graph_and_maybe_check(
   );
 
   let check_js = ps.options.check_js();
-  graph_valid(
-    &graph,
-    ps.options.type_check_mode() != TypeCheckMode::None,
-    check_js,
-  )?;
+  let mut graph_data = GraphData::default();
+  graph_data.add_graph(&graph, false);
+  graph_data
+    .check(
+      &graph.roots,
+      ps.options.type_check_mode() != TypeCheckMode::None,
+      check_js,
+    )
+    .unwrap()?;
+  ps.npm_resolver
+    .add_package_reqs(graph_data.npm_package_reqs().clone())
+    .await?;
   graph_lock_or_exit(&graph);
 
   if ps.options.type_check_mode() != TypeCheckMode::None {
@@ -403,7 +398,7 @@ async fn create_graph_and_maybe_check(
     let cache = TypeCheckCache::new(&ps.dir.type_checking_cache_db_file_path());
     let check_result = check::check(
       &graph.roots,
-      Arc::new(RwLock::new(graph.as_ref().into())),
+      Arc::new(RwLock::new(graph_data)),
       &cache,
       ps.npm_resolver.clone(),
       check::CheckOptions {
@@ -500,6 +495,9 @@ async fn bundle_command(
   let operation = |(ps, graph): (ProcState, Arc<deno_graph::ModuleGraph>)| {
     let out_file = bundle_flags.out_file.clone();
     async move {
+      // at the moment, we don't support npm specifiers in deno bundle, so show an error
+      error_for_any_npm_specifier(&graph)?;
+
       let bundle_output = bundle_module_graph(graph.as_ref(), &ps)?;
       debug!(">>>>> bundle END");
 
@@ -561,6 +559,26 @@ async fn bundle_command(
   Ok(0)
 }
 
+fn error_for_any_npm_specifier(
+  graph: &deno_graph::ModuleGraph,
+) -> Result<(), AnyError> {
+  let first_npm_specifier = graph
+    .specifiers()
+    .values()
+    .filter_map(|r| match r {
+      Ok((specifier, kind, _)) if *kind == deno_graph::ModuleKind::External => {
+        Some(specifier.clone())
+      }
+      _ => None,
+    })
+    .next();
+  if let Some(npm_specifier) = first_npm_specifier {
+    bail!("npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: {}", npm_specifier)
+  } else {
+    Ok(())
+  }
+}
+
 async fn doc_command(
   flags: Flags,
   doc_flags: DocFlags,
diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs
index 07460d9906..3de6e5f2d9 100644
--- a/cli/tests/integration/npm_tests.rs
+++ b/cli/tests/integration/npm_tests.rs
@@ -719,8 +719,16 @@ fn ensure_registry_files_local() {
 }
 
 itest!(compile_errors {
-  args: "compile -A --quiet npm/esm/main.js",
-  output_str: Some("error: npm specifiers have not yet been implemented for deno compile (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
+  args: "compile -A --quiet npm/cached_only/main.ts",
+  output_str: Some("error: npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
+  exit_code: 1,
+  envs: env_vars(),
+  http_server: true,
+});
+
+itest!(bundle_errors {
+  args: "bundle --quiet npm/esm/main.js",
+  output_str: Some("error: npm specifiers have not yet been implemented for this sub command (https://github.com/denoland/deno/issues/15960). Found: npm:chalk@5\n"),
   exit_code: 1,
   envs: env_vars(),
   http_server: true,