fix(cli): lint/format all discoverd files on each change (#12518)

cli/tests/integration/watcher_tests.rs

@@ -55,6 +55,10 @@ fn wait_for(s: &str, lines: &mut impl Iterator<Item = String>) {
   }
 }
 
+fn read_line(s: &str, lines: &mut impl Iterator<Item = String>) -> String {
+  lines.find(|m| m.contains(s)).unwrap()
+}
+
 fn check_alive_then_kill(mut child: std::process::Child) {
   assert!(child.try_wait().unwrap().is_none());
   child.kill().unwrap();
@@ -77,6 +81,8 @@ fn lint_watch_test() {
   let t = TempDir::new().expect("tempdir fail");
   let badly_linted_original =
     util::testdata_path().join("lint/watch/badly_linted.js");
+  let badly_linted_output =
+    util::testdata_path().join("lint/watch/badly_linted.js.out");
   let badly_linted_fixed1 =
     util::testdata_path().join("lint/watch/badly_linted_fixed1.js");
   let badly_linted_fixed1_output =
@@ -86,8 +92,6 @@ fn lint_watch_test() {
   let badly_linted_fixed2_output =
     util::testdata_path().join("lint/watch/badly_linted_fixed2.js.out");
   let badly_linted = t.path().join("badly_linted.js");
-  let badly_linted_output =
-    util::testdata_path().join("lint/watch/badly_linted.js.out");
 
   std::fs::copy(&badly_linted_original, &badly_linted)
     .expect("Failed to copy file");
@@ -139,6 +143,54 @@ fn lint_watch_test() {
   drop(t);
 }
 
+#[test]
+fn lint_all_files_on_each_change_test() {
+  let t = TempDir::new().expect("tempdir fail");
+  let badly_linted_fixed0 =
+    util::testdata_path().join("lint/watch/badly_linted.js");
+  let badly_linted_fixed1 =
+    util::testdata_path().join("lint/watch/badly_linted_fixed1.js");
+  let badly_linted_fixed2 =
+    util::testdata_path().join("lint/watch/badly_linted_fixed2.js");
+
+  let badly_linted_1 = t.path().join("badly_linted_1.js");
+  let badly_linted_2 = t.path().join("badly_linted_2.js");
+  std::fs::copy(&badly_linted_fixed0, &badly_linted_1)
+    .expect("Failed to copy file");
+  std::fs::copy(&badly_linted_fixed1, &badly_linted_2)
+    .expect("Failed to copy file");
+
+  let mut child = util::deno_cmd()
+    .current_dir(util::testdata_path())
+    .arg("lint")
+    .arg(&t.path())
+    .arg("--watch")
+    .arg("--unstable")
+    .stdout(std::process::Stdio::piped())
+    .stderr(std::process::Stdio::piped())
+    .spawn()
+    .expect("Failed to spawn script");
+  let mut stderr = child.stderr.as_mut().unwrap();
+  let mut stderr_lines = std::io::BufReader::new(&mut stderr)
+    .lines()
+    .map(|r| r.unwrap());
+
+  std::thread::sleep(std::time::Duration::from_secs(1));
+
+  assert_contains!(read_line("Checked", &mut stderr_lines), "Checked 2 files");
+
+  std::fs::copy(&badly_linted_fixed2, &badly_linted_2)
+    .expect("Failed to copy file");
+  std::thread::sleep(std::time::Duration::from_secs(1));
+
+  assert_contains!(read_line("Checked", &mut stderr_lines), "Checked 2 files");
+
+  assert!(child.try_wait().unwrap().is_none());
+
+  child.kill().unwrap();
+  drop(t);
+}
+
 #[test]
 fn fmt_watch_test() {
   let t = TempDir::new().unwrap();
@@ -180,6 +232,50 @@ fn fmt_watch_test() {
   check_alive_then_kill(child);
 }
 
+#[test]
+fn fmt_check_all_files_on_each_change_test() {
+  let t = TempDir::new().unwrap();
+  let badly_formatted_original =
+    util::testdata_path().join("badly_formatted.mjs");
+  let badly_formatted_1 = t.path().join("badly_formatted_1.js");
+  let badly_formatted_2 = t.path().join("badly_formatted_2.js");
+  std::fs::copy(&badly_formatted_original, &badly_formatted_1).unwrap();
+  std::fs::copy(&badly_formatted_original, &badly_formatted_2).unwrap();
+
+  let mut child = util::deno_cmd()
+    .current_dir(util::testdata_path())
+    .arg("fmt")
+    .arg(&t.path())
+    .arg("--watch")
+    .arg("--check")
+    .arg("--unstable")
+    .stdout(std::process::Stdio::piped())
+    .stderr(std::process::Stdio::piped())
+    .spawn()
+    .unwrap();
+  let (_stdout_lines, mut stderr_lines) = child_lines(&mut child);
+
+  // TODO(lucacasonato): remove this timeout. It seems to be needed on Linux.
+  std::thread::sleep(std::time::Duration::from_secs(1));
+
+  assert_contains!(
+    read_line("error", &mut stderr_lines),
+    "Found 2 not formatted files in 2 files"
+  );
+
+  // Change content of the file again to be badly formatted
+  std::fs::copy(&badly_formatted_original, &badly_formatted_1).unwrap();
+
+  std::thread::sleep(std::time::Duration::from_secs(1));
+
+  assert_contains!(
+    read_line("error", &mut stderr_lines),
+    "Found 2 not formatted files in 2 files"
+  );
+
+  check_alive_then_kill(child);
+}
+
 #[test]
 fn bundle_js_watch() {
   use std::path::PathBuf;

cli/tools/fmt.rs

@@ -81,25 +81,36 @@ pub async fn format(
 
   let resolver = |changed: Option<Vec<PathBuf>>| {
     let files_changed = changed.is_some();
-    let result =
+
-      collect_files(&include_files, &exclude_files, is_supported_ext_fmt).map(
+    let collect_files =
-        |files| {
+      collect_files(&include_files, &exclude_files, is_supported_ext_fmt);
-          let collected_files = if let Some(paths) = changed {
+
-            files
+    let (result, should_refmt) = match collect_files {
+      Ok(value) => {
+        if let Some(paths) = changed {
+          let refmt_files = value
+            .clone()
             .into_iter()
             .filter(|path| paths.contains(path))
-              .collect::<Vec<_>>()
+            .collect::<Vec<_>>();
+
+          let should_refmt = !refmt_files.is_empty();
+
+          if check {
+            (Ok((value, fmt_options.clone())), Some(should_refmt))
           } else {
-            files
+            (Ok((refmt_files, fmt_options.clone())), Some(should_refmt))
+          }
+        } else {
+          (Ok((value, fmt_options.clone())), None)
+        }
+      }
+      Err(e) => (Err(e), None),
     };
-          (collected_files, fmt_options.clone())
+
-        },
-      );
     let paths_to_watch = include_files.clone();
     async move {
-      if (files_changed || !watch)
+      if files_changed && matches!(should_refmt, Some(false)) {
-        && matches!(result, Ok((ref files, _)) if files.is_empty())
-      {
         ResolutionResult::Ignore
       } else {
         ResolutionResult::Restart {
@@ -121,13 +132,16 @@ pub async fn format(
   if watch {
     file_watcher::watch_func(resolver, operation, "Fmt").await?;
   } else {
-    let (files, fmt_options) =
+    let files =
-      if let ResolutionResult::Restart { result, .. } = resolver(None).await {
+      collect_files(&include_files, &exclude_files, is_supported_ext_fmt)
-        result?
+        .and_then(|files| {
+          if files.is_empty() {
+            Err(generic_error("No target files found."))
           } else {
-        return Err(generic_error("No target files found."));
+            Ok(files)
-      };
+          }
-    operation((files, fmt_options)).await?;
+        })?;
+    operation((files, fmt_options.clone())).await?;
   }
 
   Ok(())

cli/tools/lint.rs

@@ -105,26 +105,27 @@ pub async fn lint(
 
   let resolver = |changed: Option<Vec<PathBuf>>| {
     let files_changed = changed.is_some();
-    let result = collect_files(
+    let collect_files =
-      &*include_files.clone(),
+      collect_files(&include_files, &exclude_files, is_supported_ext);
-      &*exclude_files.clone(),
+
-      is_supported_ext,
+    let paths_to_watch = include_files.clone();
-    )
+
-    .map(|files| {
+    let (result, should_relint) = match collect_files {
+      Ok(value) => {
         if let Some(paths) = changed {
-        files
+          (
-          .into_iter()
+            Ok(value.clone()),
-          .filter(|path| paths.contains(path))
+            Some(value.iter().any(|path| paths.contains(path))),
-          .collect::<Vec<_>>()
+          )
         } else {
-        files
+          (Ok(value), None)
         }
-    });
+      }
-    let paths_to_watch = args.clone();
+      Err(e) => (Err(e), None),
+    };
+
     async move {
-      if (files_changed || !watch)
+      if files_changed && matches!(should_relint, Some(false)) {
-        && matches!(result, Ok(ref files) if files.is_empty())
-      {
         ResolutionResult::Ignore
       } else {
         ResolutionResult::Restart {