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

Fix logfile tailing under Node 10.16.0 #84

Merged
merged 2 commits into from
Sep 20, 2019
Merged

Conversation

wearhere
Copy link
Contributor

fs.watch does not consistently emit 'change' events on Node 10.16.0,
possibly due to nodejs/node#29460. fs.watchFile
still works, but is less efficient, per the docs, so fall back to that only if necessary and also suggest that users run custody under an older version of Node.

I explored several Node file-watching libraries (e.g. chokidar, sane) to try to
find a more performant solution for Node 10.16.0 and it seems that they pretty
much all fall back to fs.watch / fs.watchFile, unfortunately. sane offers
the option to use watchman, but that requires installing a daemon so is a
non-starter.

Fixes #82.

#83 tracks reverting this when the underlying Node issue has been fixed.

Tested that log files update both when running custody under v8.9.3 and
v10.16.0, and that node-tail only uses fs.watchFile in the latter.

`fs.watch` does not consistently emit `'change'` events on Node 10.16.0,
possibly due to nodejs/node#29460. `fs.watchFile`
still works, but is less efficient, per the docs, so fall back to that only if necessary and also suggest that users run custody under an older version of Node.

I explored several Node file-watching libraries (e.g. chokidar, sane) to try to
find a more performant solution for Node 10.16.0 and it seems that they pretty
much all fall back to `fs.watch` / `fs.watchFile`, unfortunately. sane offers
the option to use watchman, but that requires installing a daemon so is a
non-starter.

Fixes #82.

#83 tracks reverting this when
the underlying Node issue has been fixed.

Tested that log files update both when running custody under v8.9.3 and
v10.16.0, and that `node-tail` only uses `fs.watchFile` in the latter.
@wearhere wearhere merged commit ff061dd into master Sep 20, 2019
@wearhere wearhere deleted the jeff/fix_tail_node_10 branch September 20, 2019 00:01
@meatwallace
Copy link

Saw you merged, but "lgtm" anyway! I checked the issues and saw you're tracking the Node.js issue so we can revert - fantastic.

Barely related/relevant tidbit: not suggesting adding watchman as a dependency is a good idea unless you have a good reason, but the latest version of Flow has an opt-in setting for using watchman for file watching - If we converge to a monorepo at any point, we'll likely end up adopting it 😅

@wearhere
Copy link
Contributor Author

wearhere commented Sep 20, 2019

the latest version of Flow has an opt-in setting for using watchman for file watching - If we converge to a monorepo at any point, we'll likely end up adopting it 😅

Oh neat. Yeah custody could switch to a version of node-tail that used sane to abstract over multiple file-watching libraries, and expose a flag like --useWatchman to let users that did have the daemon installed opt into that method.

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.

Tail is not updated in real time / requires manual 'refresh' using Node >=10
2 participants