watchman_watcher: forward is_fresh_instance as an event #106
Conversation
README.md
Outdated
| In addition to change events, the watcher may emit a `fresh_instance` event that | ||
| signal the state got lost and we will not receive notifications for some of the | ||
| changes; notably, file deletions. The recommended approach when this happens is | ||
| to recrawl the filesystem according with your application logic, then continue |
There was a problem hiding this comment.
I would suggest invalidating caches here; the fresh instance result from watchman will include the full crawl results anyway. The key thing about this signal is that you should invalidate everything that you may have cached that was not included in the file list that had is_fresh_instance: true.
Since we're emitting the fresh instance event prior to passing on the list of files you may want to advise folks to invalidate everything they have cached and let them know that the event will be immediately followed by the full list of files.
README.md
Outdated
| @@ -46,6 +46,24 @@ watcher.on('delete', function (filepath, root) { console.log('file deleted', fil | |||
| watcher.close(); | |||
| ``` | |||
|
|
|||
| In addition to change events, the watcher may emit a `fresh_instance` event that | |||
There was a problem hiding this comment.
please clarify that this is a watchman-specific event.
jeanlauliac
force-pushed
the
watchman-fresh-instance
branch
from
d3f1300 to
67909b0
Compare
jeanlauliac
force-pushed
the
watchman-fresh-instance
branch
from
67909b0 to
d3e8e38
Compare
|
Hello there! ^_^ what do you think of the latest version? |
|
|
||
| ```js | ||
| function findFiles() { return new Set(glob('**/*.js')); } | ||
| var files = findFiles(); |
There was a problem hiding this comment.
this should just be an empty Set() to start with (at least with the base watchman implementation), as watchman will tell you about all of these in the initial response
There was a problem hiding this comment.
The problem is that the documentation doesn't mention that, and I believe the other watchers don't do that (providing "add" events for all files on start), so it would be confusing for the example not to follow the usual pattern. As such I'd suggest keeping the example like so, what do you think? (I know this is also incorrect, because a file may have been concurrently removed between the call to glob and the call to sane… but the other watchers are also incorrect for the same reason.)
| var files = findFiles(); | ||
| var watcher = sane('.', {glob: '**/*.js'}); | ||
|
|
||
| watcher.on('fresh_instance', () => { files = findFiles(); }); |
There was a problem hiding this comment.
and this can invalidate the cache by doing something like files.clear() or files = new Set(). The add event below will trigger to fill it up and save you from performing a glob call to walk the filesystem
There was a problem hiding this comment.
Sounds good for that one case.
|
@wez you're a collaborator, feel free to merge when you're happy with the PR. |
| ```js | ||
| function findFiles() { return new Set(glob('**/*.js')); } | ||
| var files = findFiles(); | ||
| var watcher = sane('.', {glob: '**/*.js'}); |
There was a problem hiding this comment.
note to self: we need to require the watchman watcher here, not any kind of watcher.
|
Superseded by jestjs/jest#5387. |
|
I, alas, don't have much time to look into this but I don't think this is the best way to solve it. Paging @stefanpenner maybe he has time to look into the underlying problem. |
As it happens, watchman can signal an
is_fresh_instancein subscription events. This is documented on thequerypage, but can happen for subscription events as well according with @wez.This changeset add a new event that forward this occurrence, as it means applications need to recrawl the filesystem. As this module is not concerned by file crawling in the first place, there's no way we can really handle this in a self-contained way: applications need to decide how to handle it, instead.
Note that exposing the
is_fresh_instancefeature as a simple event fromsaneis brittle and poor design: indeed, it is too easy to ignore an event, unknowingly keeping the application in a bad state. However, it has been the case for all that time already. Plus, simply adding a new event prevents us from having to release a major/breaking release.The alternative strategy would be to force people to pass a handler as argument to the watcher constructor (so, to the
sanetop-level function), or to shutdown the watcher instance whenever there is a fresh instance. But this would require a major version release. So I think the best tradeoff is forwarding the flag as an event.