⚡️ provide exclude patterns to file watcher args
#1607
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
misode
requested changes
Sep 27, 2024
| 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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