search

fgnass / filewatcher

Wrapper around fs.watch that falls back to fs.watchFile
54 stars 8 forks source link

wip: remove sync calls #2

Closed juliangruber closed 8 years ago

juliangruber commented 8 years ago

This is a WIP pull request for removing those *Sync calls. Unless clearly stated, it's better for a node program - and more expected of it - not to perform any synchronous calls.

The tests are currently failing because .removeAll() depends on .list() to always return all the watchers, however with the async implementation there's a moment in time where a file is still being watched but its watcher isn't inside the watchers array.

This change is Reviewable

fgnass commented 8 years ago

💯 thanks for starting this!

okdistribute commented 8 years ago

would list also have to be async, then?

fgnass commented 8 years ago

Removing https://github.com/juliangruber/filewatcher/blob/master/index.js#L46-L51 makes the tests pass. That workaround re-adds the directory from the previous test, letting the next one fail.

I merged this into https://github.com/fgnass/filewatcher/tree/remove-sync and added fix that makes the tests pass again. I added you @juliangruber as collaborator in case you would like to help out landing this in master and cutting a new release.

juliangruber commented 8 years ago

i was thinking we should keep lines 46-51 but instead have another means of checking whether a file is already being watched, like watchers[file] = { watcher: watcher }, so if you check !!watchers[file] it tells you whether the file is being watched, but !!watchers[file].watcher tells you whether also a watcher is set up.

fgnass commented 8 years ago

In my fix the lines are still there. They just no longer use the public API to re-add the watcher.

fgnass commented 8 years ago

published to npm as 3.0.1