Skip to content

Commit

Permalink
fs: make FSWatcher.start private
Browse files Browse the repository at this point in the history
* 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>
  • Loading branch information
lholmquist authored and Trott committed Oct 13, 2019
1 parent 40ef537 commit 7eacb74
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 20 deletions.
8 changes: 4 additions & 4 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
19 changes: 10 additions & 9 deletions lib/internal/fs/watchers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -202,5 +202,6 @@ Object.defineProperty(FSEvent.prototype, 'owner', {

module.exports = {
FSWatcher,
StatWatcher
StatWatcher,
kFSWatchStart
};
7 changes: 0 additions & 7 deletions test/parallel/test-fs-watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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.
Expand Down

0 comments on commit 7eacb74

Please sign in to comment.