-
Notifications
You must be signed in to change notification settings - Fork 155
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #654 working on adding feature fs.watchFile() #748
base: master
Are you sure you want to change the base?
Changes from all commits
58a3a30
0d29206
8a8e421
1c7c001
3fd3fb0
3f746b0
babe89c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,6 +206,60 @@ function FileSystem(options, callback) { | |
return watcher; | ||
}; | ||
|
||
//Object that uses filenames as keys | ||
const statWatchers = new Map(); | ||
|
||
this.watchFile = function (filename, options, listener) { | ||
let prevStat, currStat; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably call |
||
|
||
if (Path.isNull(filename)) { | ||
throw new Error('Path must be a string without null bytes.'); | ||
} | ||
// Checks to see if there were options passed in and if not, the callback function will be set here | ||
if (typeof options === 'function') { | ||
listener = options; | ||
options = {}; | ||
} | ||
// default 5007ms interval, persistent is not used this project | ||
const interval = options.interval || 5007; | ||
listener = listener || nop; | ||
|
||
let intervalValue = statWatchers.get(filename); | ||
|
||
if(intervalValue) { | ||
return; | ||
} | ||
else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No |
||
fs.stat(filename, function (err, stats) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's some kind of timing bug here. You do a Also, this raises the question about the |
||
var value = setInterval(function () { | ||
prevStat = currStat; | ||
|
||
//Conditional check for first run to set initial state for prevStat | ||
if(!prevStat) { | ||
prevStat = stats; | ||
} | ||
|
||
currStat = stats; | ||
|
||
if (err) { | ||
clearInterval(value); | ||
console.warn('[Filer Error] fs.watchFile encountered an error' + err.message); | ||
} | ||
if (JSON.stringify(prevStat) !== JSON.stringify(currStat)) { | ||
listener(prevStat, currStat); | ||
} | ||
// Set a new prevStat based on previous | ||
prevStat = currStat; | ||
}, | ||
interval | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indents here seem wrong, these 3 lines shouldn't all line up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought so too, but I checked the spacings and it looked right, also the linting will fail if I change it. |
||
|
||
// Stores interval return values | ||
statWatchers.set(filename, value); | ||
}); | ||
} | ||
}; | ||
|
||
// Deal with various approaches to node ID creation | ||
function wrappedGuidFn(context) { | ||
return function (callback) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
'use strict'; | ||
|
||
var util = require('../lib/test-utils.js'); | ||
var expect = require('chai').expect; | ||
|
||
describe('fs.watchFile', function() { | ||
beforeEach(util.setup); | ||
afterEach(util.cleanup); | ||
|
||
it('should be a function', function() { | ||
const fs = util.fs(); | ||
expect(typeof fs.watchFile).to.equal('function'); | ||
}); | ||
|
||
it('should throw an error if a file path is not defined', function() { | ||
const fs = util.fs(); | ||
|
||
const fn = () => fs.watchFile(undefined); | ||
expect(fn).to.throw(); | ||
}); | ||
|
||
it('should throw an error if a file is deleted', function() { | ||
const fs = util.fs(); | ||
|
||
fs.writeFile('/myfile', 'data', function(error) { | ||
if(error) throw error; | ||
}); | ||
|
||
fs.watchFile('/myfile', function(prev, curr) { | ||
expect(prev).to.exist; | ||
expect(curr).to.exist; | ||
}); | ||
|
||
fs.unlink('/myfile'); | ||
|
||
const fn = () => fs.watchFile('/myfile'); | ||
expect(fn).to.throw(); | ||
}); | ||
|
||
it('prev and curr should be equal if nothing has been changed in the file', function() { | ||
const fs = util.fs(); | ||
|
||
fs.writeFile('/myfile', 'data', function(error) { | ||
if(error) throw error; | ||
}); | ||
|
||
fs.watchFile('/myfile', function(prev, curr) { | ||
expect(prev).to.equal(curr); | ||
}); | ||
}); | ||
|
||
it('prev and curr should not be equal if something has been changed in the file', function() { | ||
const fs = util.fs(); | ||
|
||
fs.writeFile('/myfile', 'data', function(error) { | ||
if(error) throw error; | ||
}); | ||
|
||
fs.watchFile('/myfile', function(prev, curr) { | ||
expect(prev).to.equal(curr); | ||
|
||
fs.writeFile('/myfile', 'data2', function(error) { | ||
if(error) throw error; | ||
}); | ||
|
||
expect(prev).to.not.equal(curr); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to flip back and forth on this, but I'm not clear what benefit we're gaining from
Map
here over{}
for potential loss in compatibility. Can we switch back to{}
instead?