Using fs.watchFile if supported, fs.watch otherwise (fixes #1803)

[TrevorBurnham]

This isn't elegant, but at least it doesn't do platform-sniffing. Instead, it:

• Improves backward compatibility (since it uses fs.watchFile under Node 0.4.x),
• Future-proofs us (in case fs.watchFile is removed in a future release), and
• Allows us to use the less-buggy fs.watchFile on non-Windows systems (see discussion at #1803)

I've tested that it works on the Mac (detecting saves from Coda and everything); someone else will have to give it a whirl under Windows.

[michaelficarra]

Does this work with high frequency file changes (greater than 1 per second)?

[TrevorBurnham]

@michaelficarra Why wouldn't it? On any system where fs.watchFile is supported, the behavior should be the same as pre-1.1.3.

@jashkenas Actually, it looks like we may be able to salvage fs.watch by re-binding every time we get a 'rename' event, so hold up...

[jashkenas]

Is the file being renamed?

[TrevorBurnham]

Yep: nodejs/node-v0.x-archive#2062 (comment)

[TrevorBurnham]

New pull request is at #1847. However, let me make one modification before merging...

[michaelficarra]

@TrevorBurnham: I was referring to the fs.watch fallback. But after a look at the nodejs docs, I see that mtime is accurate to the millisecond level.

[TrevorBurnham]

@michaelficarra Ah, yeah, I ran some tests on that yesterday while I was IRC-ing with jashkenas. If you do rapid changes (say, 10 of them with a 100ms delay between each), only a few of the changes are detected—but the last one always is, which is the important thing. (Obviously the polling approach used by fs.watchFile similarly misses rapid changes, but will always catch the last one.)

[michaelficarra]

Ah, okay, as long as the last change is always detected like it was before.

[TrevorBurnham]

Reopening this pull request in response to #1853.

[TrevorBurnham]

So the fs.watch showstopper that was crashing the process every time it was pointed at a nonexistent file was fixed in the Node 0.6.1 release, but in trying to build a comprehensive framework for using fs.watch properly, I came across a couple other bugs that are currently impossible to work around:

• fs.watch produces no error even though target does not exist nodejs/node-v0.x-archive#2093
• Closing FSWatcher at certain times causes "assertion failed" (Mac) nodejs/node-v0.x-archive#2094
• Closing FSWatcher at arbitrary time leaves process running nodejs/node-v0.x-archive#2095

@jashkenas, I really think we should merge this pull in (using fs.watchFile unless it's unavailable) until the Node team fixes these issues. Polling every 500ms is inelegant, but at least it doesn't crash the whole process if you do it at the wrong moment, or lose track of files when you do a git checkout (as reported at #1853).

Meanwhile, I'm working on a module that'll let us get the right behaviors out of fs.watch once the bugs are worked out.

[TrevorBurnham]

More bad news for fs.watch:

• Multiple fs.watch-es on the same file cause process to crash nodejs/node-v0.x-archive#2105

It's likely to take a while for the Node team to clean all this up.

[jashkenas]

Shall we just revert to the previous fs.watchFile implementation, and allow Windows users to suffer without --watch for a bit longer?

[michaelficarra]

Yes.

edit: I was a little quick with my run-away-from-fs.watch response. @TrevorBurnham's response below is more reasonable. I thought for a minute that the fs.watchFile error wasn't catchable, but that was a different error.

[TrevorBurnham]

I'd much rather do a 1.1.4 release based on this patch, with fs.watch as a fallback. Then everyone's happy, including Windows users and potentially people in a future where fs.watchFile is gone. Then we do a full-on transition to fs.watch in 1.2, after the Node team has worked the kinks out.

On Nov 14, 2011, at 1:20 PM, Jeremy [email protected] wrote:

Shall we just revert to the previous fs.watchFile implementation, and allow Windows users to suffer without --watch for a bit longer?

---

Reply to this email directly or view it on GitHub:
#1846 (comment)

[jashkenas]

I'd rather not half-ass it. If it's unusably buggy, then it's unusably buggy ... if it's good enough, then it's good enough for all of us. It sounds like the former is the case.

[TrevorBurnham]

Well, it's certainly more usable than fs.watchFile under Windows. Is it really a "half-ass" solution if it produces the best possible behavior on all platforms?

On Nov 14, 2011, at 1:27 PM, Jeremy [email protected] wrote:

I'd rather not half-ass it. If it's unusably buggy, then it's unusably buggy ... if it's good enough, then it's good enough for all of us. It sounds like the former is the case.

---

Reply to this email directly or view it on GitHub:
#1846 (comment)

[dreamcodez]

I tested this build on mac os lion, this works for me:

https://github.com/TrevorBurnham/coffee-script/tree/issue_1803

[TrevorBurnham]

Mathhew: Yes, but you'll hit errors in various edge cases; see my links to the Node issue tracker above.

On Nov 14, 2011, at 1:44 PM, Matthew [email protected] wrote:

I tested this build on mac os lion, this works for me:

https://github.com/TrevorBurnham/coffee-script/tree/issue_1803

---

Reply to this email directly or view it on GitHub:
#1846 (comment)

[timmolendijk]

What's against just being pragmatic/conservative with tools and other code that is not central to the project?

Ever since 1.1.3 I can no longer watch for changes (and no I'm not running Windows), while it used to work just fine. I'd say wait with using fs.watch until it's ready for prime time.