Issue #654 working on adding feature fs.watchFile() #748

andrewkoung · 2019-03-20T01:50:25Z

Just putting a PR up of my current progress. Feel free to let me know if there could be improvements as I go.

humphd

Did a read through of this, and left a bunch of comments. Nice working getting going on this so quickly. Let me know if what I wrote brings up more questions for you.

src/filesystem/interface.js

humphd · 2019-03-21T16:56:38Z

src/filesystem/interface.js

+    fs.stat(filename, function(err, stats) { prevStat = stats}); 
+
+    //stores interval return values
+    statWatchers.set(filename, value);


Question: what do we do if there is already a watcher in here for this filename? Probably we need an array of intervals for each filename vs. assuming it will only be one? That will complicate removing it. Need to ponder this.

If there's a watcher already associated with the filename, maybe it shouldn't it be ignored. There would be a setInterval() that's associated with that filename won't there be?

humphd · 2019-03-21T16:56:57Z

src/filesystem/interface.js

+    var value = setInterval(function() {
+      fs.stat(filename, function(err, stats) {
+        if(err) {
+          console.log(err);


This isn't good enough. We'll have to deal with it.

Any errors handling in nodejs seems to be happening in the highlighted area

I tried to analyze the start method, but it seems to be too complex for me with the terminology being used here, but I believe the highlighted portion below is what's important.

Node is able to throw the error here because they are doing the initial stat synchronously, so the ultimate recipient of the throw would be the caller of fs.watchFile. We can only do asynchronous calls in Filer and the Browser (i.e., we can't block on access to IndexedDB), so throwing doesn't make as much sense for us.

Another problem I see is that node is returning the stat object it gets back from that synchronous call. We also can't do this.

I'm not 100% sure what the right approach is here. Perhaps we need to kill the interval and have the watcher just become a no-op if there is an error? Maybe logging an error to the console in addition does make sense. Perhaps you could do:

console.warn([Filer Error] fs.watchFile encountered an error with ${path}: ${err.message})

humphd · 2019-03-21T16:58:27Z

src/filesystem/interface.js

+          console.log(err);
+        }
+        //Store the current stat
+        currStat = stats;


I think this is happening too early. Do you not need:

Move currStat to prevStat

Move stats to currStat

Diff the inner values of the objects (not reference equality)

I thought the order makes sense in this case because initial prevStat and currStat are undefined, so I set an initial stat value to prevStat. In which I then set the stats coming from fs.stat to currStat since it's undefined. In which does a compare and then sets current to prev.

Could be. Let's circle back to this when you get deeper into it.

src/filesystem/interface.js

humphd · 2019-03-21T16:59:59Z

tests/spec/fs.watchFile.spec.js

+  it('should get a change event when writing a file', function(done) {
+    const fs = util.fs();
+
+    const watcher = fs.watchFile('/myfile.txt', function(event, filename) {


watchFile doesn't return a value, so you can drop const watcher =

Question: what does node.js do when you watchFile a file that doesn't exist yet? Your code above will break when you do the initial stat, and it gets an ENOENT error.

Node attempts to receive the watcher using this line here stat = statWatchers.get(filename);. If there is no value associated with the key, it'll return undefined. It does a conditional check to see if the stat variable is undefined and create a new watcher and then assign a value to the key. Whether the stat variable was undefined or defined, it lastly attaches a listener stat.addListener('change', listener);.

So we have a choice to make in this case: we either duplicate node's use of stat on an internval, or we alter the way watchFile works internally to just call into watch, which uses event based monitoring vs. polling. The upside to the latter is that it already works.

…o be discussed

codecov-io · 2019-03-25T04:12:55Z

Codecov Report

Merging #748 into master will decrease coverage by 17.72%.
The diff coverage is 88%.

@@             Coverage Diff             @@
##           master     #748       +/-   ##
===========================================
- Coverage   86.88%   69.16%   -17.73%     
===========================================
  Files          16       16               
  Lines        1739     1764       +25     
===========================================
- Hits         1511     1220      -291     
- Misses        228      544      +316

Impacted Files	Coverage Δ
src/filesystem/interface.js	`84.12% <88%> (-9.17%)`	⬇️
src/shell/shell.js	`5.83% <0%> (-82.5%)`	⬇️
src/shell/environment.js	`22.22% <0%> (-77.78%)`	⬇️
src/fs-watcher.js	`23.07% <0%> (-69.24%)`	⬇️
src/filesystem/implementation.js	`78.69% <0%> (-5.44%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aae538...babe89c. Read the comment docs.

src/filesystem/interface.js

humphd · 2019-03-28T14:42:30Z

tests/spec/fs.watchFile.spec.js

+    expect(typeof fs.watchFile).to.equal('function');
+  });
+
+  /*


We need a bunch more tests here, can you expand this please?

humphd

Left some more feedback. We also need to update the README to add the docs for this method. Follow the example of other methods on that page, in terms of what's needed.

src/filesystem/interface.js

humphd · 2019-03-29T15:54:58Z

src/filesystem/interface.js

-    }); 
+        },
+        interval
+        );


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

I thought so too, but I checked the spacings and it looked right, also the linting will fail if I change it.

humphd · 2019-03-29T16:12:25Z

tests/spec/fs.watchFile.spec.js

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


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.

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.

humphd

I haven't had enough time to play with this extensively yet, but I've done another pass over the code. I still think the tests need to be fleshed out more. We should be doing more checks on the actual contents of prev/curr to see what they contain, and if the values make sense based on what we've done with the files.

I've also left some other notes.

humphd · 2019-04-16T18:45:21Z

src/filesystem/interface.js

+    if(intervalValue) {
+      return;
+    }
+    else { 


No else after return. Get rid of this and unindent the code below.

humphd · 2019-04-16T18:47:26Z

src/filesystem/interface.js

@@ -206,6 +206,60 @@ function FileSystem(options, callback) {
    return watcher;
  };

+  //Object that uses filenames as keys
+  const statWatchers = new Map();


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?

humphd · 2019-04-16T18:50:08Z

src/filesystem/interface.js

+  const statWatchers = new Map();
+
+  this.watchFile = function (filename, options, listener) {
+    let prevStat, currStat;


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

humphd · 2019-04-16T18:54:25Z

src/filesystem/interface.js

+      return;
+    }
+    else { 
+      fs.stat(filename, function (err, stats) {


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.

andrewkoung added 3 commits March 19, 2019 21:49

Issue filerjs#654 working on adding feature fs.watchFile()

58a3a30

added more logic

0d29206

made changes

8a8e421

humphd requested changes Mar 21, 2019

View reviewed changes

andrewkoung added 2 commits March 25, 2019 00:05

partially made some requested changes, but other changes still need t…

1c7c001

…o be discussed

removed outddated test methods

3fd3fb0

humphd requested changes Mar 28, 2019

View reviewed changes

rework logic and added some test scripts

3f746b0

humphd requested changes Mar 29, 2019

View reviewed changes

made syntax changes, updated readme, and added more tests

babe89c

humphd requested changes Apr 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #654 working on adding feature fs.watchFile() #748

Issue #654 working on adding feature fs.watchFile() #748

andrewkoung commented Mar 20, 2019

humphd left a comment

humphd Mar 21, 2019

andrewkoung Mar 22, 2019

humphd Mar 21, 2019

andrewkoung Mar 28, 2019

humphd Mar 28, 2019

humphd Mar 21, 2019

andrewkoung Mar 21, 2019

humphd Mar 21, 2019

humphd Mar 21, 2019

humphd Mar 21, 2019

andrewkoung Mar 22, 2019

humphd Mar 28, 2019

codecov-io commented Mar 25, 2019 •

edited

Loading

humphd Mar 28, 2019

humphd left a comment

humphd Mar 29, 2019

andrewkoung Mar 29, 2019

humphd Mar 29, 2019

andrewkoung Mar 29, 2019

humphd left a comment

humphd Apr 16, 2019

humphd Apr 16, 2019

humphd Apr 16, 2019

humphd Apr 16, 2019

Issue #654 working on adding feature fs.watchFile() #748

Are you sure you want to change the base?

Issue #654 working on adding feature fs.watchFile() #748

Conversation

andrewkoung commented Mar 20, 2019

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 25, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humphd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 25, 2019 •

edited

Loading