From 7eacb7438960da98eaf210e232ea93a98e247b72 Mon Sep 17 00:00:00 2001
From: Lucas Holmquist <lholmqui@redhat.com>
Date: Wed, 9 Oct 2019 08:40:30 -0400
Subject: [PATCH] fs: make FSWatcher.start private

* 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: https://github.com/nodejs/node/pull/29905
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
---
 lib/fs.js                      |  8 ++++----
 lib/internal/fs/watchers.js    | 19 ++++++++++---------
 test/parallel/test-fs-watch.js |  7 -------
 3 files changed, 14 insertions(+), 20 deletions(-)

diff --git a/lib/fs.js b/lib/fs.js
index d5c7ea70d8ec76..1b3df1119f3aa5 100644
--- 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
index bde07cfdc59d41..4dfa83c3b218e2 100644
--- 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
index e596f32a766926..fb674bbfeab4c5 100644
--- 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.