fs: make FSWatcher.start private

lholmquist · Trott · commit 7eacb7438960 · 2019-10-12T19:56:07.000-07:00
* This is a semver-major change to rename the FSWatcher.start function to FSWatcher._start to make it private The motivation here is that it serves no purpose to the end user. An instance of FSWatcher is returned when a user calls fs.watch, which will call the start method. A user can't create an instance of a FSWatcher directly. If the start method is called by a user it is a noop since the watcher has already started. Calling start after a watcher has closed is also a noop PR-URL: #29905 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
diff --git a/lib/fs.js b/lib/fs.js
@@ -1342,10 +1342,10 @@ function watch(filename, options, listener) {
   if (!watchers)
     watchers = require('internal/fs/watchers');
   const watcher = new watchers.FSWatcher();
-  watcher.start(filename,
-                options.persistent,
-                options.recursive,
-                options.encoding);
+  watcher[watchers.kFSWatchStart](filename,
+                                  options.persistent,
+                                  options.recursive,
+                                  options.encoding);
 
   if (listener) {
     watcher.addListener('change', listener);
diff --git a/lib/internal/fs/watchers.js b/lib/internal/fs/watchers.js
@@ -25,6 +25,8 @@ const assert = require('internal/assert');
 const kOldStatus = Symbol('kOldStatus');
 const kUseBigint = Symbol('kUseBigint');
 
+const kFSWatchStart = Symbol('kFSWatchStart');
+
 function emitStop(self) {
   self.emit('stop');
 }
@@ -135,18 +137,16 @@ function FSWatcher() {
 Object.setPrototypeOf(FSWatcher.prototype, EventEmitter.prototype);
 Object.setPrototypeOf(FSWatcher, EventEmitter);
 
-
-// FIXME(joyeecheung): this method is not documented.
 // At the moment if filename is undefined, we
-// 1. Throw an Error if it's the first time .start() is called
-// 2. Return silently if .start() has already been called
+// 1. Throw an Error if it's the first time Symbol('kFSWatchStart') is called
+// 2. Return silently if Symbol('kFSWatchStart') has already been called
 //    on a valid filename and the wrap has been initialized
 // 3. Return silently if the watcher has already been closed
 // This method is a noop if the watcher has already been started.
-FSWatcher.prototype.start = function(filename,
-                                     persistent,
-                                     recursive,
-                                     encoding) {
+FSWatcher.prototype[kFSWatchStart] = function(filename,
+                                              persistent,
+                                              recursive,
+                                              encoding) {
   if (this._handle === null) {  // closed
     return;
   }
@@ -202,5 +202,6 @@ Object.defineProperty(FSEvent.prototype, 'owner', {
 
 module.exports = {
   FSWatcher,
-  StatWatcher
+  StatWatcher,
+  kFSWatchStart
 };
diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
@@ -57,8 +57,6 @@ for (const testCase of cases) {
   });
   watcher.on('close', common.mustCall(() => {
     watcher.close(); // Closing a closed watcher should be a noop
-    // Starting a closed watcher should be a noop
-    watcher.start();
   }));
   watcher.on('change', common.mustCall(function(eventType, argFilename) {
     if (interval) {
@@ -71,16 +69,11 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    // Starting a started watcher should be a noop
-    watcher.start();
-    watcher.start(pathToWatch);
-
     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
-    watcher.start();  // Starting a closed watcher should be a noop
   }));
 
   // Long content so it's actually flushed. toUpperCase so there's real change.
