From 301f6cc553bd73cfa345ae7de6ee81655efc57d0 Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Wed, 14 Mar 2018 18:55:43 +0800
Subject: [PATCH] fs: remove watcher state errors for fs.watch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- Remove ERR_FS_WATCHER_ALREADY_STARTED and
  ERR_FS_WATCHER_NOT_STARTED because those two situations should
  result in noop instead of errors for consistency with the
  documented behavior of fs.watchFile.
  This partially reverts https://github.com/nodejs/node/pull/19089
- Update comments about this behavior.

Refs: https://github.com/nodejs/node/pull/19089

PR-URL: https://github.com/nodejs/node/pull/19345
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
---
 doc/api/errors.md              | 12 ------------
 lib/fs.js                      | 20 ++++++++++----------
 lib/internal/errors.js         |  4 ----
 test/parallel/test-fs-watch.js | 10 ++--------
 4 files changed, 12 insertions(+), 34 deletions(-)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index f5190dc52bf..5c38dde4130 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -783,18 +783,6 @@ falsy value.
 An invalid symlink type was passed to the [`fs.symlink()`][] or
 [`fs.symlinkSync()`][] methods.
 
-<a id="ERR_FS_WATCHER_ALREADY_STARTED"></a>
-### ERR_FS_WATCHER_ALREADY_STARTED
-
-An attempt was made to start a watcher returned by `fs.watch()` that has
-already been started.
-
-<a id="ERR_FS_WATCHER_NOT_STARTED"></a>
-### ERR_FS_WATCHER_NOT_STARTED
-
-An attempt was made to initiate operations on a watcher returned by
-`fs.watch()` that has not yet been started.
-
 <a id="ERR_HTTP_HEADERS_SENT"></a>
 ### ERR_HTTP_HEADERS_SENT
 
diff --git a/lib/fs.js b/lib/fs.js
index 352d2fd1ffd..b4c2d5cdbb1 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -36,8 +36,6 @@ const fs = exports;
 const { Buffer } = require('buffer');
 const errors = require('internal/errors');
 const {
-  ERR_FS_WATCHER_ALREADY_STARTED,
-  ERR_FS_WATCHER_NOT_STARTED,
   ERR_INVALID_ARG_TYPE,
   ERR_INVALID_CALLBACK,
   ERR_OUT_OF_RANGE
@@ -1342,25 +1340,26 @@ util.inherits(FSWatcher, EventEmitter);
 
 // FIXME(joyeecheung): this method is not documented.
 // At the moment if filename is undefined, we
-// 1. Throw an Error from C++ land if it's the first time .start() is called
-// 2. Return silently from C++ land if .start() has already been called
+// 1. Throw an Error if it's the first time .start() is called
+// 2. Return silently if .start() has already been called
 //    on a valid filename and the wrap has been initialized
+// This method is a noop if the watcher has already been started.
 FSWatcher.prototype.start = function(filename,
                                      persistent,
                                      recursive,
                                      encoding) {
   lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
   if (this._handle.initialized) {
-    throw new ERR_FS_WATCHER_ALREADY_STARTED();
+    return;
   }
 
   filename = getPathFromURL(filename);
   validatePath(filename, 'filename');
 
-  var err = this._handle.start(pathModule.toNamespacedPath(filename),
-                               persistent,
-                               recursive,
-                               encoding);
+  const err = this._handle.start(pathModule.toNamespacedPath(filename),
+                                 persistent,
+                                 recursive,
+                                 encoding);
   if (err) {
     const error = errors.uvException({
       errno: err,
@@ -1372,10 +1371,11 @@ FSWatcher.prototype.start = function(filename,
   }
 };
 
+// This method is a noop if the watcher has not been started.
 FSWatcher.prototype.close = function() {
   lazyAssert()(this._handle instanceof FSEvent, 'handle must be a FSEvent');
   if (!this._handle.initialized) {
-    throw new ERR_FS_WATCHER_NOT_STARTED();
+    return;
   }
   this._handle.close();
 };
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 3d0b6cf9b39..d5260bd501f 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -654,10 +654,6 @@ E('ERR_FALSY_VALUE_REJECTION', 'Promise was rejected with falsy value', Error);
 E('ERR_FS_INVALID_SYMLINK_TYPE',
   'Symlink type must be one of "dir", "file", or "junction". Received "%s"',
   Error); // Switch to TypeError. The current implementation does not seem right
-E('ERR_FS_WATCHER_ALREADY_STARTED',
-  'The watcher has already been started', Error);
-E('ERR_FS_WATCHER_NOT_STARTED',
-  'The watcher has not been started', Error);
 E('ERR_HTTP2_ALTSVC_INVALID_ORIGIN',
   'HTTP/2 ALTSVC frames require a valid origin', TypeError);
 E('ERR_HTTP2_ALTSVC_LENGTH',
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
index f980363b9fd..24600b79ceb 100644
--- a/test/parallel/test-fs-watch.js
+++ b/test/parallel/test-fs-watch.js
@@ -65,16 +65,10 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    common.expectsError(() => watcher.start(), {
-      code: 'ERR_FS_WATCHER_ALREADY_STARTED',
-      message: 'The watcher has already been started'
-    });
+    watcher.start();  // starting a started watcher should be a noop
     // end of test case
     watcher.close();
-    common.expectsError(() => watcher.close(), {
-      code: 'ERR_FS_WATCHER_NOT_STARTED',
-      message: 'The watcher has not been started'
-    });
+    watcher.close(); // closing a closed watcher should be a noop
   }));
 
   // long content so it's actually flushed. toUpperCase so there's real change.