Skip to content

Use fs.watchFile instead of fs.watch for compatibility#7

Merged
nicjansma merged 1 commit into
nicjansma:masterfrom
paton:patch-2
Sep 13, 2014
Merged

Use fs.watchFile instead of fs.watch for compatibility#7
nicjansma merged 1 commit into
nicjansma:masterfrom
paton:patch-2

Conversation

@paton

@paton paton commented Sep 6, 2014

Copy link
Copy Markdown
Contributor

Some text editors (like vim) write to a temp file, and the overwrite the original file with the temp file.

fs.watch works by listening to a particular file, so when I save a file, the original watcher is disconnected (because the file was overwritten by temp file) and the file is no longer being watched.

Switching to fs.watchFile fixes this problem because fs.watchFile works by polling the file to check for changes, while fs.watch uses a lower level file-watching feature of the operating system it's running on.

I've been using a version of this module with this change for the last year and it's worked well.

@nicjansma

Copy link
Copy Markdown
Owner

Thanks paton, this prompted me to read up on the two APIs a bit more:
http://nodejs.org/docs/v0.10.0/api/fs.html#fs_fs_watchfile_filename_options_listener

It looks like watchFile() is considered slightly more portable, though with a performance penalty because you're now polling. However, as you mention, this also gives flexibility with how some editors behave.

The only change I would prefer is to not poll every 150ms. Something like 500ms or 1s would seem adequate.

@paton

paton commented Sep 8, 2014

Copy link
Copy Markdown
Contributor Author

500 / 1000ms isn't quite adequate in my case, as I often edit a file and refresh the page in less than that amount of time.

Perhaps we can default it at 500ms and add an option to input a custom interval in the configuration.

@nicjansma

Copy link
Copy Markdown
Owner

That sounds great -- would you mind updating the PR with a new option? pollInterval or something?

@paton

paton commented Sep 8, 2014

Copy link
Copy Markdown
Contributor Author

Awesome! Will update the PR later today.

@paton

paton commented Sep 10, 2014

Copy link
Copy Markdown
Contributor Author

@nicjansma just updated the PR. Let me know what you think

@nicjansma nicjansma merged commit 20096be into nicjansma:master Sep 13, 2014
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

Successfully merging this pull request may close these issues.

2 participants