fix(watcher): watcher doesn't exit when module resolution fails (#8521)

This commit makes the file watcher continue to work even if module
resolution fails at the initial attempt, allowing us to execute `run`
or `bundle` subcommand when a script has invalid syntax. In such
cases, the watcher observes a single file that is specified as an
command line argument.

cli/file_watcher.rs

@@ -22,7 +22,7 @@ use tokio::time::{delay_for, Delay};
 
 const DEBOUNCE_INTERVAL_MS: Duration = Duration::from_millis(200);
 
-type FileWatcherFuture<T> = Pin<Box<dyn Future<Output = Result<T, AnyError>>>>;
+type FileWatcherFuture<T> = Pin<Box<dyn Future<Output = T>>>;
 
 struct Debounce {
   delay: Delay,
@@ -63,7 +63,7 @@ impl Stream for Debounce {
   }
 }
 
-async fn error_handler(watch_future: FileWatcherFuture<()>) {
+async fn error_handler(watch_future: FileWatcherFuture<Result<(), AnyError>>) {
   let result = watch_future.await;
   if let Err(err) = result {
     let msg = format!("{}: {}", colors::red_bold("error"), err.to_string(),);
@@ -94,7 +94,7 @@ pub async fn watch_func<F, G>(
 ) -> Result<(), AnyError>
 where
   F: Fn() -> Result<Vec<PathBuf>, AnyError>,
-  G: Fn(Vec<PathBuf>) -> FileWatcherFuture<()>,
+  G: Fn(Vec<PathBuf>) -> FileWatcherFuture<Result<(), AnyError>>,
 {
   let mut debounce = Debounce::new();
 
@@ -129,6 +129,17 @@ where
   }
 }
 
+pub enum ModuleResolutionResult<T> {
+  Success {
+    paths_to_watch: Vec<PathBuf>,
+    module_info: T,
+  },
+  Fail {
+    source_path: PathBuf,
+    error: AnyError,
+  },
+}
+
 /// This function adds watcher functionality to subcommands like `run` or `bundle`.
 /// The difference from [`watch_func`] is that this does depend on [`ModuleGraph`].
 ///
@@ -154,36 +165,36 @@ pub async fn watch_func_with_module_resolution<F, G, T>(
   job_name: &str,
 ) -> Result<(), AnyError>
 where
-  F: Fn() -> FileWatcherFuture<(Vec<PathBuf>, T)>,
+  F: Fn() -> FileWatcherFuture<ModuleResolutionResult<T>>,
-  G: Fn(T) -> FileWatcherFuture<()>,
+  G: Fn(T) -> FileWatcherFuture<Result<(), AnyError>>,
   T: Clone,
 {
   let mut debounce = Debounce::new();
   // Store previous data. If module resolution fails at some point, the watcher will try to
   // continue watching files using these data.
-  let mut paths = None;
+  let mut paths;
   let mut module = None;
 
   loop {
     match module_resolver().await {
-      Ok((next_paths, next_module)) => {
+      ModuleResolutionResult::Success {
-        paths = Some(next_paths);
+        paths_to_watch,
-        module = Some(next_module);
+        module_info,
+      } => {
+        paths = paths_to_watch;
+        module = Some(module_info);
       }
-      Err(e) => {
+      ModuleResolutionResult::Fail { source_path, error } => {
-        // If at least one of `paths` and `module` is `None`, the watcher cannot decide which files
+        paths = vec![source_path];
-        // should be watched. So return the error immediately without watching anything.
+        if module.is_none() {
-        if paths.is_none() || module.is_none() {
+          eprintln!("{}: {}", colors::red_bold("error"), error);
-          return Err(e);
         }
       }
     }
-    // These `unwrap`s never cause panic since `None` is already checked above.
+    let _watcher = new_watcher(&paths, &debounce)?;
-    let cur_paths = paths.clone().unwrap();
-    let cur_module = module.clone().unwrap();
 
-    let _watcher = new_watcher(&cur_paths, &debounce)?;
+    if let Some(module) = &module {
-    let func = error_handler(operation(cur_module));
+      let func = error_handler(operation(module.clone()));
       let mut is_file_changed = false;
       select! {
         _ = debounce.next() => {
@@ -208,6 +219,18 @@ where
           colors::intense_blue("Watcher"),
         );
       }
+    } else {
+      info!(
+        "{} {} failed! Restarting on file change...",
+        colors::intense_blue("Watcher"),
+        job_name,
+      );
+      debounce.next().await;
+      info!(
+        "{} File change detected! Restarting!",
+        colors::intense_blue("Watcher"),
+      );
+    }
   }
 }
 

cli/main.rs

@@ -50,6 +50,7 @@ mod worker;
 
 use crate::file_fetcher::File;
 use crate::file_fetcher::FileFetcher;
+use crate::file_watcher::ModuleResolutionResult;
 use crate::media_type::MediaType;
 use crate::permissions::Permissions;
 use crate::program_state::ProgramState;
@@ -307,10 +308,11 @@ async fn bundle_command(
 
   let module_resolver = || {
     let flags = flags.clone();
-    let source_file = source_file.clone();
+    let source_file1 = source_file.clone();
+    let source_file2 = source_file.clone();
     async move {
       let module_specifier =
-        ModuleSpecifier::resolve_url_or_path(&source_file)?;
+        ModuleSpecifier::resolve_url_or_path(&source_file1)?;
 
       debug!(">>>>> bundle START");
       let program_state = ProgramState::new(flags.clone())?;
@@ -373,6 +375,16 @@ async fn bundle_command(
 
       Ok((paths_to_watch, module_graph))
     }
+    .map(move |result| match result {
+      Ok((paths_to_watch, module_graph)) => ModuleResolutionResult::Success {
+        paths_to_watch,
+        module_info: module_graph,
+      },
+      Err(e) => ModuleResolutionResult::Fail {
+        source_path: PathBuf::from(source_file2),
+        error: e,
+      },
+    })
     .boxed_local()
   };
 
@@ -423,7 +435,10 @@ async fn bundle_command(
     )
     .await?;
   } else {
-    let (_, module_graph) = module_resolver().await?;
+    let module_graph = match module_resolver().await {
+      ModuleResolutionResult::Fail { error, .. } => return Err(error),
+      ModuleResolutionResult::Success { module_info, .. } => module_info,
+    };
     operation(module_graph).await?;
   }
 
@@ -610,10 +625,11 @@ async fn run_from_stdin(flags: Flags) -> Result<(), AnyError> {
 
 async fn run_with_watch(flags: Flags, script: String) -> Result<(), AnyError> {
   let module_resolver = || {
-    let script = script.clone();
+    let script1 = script.clone();
+    let script2 = script.clone();
     let flags = flags.clone();
     async move {
-      let main_module = ModuleSpecifier::resolve_url_or_path(&script)?;
+      let main_module = ModuleSpecifier::resolve_url_or_path(&script1)?;
       let program_state = ProgramState::new(flags)?;
       let handler = Rc::new(RefCell::new(FetchHandler::new(
         &program_state,
@@ -641,6 +657,16 @@ async fn run_with_watch(flags: Flags, script: String) -> Result<(), AnyError> {
 
       Ok((paths_to_watch, main_module))
     }
+    .map(move |result| match result {
+      Ok((paths_to_watch, module_info)) => ModuleResolutionResult::Success {
+        paths_to_watch,
+        module_info,
+      },
+      Err(e) => ModuleResolutionResult::Fail {
+        source_path: PathBuf::from(script2),
+        error: e,
+      },
+    })
     .boxed_local()
   };
 

cli/tests/integration_tests.rs

@@ -527,6 +527,9 @@ fn fmt_watch_test() {
   let actual = std::fs::read_to_string(badly_formatted).unwrap();
   assert_eq!(expected, actual);
 
+  // the watcher process is still alive
+  assert!(child.try_wait().unwrap().is_none());
+
   child.kill().unwrap();
   drop(t);
 }
@@ -1231,7 +1234,7 @@ fn bundle_js_watch() {
   assert!(stderr_lines.next().unwrap().contains("mod6.bundle.js"));
   let file = PathBuf::from(&bundle);
   assert!(file.is_file());
-  assert!(stderr_lines.next().unwrap().contains("Bundle finished!"));
+  wait_for_process_finished("Bundle", &mut stderr_lines);
 
   std::fs::write(&file_to_watch, "console.log('Hello world2');")
     .expect("error writing file");
@@ -1244,7 +1247,7 @@ fn bundle_js_watch() {
   assert!(stderr_lines.next().unwrap().contains("mod6.bundle.js"));
   let file = PathBuf::from(&bundle);
   assert!(file.is_file());
-  assert!(stderr_lines.next().unwrap().contains("Bundle finished!"));
+  wait_for_process_finished("Bundle", &mut stderr_lines);
 
   // Confirm that the watcher keeps on working even if the file is updated and has invalid syntax
   std::fs::write(&file_to_watch, "syntax error ^^")
@@ -1258,24 +1261,29 @@ fn bundle_js_watch() {
   assert!(stderr_lines.next().unwrap().contains("mod6.bundle.js"));
   let file = PathBuf::from(&bundle);
   assert!(file.is_file());
-  assert!(stderr_lines.next().unwrap().contains("Bundle finished!"));
+  wait_for_process_finished("Bundle", &mut stderr_lines);
+
+  // the watcher process is still alive
+  assert!(deno.try_wait().unwrap().is_none());
 
   deno.kill().unwrap();
   drop(t);
 }
 
-/// Confirm that the watcher exits immediately if module resolution fails at the first attempt
+/// Confirm that the watcher continues to work even if module resolution fails at the *first* attempt
 #[test]
-fn bundle_watch_fail() {
+fn bundle_watch_not_exit() {
   let t = TempDir::new().expect("tempdir fail");
   let file_to_watch = t.path().join("file_to_watch.js");
   std::fs::write(&file_to_watch, "syntax error ^^")
     .expect("error writing file");
+  let target_file = t.path().join("target.js");
 
   let mut deno = util::deno_cmd()
     .current_dir(util::root_path())
     .arg("bundle")
     .arg(&file_to_watch)
+    .arg(&target_file)
     .arg("--watch")
     .arg("--unstable")
     .env("NO_COLOR", "1")
@@ -1291,7 +1299,26 @@ fn bundle_watch_fail() {
   std::thread::sleep(std::time::Duration::from_secs(1));
   assert!(stderr_lines.next().unwrap().contains("file_to_watch.js"));
   assert!(stderr_lines.next().unwrap().contains("error:"));
-  assert!(!deno.wait().unwrap().success());
+  assert!(stderr_lines.next().unwrap().contains("Bundle failed!"));
+  // the target file hasn't been created yet
+  assert!(!target_file.is_file());
+
+  // Make sure the watcher actually restarts and works fine with the proper syntax
+  std::fs::write(&file_to_watch, "console.log(42);")
+    .expect("error writing file");
+  std::thread::sleep(std::time::Duration::from_secs(1));
+  assert!(stderr_lines
+    .next()
+    .unwrap()
+    .contains("File change detected!"));
+  assert!(stderr_lines.next().unwrap().contains("file_to_watch.js"));
+  assert!(stderr_lines.next().unwrap().contains("target.js"));
+  wait_for_process_finished("Bundle", &mut stderr_lines);
+  // bundled file is created
+  assert!(target_file.is_file());
+
+  // the watcher process is still alive
+  assert!(deno.try_wait().unwrap().is_none());
 
   drop(t);
 }
@@ -1328,12 +1355,16 @@ fn info_with_compiled_source() {
   assert_eq!(output.stderr, b"");
 }
 
-// Helper function to skip watcher output that doesn't contain
+/// Helper function to skip watcher output that doesn't contain
-// "Process finished" phrase.
+/// "{job_name} finished" phrase.
-fn wait_for_process_finished(stderr_lines: &mut impl Iterator<Item = String>) {
+fn wait_for_process_finished(
+  job_name: &str,
+  stderr_lines: &mut impl Iterator<Item = String>,
+) {
+  let phrase = format!("{} finished", job_name);
   loop {
     let msg = stderr_lines.next().unwrap();
-    if msg.contains("Process finished") {
+    if msg.contains(&phrase) {
       break;
     }
   }
@@ -1366,7 +1397,7 @@ fn run_watch() {
     std::io::BufReader::new(stderr).lines().map(|r| r.unwrap());
 
   assert!(stdout_lines.next().unwrap().contains("Hello world"));
-  wait_for_process_finished(&mut stderr_lines);
+  wait_for_process_finished("Process", &mut stderr_lines);
 
   // TODO(lucacasonato): remove this timeout. It seems to be needed on Linux.
   std::thread::sleep(std::time::Duration::from_secs(1));
@@ -1379,7 +1410,7 @@ fn run_watch() {
 
   assert!(stderr_lines.next().unwrap().contains("Restarting"));
   assert!(stdout_lines.next().unwrap().contains("Hello world2"));
-  wait_for_process_finished(&mut stderr_lines);
+  wait_for_process_finished("Process", &mut stderr_lines);
 
   // Add dependency
   let another_file = t.path().join("another_file.js");
@@ -1393,7 +1424,7 @@ fn run_watch() {
   std::thread::sleep(std::time::Duration::from_secs(1));
   assert!(stderr_lines.next().unwrap().contains("Restarting"));
   assert!(stdout_lines.next().unwrap().contains('0'));
-  wait_for_process_finished(&mut stderr_lines);
+  wait_for_process_finished("Process", &mut stderr_lines);
 
   // Confirm that restarting occurs when a new file is updated
   std::fs::write(&another_file, "export const foo = 42;")
@@ -1401,7 +1432,7 @@ fn run_watch() {
   std::thread::sleep(std::time::Duration::from_secs(1));
   assert!(stderr_lines.next().unwrap().contains("Restarting"));
   assert!(stdout_lines.next().unwrap().contains("42"));
-  wait_for_process_finished(&mut stderr_lines);
+  wait_for_process_finished("Process", &mut stderr_lines);
 
   // Confirm that the watcher keeps on working even if the file is updated and has invalid syntax
   std::fs::write(&file_to_watch, "syntax error ^^")
@@ -1409,7 +1440,7 @@ fn run_watch() {
   std::thread::sleep(std::time::Duration::from_secs(1));
   assert!(stderr_lines.next().unwrap().contains("Restarting"));
   assert!(stderr_lines.next().unwrap().contains("error:"));
-  wait_for_process_finished(&mut stderr_lines);
+  wait_for_process_finished("Process", &mut stderr_lines);
 
   // Then restore the file
   std::fs::write(
@@ -1420,15 +1451,18 @@ fn run_watch() {
   std::thread::sleep(std::time::Duration::from_secs(1));
   assert!(stderr_lines.next().unwrap().contains("Restarting"));
   assert!(stdout_lines.next().unwrap().contains("42"));
-  wait_for_process_finished(&mut stderr_lines);
+  wait_for_process_finished("Process", &mut stderr_lines);
+
+  // the watcher process is still alive
+  assert!(child.try_wait().unwrap().is_none());
 
   child.kill().unwrap();
   drop(t);
 }
 
-/// Confirm that the watcher exits immediately if module resolution fails at the first attempt
+/// Confirm that the watcher continues to work even if module resolution fails at the *first* attempt
 #[test]
-fn run_watch_fail() {
+fn run_watch_not_exit() {
   let t = TempDir::new().expect("tempdir fail");
   let file_to_watch = t.path().join("file_to_watch.js");
   std::fs::write(&file_to_watch, "syntax error ^^")
@@ -1437,22 +1471,36 @@ fn run_watch_fail() {
   let mut child = util::deno_cmd()
     .current_dir(util::root_path())
     .arg("run")
-    .arg(&file_to_watch)
     .arg("--watch")
     .arg("--unstable")
+    .arg(&file_to_watch)
     .env("NO_COLOR", "1")
     .stdout(std::process::Stdio::piped())
     .stderr(std::process::Stdio::piped())
     .spawn()
     .expect("failed to spawn script");
 
+  let stdout = child.stdout.as_mut().unwrap();
+  let mut stdout_lines =
+    std::io::BufReader::new(stdout).lines().map(|r| r.unwrap());
   let stderr = child.stderr.as_mut().unwrap();
   let mut stderr_lines =
     std::io::BufReader::new(stderr).lines().map(|r| r.unwrap());
 
   std::thread::sleep(std::time::Duration::from_secs(1));
   assert!(stderr_lines.next().unwrap().contains("error:"));
-  assert!(!child.wait().unwrap().success());
+  assert!(stderr_lines.next().unwrap().contains("Process failed!"));
+
+  // Make sure the watcher actually restarts and works fine with the proper syntax
+  std::fs::write(&file_to_watch, "console.log(42);")
+    .expect("error writing file");
+  std::thread::sleep(std::time::Duration::from_secs(1));
+  assert!(stderr_lines.next().unwrap().contains("Restarting"));
+  assert!(stdout_lines.next().unwrap().contains("42"));
+  wait_for_process_finished("Process", &mut stderr_lines);
+
+  // the watcher process is still alive
+  assert!(child.try_wait().unwrap().is_none());
 
   drop(t);
 }