fix(node/fs): fix fs.watch (#2469)

node/_fs/_fs_watch.ts

@@ -1,7 +1,8 @@
 // Copyright 2018-2022 the Deno authors. All rights reserved. MIT license.
-import { fromFileUrl } from "../path.ts";
+import { basename } from "../path.ts";
 import { EventEmitter } from "../events.ts";
 import { notImplemented } from "../_utils.ts";
+import { getValidatedPath } from "../internal/fs/utils.mjs";
 
 export function asyncIterableIteratorToCallback<T>(
   iterator: AsyncIterableIterator<T>,
@@ -76,26 +77,45 @@ export function watch(
     : typeof optionsOrListener2 === "object"
     ? optionsOrListener2
     : undefined;
-  filename = filename instanceof URL ? fromFileUrl(filename) : filename;
 
-  const iterator = Deno.watchFs(filename, {
-    recursive: options?.recursive || false,
-  });
+  const watchPath = getValidatedPath(filename).toString();
 
-  if (!listener) throw new Error("No callback function supplied");
+  let iterator: Deno.FsWatcher;
+  // Start the actual watcher in the next event loop
+  // to avoid error in compat test case (parallel/test-fs-watch.js)
+  const timer = setTimeout(() => {
+    iterator = Deno.watchFs(watchPath, {
+      recursive: options?.recursive || false,
+    });
+
+    asyncIterableToCallback<Deno.FsEvent>(iterator, (val, done) => {
+      if (done) return;
+      fsWatcher.emit(
+        "change",
+        convertDenoFsEventToNodeFsEvent(val.kind),
+        basename(val.paths[0]),
+      );
+    }, (e) => {
+      fsWatcher.emit("error", e);
+    });
+  }, 0);
 
   const fsWatcher = new FSWatcher(() => {
-    if (iterator.return) iterator.return();
+    clearTimeout(timer);
+    try {
+      iterator?.close();
+    } catch (e) {
+      if (e instanceof Deno.errors.BadResource) {
+        // already closed
+        return;
+      }
+      throw e;
+    }
   });
 
-  fsWatcher.on("change", listener);
-
-  asyncIterableToCallback<Deno.FsEvent>(iterator, (val, done) => {
-    if (done) return;
-    fsWatcher.emit("change", val.kind, val.paths[0]);
-  }, (e) => {
-    fsWatcher.emit("error", e);
-  });
+  if (listener) {
+    fsWatcher.on("change", listener);
+  }
 
   return fsWatcher;
 }
@@ -103,10 +123,19 @@ export function watch(
 export { watch as watchFile };
 
 class FSWatcher extends EventEmitter {
-  close: () => void;
+  #closer: () => void;
+  #closed = false;
   constructor(closer: () => void) {
     super();
-    this.close = closer;
+    this.#closer = closer;
+  }
+  close() {
+    if (this.#closed) {
+      return;
+    }
+    this.#closed = true;
+    this.emit("close");
+    this.#closer();
   }
   ref() {
     notImplemented("FSWatcher.ref() is not implemented");
@@ -115,3 +144,15 @@ class FSWatcher extends EventEmitter {
     notImplemented("FSWatcher.unref() is not implemented");
   }
 }
+
+type NodeFsEventType = "rename" | "change";
+
+function convertDenoFsEventToNodeFsEvent(
+  kind: Deno.FsEvent["kind"],
+): NodeFsEventType {
+  if (kind === "create" || kind === "remove") {
+    return "rename";
+  } else {
+    return "change";
+  }
+}

node/_tools/config.json

@@ -280,6 +280,7 @@
       "test-fs-rmdir-recursive-warns-on-file.js",
       "test-fs-rmdir-recursive.js",
       "test-fs-rmdir-type-check.js",
+      "test-fs-watch.js",
       "test-fs-write-buffer.js",
       "test-fs-write-file-buffer.js",
       "test-fs-write-file-invalid-path.js",

node/_tools/test/parallel/test-fs-watch.js

@@ -0,0 +1,105 @@
+// deno-fmt-ignore-file
+// deno-lint-ignore-file
+
+// Copyright Joyent and Node contributors. All rights reserved. MIT license.
+// Taken from Node 16.13.0
+// This file is automatically generated by "node/_tools/setup.ts". Do not modify this file manually
+
+'use strict';
+const common = require('../common');
+
+if (common.isIBMi)
+  common.skip('IBMi does not support `fs.watch()`');
+
+// Tests if `filename` is provided to watcher on supported platforms
+
+const fs = require('fs');
+const assert = require('assert');
+const { join } = require('path');
+
+class WatchTestCase {
+  constructor(shouldInclude, dirName, fileName, field) {
+    this.dirName = dirName;
+    this.fileName = fileName;
+    this.field = field;
+    this.shouldSkip = !shouldInclude;
+  }
+  get dirPath() { return join(tmpdir.path, this.dirName); }
+  get filePath() { return join(this.dirPath, this.fileName); }
+}
+
+const cases = [
+  // Watch on a directory should callback with a filename on supported systems
+  new WatchTestCase(
+    common.isLinux || common.isOSX || common.isWindows || common.isAIX,
+    'watch1',
+    'foo',
+    'filePath'
+  ),
+  // Watch on a file should callback with a filename on supported systems
+  new WatchTestCase(
+    common.isLinux || common.isOSX || common.isWindows,
+    'watch2',
+    'bar',
+    'dirPath'
+  ),
+];
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+for (const testCase of cases) {
+  if (testCase.shouldSkip) continue;
+  fs.mkdirSync(testCase.dirPath);
+  // Long content so it's actually flushed.
+  const content1 = Date.now() + testCase.fileName.toLowerCase().repeat(1e4);
+  fs.writeFileSync(testCase.filePath, content1);
+
+  let interval;
+  const pathToWatch = testCase[testCase.field];
+  const watcher = fs.watch(pathToWatch);
+  watcher.on('error', (err) => {
+    if (interval) {
+      clearInterval(interval);
+      interval = null;
+    }
+    assert.fail(err);
+  });
+  watcher.on('close', common.mustCall(() => {
+    watcher.close(); // Closing a closed watcher should be a noop
+  }));
+  watcher.on('change', common.mustCall(function(eventType, argFilename) {
+    if (interval) {
+      clearInterval(interval);
+      interval = null;
+    }
+    if (common.isOSX)
+      assert.strictEqual(['rename', 'change'].includes(eventType), true);
+    else
+      assert.strictEqual(eventType, 'change');
+    assert.strictEqual(argFilename, testCase.fileName);
+
+    watcher.close();
+
+    // We document that watchers cannot be used anymore when it's closed,
+    // here we turn the methods into noops instead of throwing
+    watcher.close(); // Closing a closed watcher should be a noop
+  }));
+
+  // Long content so it's actually flushed. toUpperCase so there's real change.
+  const content2 = Date.now() + testCase.fileName.toUpperCase().repeat(1e4);
+  interval = setInterval(() => {
+    fs.writeFileSync(testCase.filePath, '');
+    fs.writeFileSync(testCase.filePath, content2);
+  }, 100);
+}
+
+[false, 1, {}, [], null, undefined].forEach((input) => {
+  assert.throws(
+    () => fs.watch(input, common.mustNotCall()),
+    {
+      code: 'ERR_INVALID_ARG_TYPE',
+      name: 'TypeError'
+    }
+  );
+});