From 14354a0ba6c96421bac5f9d33b6aa85df53be516 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Wed, 1 Jul 2015 08:14:52 -0700 Subject: [PATCH 1/2] fs: fix error on bad listener type When the listener was truthy but NOT a function, fs.watchFile would throw an error through the EventEmitter. This caused a problem because it would only be thrown after the listener was started, which left the listener on. There should be no backwards compatability issues because the error was always thrown, just in a different manner. Also adds tests for this and other basic functionality. --- lib/fs.js | 15 +++++++-------- test/parallel/test-fs-watchfile.js | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-) create mode 100644 test/parallel/test-fs-watchfile.js diff --git a/lib/fs.js b/lib/fs.js index 58704e529747b2..9c13a82c41e5b1 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1305,13 +1305,12 @@ StatWatcher.prototype.stop = function() { const statWatchers = new Map(); -fs.watchFile = function(filename) { +fs.watchFile = function(filename, options, listener) { nullCheck(filename); filename = pathModule.resolve(filename); var stat; - var listener; - var options = { + var defaults = { // Poll interval in milliseconds. 5007 is what libev used to use. It's // a little on the slow side but let's stick with it for now to keep // behavioral changes to a minimum. @@ -1319,14 +1318,14 @@ fs.watchFile = function(filename) { persistent: true }; - if (arguments[1] !== null && typeof arguments[1] === 'object') { - options = util._extend(options, arguments[1]); - listener = arguments[2]; + if (options !== null && typeof options === 'object') { + options = util._extend(defaults, options); } else { - listener = arguments[1]; + listener = options; + options = defaults; } - if (!listener) { + if (typeof listener !== 'function') { throw new Error('watchFile requires a listener function'); } diff --git a/test/parallel/test-fs-watchfile.js b/test/parallel/test-fs-watchfile.js new file mode 100644 index 00000000000000..a64858ce0f7154 --- /dev/null +++ b/test/parallel/test-fs-watchfile.js @@ -0,0 +1,17 @@ +'use strict'; + +const fs = require('fs'); +const assert = require('assert'); + +// Basic usage tests. +assert.throws(function() { + fs.watchFile('./some-file'); +}, /watchFile requires a listener function/); + +assert.throws(function() { + fs.watchFile('./another-file', {}, 'bad listener'); +}, /watchFile requires a listener function/); + +assert.throws(function() { + fs.watchFile(new Object(), function() {}); +}, /Path must be a string/); From f6467b0c7fd6fe3670101c08bf92f4e3ffc68599 Mon Sep 17 00:00:00 2001 From: Brendan Ashworth Date: Wed, 1 Jul 2015 08:13:54 -0700 Subject: [PATCH 2/2] doc: document fs.watchFile behaviour on ENOENT When fs.watchFile encounters an ENOENT error, it invokes the given callback with some error data. This caused an issue as it was different behaviour than Node v0.10. Instead of changing this behaviour, document it and add a test. Ref: https://github.com/nodejs/io.js/issues/1745 Ref: https://github.com/nodejs/io.js/pull/2028 --- doc/api/fs.markdown | 3 +++ test/parallel/test-fs-watchfile.js | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown index bb90cc8d2e3f4e..985a4e4c967a0f 100644 --- a/doc/api/fs.markdown +++ b/doc/api/fs.markdown @@ -576,6 +576,9 @@ These stat objects are instances of `fs.Stat`. If you want to be notified when the file was modified, not just accessed you need to compare `curr.mtime` and `prev.mtime`. +_Note: when an `fs.watchFile` operation results in an `ENOENT` error, it will +invoke the callback once. This is a change in functionality since v0.10._ + _Note: `fs.watch` is more efficient than `fs.watchFile` and `fs.unwatchFile`. `fs.watch` should be used instead of `fs.watchFile` and `fs.unwatchFile` when possible._ diff --git a/test/parallel/test-fs-watchfile.js b/test/parallel/test-fs-watchfile.js index a64858ce0f7154..eacb2f9d821982 100644 --- a/test/parallel/test-fs-watchfile.js +++ b/test/parallel/test-fs-watchfile.js @@ -1,7 +1,10 @@ 'use strict'; const fs = require('fs'); +const path = require('path'); const assert = require('assert'); +const common = require('../common'); +const fixtures = path.join(__dirname, '..', 'fixtures'); // Basic usage tests. assert.throws(function() { @@ -15,3 +18,9 @@ assert.throws(function() { assert.throws(function() { fs.watchFile(new Object(), function() {}); }, /Path must be a string/); + +// Test ENOENT. Should fire once. +const enoentFile = path.join(fixtures, 'empty', 'non-existent-file'); +fs.watchFile(enoentFile, common.mustCall(function(curr, prev) { + fs.unwatchFile(enoentFile); +}));