From 62ec12703f7869e2481b5289f42b4fb56a61b71a Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 9 Oct 2019 08:40:30 -0400 Subject: [PATCH 1/2] 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 --- lib/fs.js | 8 ++++---- lib/internal/fs/watchers.js | 14 ++++++-------- test/parallel/test-fs-watch.js | 8 ++++---- 3 files changed, 14 insertions(+), 16 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 34517a17dab484..20a8059f0a028b 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._start(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..33699f78792a40 100644 --- a/lib/internal/fs/watchers.js +++ b/lib/internal/fs/watchers.js @@ -135,18 +135,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 ._start() is called +// 2. Return silently if ._start() 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._start = function(filename, + persistent, + recursive, + encoding) { if (this._handle === null) { // closed return; } diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js index e596f32a766926..470ae2f447756f 100644 --- a/test/parallel/test-fs-watch.js +++ b/test/parallel/test-fs-watch.js @@ -58,7 +58,7 @@ 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._start(); })); watcher.on('change', common.mustCall(function(eventType, argFilename) { if (interval) { @@ -72,15 +72,15 @@ for (const testCase of cases) { assert.strictEqual(argFilename, testCase.fileName); // Starting a started watcher should be a noop - watcher.start(); - watcher.start(pathToWatch); + 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 + watcher._start(); // Starting a closed watcher should be a noop })); // Long content so it's actually flushed. toUpperCase so there's real change. From 984fa00a342bf22bf8583eafe9ca64d5c7892305 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 9 Oct 2019 10:53:22 -0400 Subject: [PATCH 2/2] squash: use symbols for private --- lib/fs.js | 8 ++++---- lib/internal/fs/watchers.js | 17 ++++++++++------- test/parallel/test-fs-watch.js | 7 ------- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 20a8059f0a028b..a57e97d69ad889 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 33699f78792a40..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'); } @@ -136,15 +138,15 @@ Object.setPrototypeOf(FSWatcher.prototype, EventEmitter.prototype); Object.setPrototypeOf(FSWatcher, EventEmitter); // 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; } @@ -200,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 470ae2f447756f..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.