Watch files opened via API

[50Wliu]

Implements & closes #467.

This is a very simple file-watching implementation - for every log opened via api.openLogs(), we'll add it to a file watcher. When those files change (or are deleted), we'll update the SARIF Viewer as appropriate. We'll remove the files from the watcher once they're closed.

What I don't know is how chokidar handles multiple add calls for the same file - will it start watching the same file twice? Or is it smart enough to know that it's already watching and no-op? I don't have an easy way to test that scenario, but I'm hoping it's a pretty rare edge case for someone to repeatedly openLogs on a file that's already open.

I've tested this locally and confirmed that the openLogs implementation works, but not that closeLogs successfully removes from the watcher (though I'd be very surprised if it didn't).

[jeffersonking]

@50Wliu: Thanks for taking the lead on this. Functionally this looks great. One, question is how often do you anticipate quick successions of edits/changes to the same file?

Note: I did a quick test and Chokidar seems to ignore adding files that have already been added.

Stylistically: I would prefer to have greater encapsulation for this new logic. I would propose ignoreInitial: false and have the watcher be 100% responsible for dealing with store.logs. watcher can be renamed (and thought of) as store.watchedLogs. This way you either deal with store.watchedLogs or store.logs, but not a mix of both (which might lead to mistakes).

[50Wliu]

how often do you anticipate quick successions of edits/changes to the same file?

For our use case - never. I can see how difficult it'd be to add a debounce timeout for each file though.

Note: I did a quick test and Chokidar seems to ignore adding files that have already been added.

Thanks!

Stylistically

This makes sense. I was thinking it'd be nice to rely on the initial functionality. If I get what you're saying - all the API methods would act on watchedLogs, which internally would update logs. Everything else continues to use logs as usual.

Thanks for the quick turnaround! I'll work on those changes.

[jeffersonking]

For our use case - never. I can see how difficult it'd be to add a debounce timeout for each file though.

Glad that's the case. Prefer to defer the debounce until absolutely needed.

[50Wliu]

Right, so I'm looking into moving the logic into the Store and turning off ignoreInitial, but I'm running into some issues:

1. Watching happens asynchronously, meaning that we can no longer use the cancellationToken provided in the openLogs API (all we'll do in the new model is add the new logs to the watcher - nothing cancellable there. Loading & showing the logs happens whenever the watcher gets around to it).
2. No corresponding event for "we've stopped watching a file", so closeLogs still has to operate directly on store.logs to remove the log.

I'm now thinking that the current implementation might be the most compact and straightforward (while preserving existing API functionality) - thoughts?

[chrishuynhc]

Hi @jeffersonking, have you had the chance to finish reviewing? @50Wliu is OOF and asked me to follow up on this change :)

[jeffersonking]

@chrishuynhc Not yet. Will do today. Thanks.

[jeffersonking]

@50Wliu Good observations and I agree.

Ideally we would be able to add/remove watched logs (say from other code paths) without going the API code path. However, we can do that refactoring when that day comes.

[50Wliu]

@jeffersonking, @EasyRhinoMSFT - any chance I could get a signoff + merge? I believe all concerns have been addressed :).

[50Wliu]

Thank you! ❤️

[EasyRhinoMSFT]

Thank you Winston! This is an important feature and you've helped our goal of keeping the two Viewer extensions in sync. 😎

[50Wliu]

@EasyRhinoMSFT - would you mind publishing a new release for this, please?

[EasyRhinoMSFT]

Sure thing -- apologies for forgetting this, I was on a two-week vacation.

[50Wliu]

Thanks so much!

[EasyRhinoMSFT]

@50Wliu release to the Marketplace complete 👍