⚡️ provide exclude patterns to file watcher args
#1607
Conversation
- known pitfall of chokidar described in the comment. we can help users avoid this easy mistake
- i think in the vast majority of cases, users won't want us parsing any files in there (.git, .yarn, ...)
- useful for excluding a project root that is only for references and Spyglass doesn't need to parse - windows made this a pain, but it works for me locally; i can specify an absolute path (e.g. a workspace root i want to exclude) and the watcher skips the root's files. - making it case-insensitive for windows was the annoying part
TheAfroOfDoom
changed the title
exclude paths to file watcher
TheAfroOfDoom
changed the title
exclude paths to file watcherexclude paths to file watcher args
TheAfroOfDoom
changed the title
exclude paths to file watcher argsexclude paths to file watcher args
| watch(locations: FsLocation[], options: { usePolling?: boolean }): FsWatcher | ||
| watch( | ||
| locations: FsLocation[], | ||
| options: WatchOptions, |
There was a problem hiding this comment.
This shouldn't directly use chokidar's WatchOptions, this is an interface that needs to be shared between the nodejs and browser externals
| } | ||
| } | ||
| // Chokidar never matches paths with a trailing (back)slash, so fix any paths | ||
| // that may be specified as such | ||
| this.#excludePaths.push(...paths.map((path) => path.replace(/(\\|\/)$/, ''))) |
There was a problem hiding this comment.
Since this is a chokidar specific requirement, this should be moved to the NodeJsExternals watch handler.
| @@ -185,6 +187,7 @@ export class Project implements ExternalEventEmitter { | |||
| config!: Config | |||
| ignore: Ignore = ignore() | |||
| readonly downloader: Downloader | |||
| readonly #excludePaths: string[] = [] | |||
There was a problem hiding this comment.
This is weirdly named, it should be excludePatterns
| .map((path) => resolvePath(path)) | ||
| const matchesAbsolutePath = (path: string): boolean => { | ||
| return absoluteExcludePaths.some((exclude) => { | ||
| if (process.platform === 'win32') { |
TheAfroOfDoom
changed the title
exclude paths to file watcher argsexclude patterns to file watcher args
|
TODO should maybe also bump cache version |
|
No longer relevant after #1695 was fixed. I did experiment briefly with passing the patterns to Chokidar, but passing a function made performance worse (because |
right now we filter files for Spyglass to care about after we've already told chokidar to watch all our root directories.
chokidar ends up watching every file in the directory and is very slow, even if we later ignore certain files based on
config.env.exclude.not only that, but i couldn't get exclude patterns to work that you'd expect to work. for instance:
datapacks/omega-flowey/data/*/testwouldn't match the correspondingtestdirectory relative to any of the project roots (mostly fixed ✅)so i'd get lots of spyglass errors due to
packtest's non-vanilla commands just on project openSpyglass still shows errors if you open the file, but this is progress.
we could fix ^ by hooking the exclude config into
connection.onHover(etc.) somewhere, but that's out of scope hereexplicitly ignored datapack directories would still get parsed and show up in references/definitions (also mostly fixed ✅)
i have a
builddirectory that is.gitignored, and this would still show up when i open a function's references.on project load, these files are no longer bound to the function references, but they unfortunately still are bound again if you open the file. still an improvement though, and we can probably remedy further with similar methods to the
packtestrelated stuff above.gitignore'd directoryresourcepack/pack.mcmeta(from project root(s)) wouldn't match a RP pack.mcmeta; i'd still get pack version errorsnote that while the watcher is no longer watching this file if i add it to my exclude list, Spyglass still finds it because
findPackMcmetadoesn't use the exclude config yet.maybe less relevant though bc i know Misode's been working on improving that specific bug anyway
major performance gain
regardless of the above issues being partially remedied, the biggest gain we get is from chokidar not recursively adding every file under every project root to the watched files list. with a properly formatted
ignoredlist, chokidar stops early at a directory that matches an ignore pattern.profile comparisons:
#readytimewatch file count