FileSystemWatcher should ignore folders specified under 'oni.exclude'

[josemarluedke]

In the last version of Oni (v0.3.3-1) it was introduced a file system watcher (#2116 with further fixes in #2121). The file system watcher should exclude folders that is specified under oni.exclude configuration, or even added new configuration.

For projects that generate tmp folders with many, many files under it, Oni becomes very unresponsive when opening such projects. After a certain amount of time with Oni open (~2-4 min), it will start to respond as expected.

[bryphe]

Thanks @josemarluedke for your investigation here!

IMO, this is a release blocker - I don't want users to hit unexpected performance regressions when upgrading. I like the idea of respecting oni.exclude as well.

I'm also wondering if it makes sense for the FileSystemWatcher to be 'off-by-default' for now - I also hit #2142 which was related to this behavior, so it might be good for it 'bake' a little more before being on-by-default. It's very useful but every platform has some file system quirks, and it's potentially sensitive to different project configurations, so it can be challenging to iron out all the issues and get this right across the board - having it 'off-by-default' will let us gradually test this. 👍 We could have a manual 'refresh' button or command in lieu of this, for now, too.

The one concern I have with it being 'off-by-default' is that it may impact some of the new functionality - like rename, add file, delete folder - @Akin909 might have some ideas here - perhaps we could explicitly refresh with those once the gestures are completed.

[akinsho]

@bryphe yeah I think the behaviour of the FileSystemWatcher when there are a lot of files can be a little intense and have a hit on performance although it might just be a question of figuring out what to listen to (There are quite a lot of configuration options almost none of which I'm using atm so there's a lot of room to tweak things). I'm fine with having it off by default and the new Explorer functionality I actually designed without much thought to the watcher so all that functionality already explicitly calls Refresh and I was actually thinking of a separate PR to remove the redundant calls although with the Watcher off by default then that allows these to function correctly on their own.

Unfortunately my mac died quite recently so I probably won't be able to make a PR to turn if off soon, but was thinking it could be as simple as wrapping the watcher init function in a configuration option to prevent it listening to the FS.

[josemarluedke]

I like the idea of having a command to refresh it manually. NERD tree has r as a command to refresh, IMO, it would be nice to have the same in the explorer.

[bryphe]

Unfortunately my mac died quite recently so I probably won't be able to make a PR to turn if off soon, but was thinking it could be as simple as wrapping the watcher init function in a configuration option to prevent it listening to the FS.

Ah ya, that makes sense. Well, hopefully once we explore this a bit, we can enable it to be on-by-default again. It wasn't clear to me if the bottleneck was somewhere in the File I/O operations chokidar is doing, or if it is due to heavy activity in our redux/react layer.

Unfortunately my mac died quite recently so I probably won't be able to make a PR to turn if off soon, but was thinking it could be as simple as wrapping the watcher init function in a configuration option to prevent it listening to the FS.

Oh man sorry to hear that @Akin909 😦 I'll be happy to put together a PR for this - I'll add an explorer.autoRefresh setting that's default to false. I tested and all the other functionality - like rename, copy/paste, add, seemed to refresh after the gesture (I didn't want that behavior to regress).

I like the idea of having a command to refresh it manually. NERD tree has r as a command to refresh, IMO, it would be nice to have the same in the explorer.

I like this idea too! It looks like we have r bound to rename currently - would <c-r> be reasonable for refresh?

[bryphe]

Keeping this open to track the remaining work of incorporating oni.exclude.

[akinsho]

Re. oni.exclude functionality the watcher can be initialiased with config options one of which is an ignored option which I currently pass a string for **/node_modules need to figure out if it can be passed an array of strings or if a regex needs to be formulated from the exclude options. Seems to use anymatch under the hood and the examples include an array being passed so think an array of paths/globs can be passed

[bryphe]

Cool, thanks @Akin909 ! That doesn't sound too bad 👍

[akinsho]

@bryphe I've had a look at adding the oni.exclude (using a loaner mac) to chokidar's ignored but I think in a very large dir's like the home dir oni still gets v. heavily spammed with fs events when entering a large dir, although you can initially ignore the first load aka if oni loads in a directory, though subsequent loads dispatch an event for every file in the watched dir.

I've had a look at how vscode have dealt with this and this is a link to their chokidarWatcher and they seem to have a throttle on the actual events which I think we discussed initially, events aren't lost as far as I can see just sent in more manageable chunks.

I'm still looking into ways round the slowdown including setting maximum limits on the depth of the files watched and also discovered paulmillr/chokidar#447 where the cpu usage is discussed although I'm seeing oni become v. slow to respond as opposed to necessarily really high cpu usage