Skip to content
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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 33 additions & 24 deletions src/filesystem/interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -207,47 +207,56 @@ function FileSystem(options, callback) {
};

//Object that uses filenames as keys
const statWatchers = new Map();
const statWatchers = new Map();
Copy link
Contributor

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?


this.watchFile = function(filename, options, listener) {
let prevStat, currStat;
this.watchFile = function (filename, options, listener) {
let prevStat, currStat;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably call Path.normalize(filename) on filename before we use it internally.


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;
listener = options;
options = {};
}
// default 5007ms interval, persistent is not used this project
const interval = options.interval || 5007;
listener = listener || nop;

// Stores initial prev value to compare
fs.stat(filename, function(err, stats) {
prevStat = stats;

// Stores interval return values
statWatchers.set(filename, value);

var value = setInterval(function() {
fs.stat(filename, function(err, stats) {
if(err) {
console.log(err);
}
// Store the current stat

let intervalValue = statWatchers.get(filename);

// Checks to see if there's a pre-existing watcher on the file
if (intervalValue === undefined) {
andrewkoung marked this conversation as resolved.
Show resolved Hide resolved
// Stores initial prev value to compare
fs.stat(filename, function (err, stats) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some kind of timing bug here. You do a stat, and then inside the callback you start your interval, but never seem to call stat again in that interval's callback. Each interval should recall stat, so you have updated values for prev/currStat.

Also, this raises the question about the interval's lower bounds. There is some amount of time that a stat will take to execute. We shouldn't trigger another one before it's done. Probably we should ignore requests for interval to be lower than 5007 and just use that as our floor.

var value = setInterval(function () {
prevStat = currStat;

//Conditional check for first run to set initial state for prevStat
if(prevStat === undefined) {
andrewkoung marked this conversation as resolved.
Show resolved Hide resolved
prevStat = stats;
}

currStat = stats;
if(Object.toJSON(prevStat) !== Object.toJSON(currStat)) {

if (err) {
clearInterval(value);
console.warn('[Filer Error] fs.watchFile encountered an error: ' + err);
andrewkoung marked this conversation as resolved.
Show resolved Hide resolved
}
if (JSON.stringify(prevStat) !== JSON.stringify(currStat)) {
listener(prevStat, currStat);
}
// Set a new prevStat based on previous
prevStat = currStat;
});
},
interval
);
});
},
interval
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indents here seem wrong, these 3 lines shouldn't all line up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand Down
25 changes: 14 additions & 11 deletions tests/spec/fs.watchFile.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,27 @@ describe('fs.watchFile', function() {
afterEach(util.cleanup);

it('should be a function', function() {
var fs = util.fs();
const fs = util.fs();
expect(typeof fs.watchFile).to.equal('function');
});

/*
it('should get a change event when writing a file', function(done) {
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();
});

fs.watchFile('/myfile.txt', function(event, filename) {
expect(event).to.equal('change');
expect(filename).to.equal('/myfile.txt');
watcher.close();
done();
});
it('prev and curr should be populated', function() {
const fs = util.fs();

fs.writeFile('/myfile.txt', 'data', function(error) {
fs.writeFile('/myfile', 'data', function(error) {
if(error) throw error;
});

fs.watchFile('/myfile', function(prev, curr) {
expect(prev).to.exist;
expect(curr).to.exist;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do more than just check if the objects exist. We'd expect them to exist, and also to match what a stat object looks like, see https://github.com/filerjs/filer/blob/master/src/stats.js#L10.

Some other tests that I can think of:

  • if you watchFile a file, and then write to it, your listener should get called
  • We'd want to see that the stats (prev vs. curr) do in fact change as the file changes.
  • What happens if we delete a file?
  • Does watchFile work across symlinks?
  • Is the interval value passed by the user honoured in the tests? What if they set another value, do we stick to that timing?

There are more we could do, but at the very least, we need proof that this is working as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've worked on creating some tests for half the points mentioned, but I'm going to have to figure out the logic to test the ones I haven't covered.

});
});
*/
});