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

Add support for fs.watchFile() #654

Open
humphd opened this issue Jan 3, 2019 · 9 comments
Open

Add support for fs.watchFile() #654

humphd opened this issue Jan 3, 2019 · 9 comments

Comments

@humphd
Copy link
Contributor

humphd commented Jan 3, 2019

https://nodejs.org/api/fs.html#fs_fs_watchfile_filename_options_listener

See also #553 for adding fs.unwatchFile()

@andrewkoung
Copy link
Contributor

@humphd Can I try tackling this issue for release 0.3?

@humphd
Copy link
Contributor Author

humphd commented Mar 13, 2019

@andrewkoung sure, that's fine with me. Do you want to do a bit of reading/studying on your own, and then ask any questions you have in here? I'm happy to mentor you on the work.

@andrewkoung
Copy link
Contributor

I'm going to try studying it first and see what I can figure out and then ask here. Thanks!

@humphd
Copy link
Contributor Author

humphd commented Mar 13, 2019

OK, sounds good. If/when you have questions, feel free to ask them here. Happy to help.

@andrewkoung
Copy link
Contributor

andrewkoung commented Mar 13, 2019

@humphd I did some looking around and the difference between watch() and watchFile() seems to be that watchFile calls stat periodically. Is there a class in this project that has functions to perform stat checking?

@humphd
Copy link
Contributor Author

humphd commented Mar 14, 2019

Right, watchFile will poll instead of waiting on events from the OS. If we decide to do polling, the easiest way would be to start an interval timer with setInterval() and have it call fs.stat() on the file path until unwatchFile() is called, which would then call clearInterval().

I'm not sure what I think about polling in Filer. On the one hand, it makes things pretty lightweight in terms of code; but since we already have infrastructure in place for doing watch events with fs.watch(), it might also make sense to simply have watchFile do the same thing as watch internally.

@modeswitch what are your thoughts here?

@andrewkoung while we figure out which way you should go, why don't you start by writing tests for this?

@andrewkoung
Copy link
Contributor

@humphd No problem about the tests! On the other hand, I've been doing some extreme deep diving these past few days and this is what I have so far... am I heading in the right direction?

`this.watchFile = function(filename, options, listener) {
    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
options = options || { interval: 5007, persistent: true };

listener = listener || nop;
const watcher = new FSWatcher();
watcher.start(filename, false, options.recursive);

setInterval(function() {
  fs.stat(filename, (err, stats) => {
    if(err) {
      console.log(err);
    }
    //if the file changes...
    watcher.on('change', listener);
    //somehow emit the event so that it returns back the prev and curr 
  });
}, 
options.interval
);

return watcher; };

@humphd
Copy link
Contributor Author

humphd commented Mar 18, 2019

@andrewkoung thanks for the update. Here are a few thoughts, reading your code above:

  • The way you're dealing with interval will fail in some cases. I would suggest you create another line that basically does const interval = options.interval || 5007; after you deal with the options object. You can probably ignore persistent for now.
  • The code you have for the watcher can be removed. Instead, we'll use your interval.
  • You will need to create an object that uses filenames as keys, and stores interval return values, so we can use that to clearInterval() in fs.unwatch(path). This object will need to be shared with the fs.unwatch code. Some work started on unwatch in Issue 551-Add unwatchfile method #553, which you might decide to take over.
  • You'll need to store the current and prev result of calling stat. I would declare these above your setInterval call. Then, when you do a stat you will need to compare the two objects, and see if anything has changed. If so, you'll need to call the listener function that was passed to you, passing along the current and prev values.

If you want to open a PR when you do that, I can start commenting directly in the code to help you more.

@andrewkoung
Copy link
Contributor

Ok thanks! I'll give it a try!

andrewkoung added a commit to andrewkoung/filer that referenced this issue Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants