fs: add 'close' event to FSWatcher #19900
Conversation
nodejs-github-bot
added
the
fs
|
A test would be good, yes. |
|
Docs are needed as well. |
|
Why do you need a |
|
@mcollina To keep the function watch(root) {
return fs.watch(root, (evt, file) => {
// ...
}).on('close', () => {
// ...
})
}Currently, I'm monkey-patching the function watch(root) {
let watcher = fs.watch(root, (evt, file) => {
// ...
})
let {close} = watcher
watcher.close = function() {
close.call(watcher)
// ...
}
return watcher
} |
|
@aleclarson definitely better. Can you add a unit test? |
|
Added a test and docs. Note the |
|
@aleclarson the |
test/parallel/test-fs-watch.js
Outdated
| @@ -54,6 +54,10 @@ for (const testCase of cases) { | |||
| } | |||
| assert.fail(err); | |||
| }); | |||
| let closeCount = 0; | |||
There was a problem hiding this comment.
You can drop the closeCount, and just write watcher.on('close', common.mustCall());
There was a problem hiding this comment.
Does that protect against multiple calls?
|
Good to go 👍 |
lib/fs.js
Outdated
| @@ -1387,6 +1387,7 @@ FSWatcher.prototype.close = function() { | |||
| return; | |||
| } | |||
| this._handle.close(); | |||
| this.emit('close'); | |||
There was a problem hiding this comment.
Can you wrap this in a process.nextTick()? Zalgo and all that.
|
Ping @aleclarson |
|
Now wrapped with |
joyeecheung
added
the
semver-minor
|
Landed in 5cc948b. |
PR-URL: #19900 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #19900 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#19900 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Should
process.nextTickwrap this? Are tests necessary here?