Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: switch from fast-glob to fdir #2433

Merged
merged 5 commits into from
Aug 19, 2024

Conversation

@dummdidumm dummdidumm changed the base branch from master to svelte-check-major July 31, 2024 11:56
- use withFullPaths to simplify code
- use exclude to speed up crawling
- fix filter function
- error on invalid patterns
@dummdidumm
Copy link
Member

Tweaked a few things and switched to fdir in svelte-language-server aswell - @jasonlyu123 anything that speaks against that?

});
const absFilePaths = files.map((f) => path.resolve(workspaceUri.fsPath, f));
const isIngored = (path: string) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isIngored = (path: string) => {
const isIgnored = (path: string) => {

@benmccann
Copy link
Member Author

Glad you were able to figure out how to call fdir directly as it can be tricky to figure out the combination of options needed! Since I created this PR there's been a new tinyglobby package created, which is a drop-in replacement for fast-glob that has only fdir and picomatch as dependencies, so if it's helpful you could also take a look at that. I didn't quite follow what some of the new code was doing...

@dummdidumm
Copy link
Member

think going with fdir is fine, we don't really need the glob syntax support (and I'm fine with only supporting a very limited glob syntax subset for the --ignore option)

@jasonlyu123
Copy link
Member

jasonlyu123 commented Aug 1, 2024

fast-glob default to follow symlinks but fdir didn't. Do we want to support it? The default of fdir is to read the symlinked file but not walk down the symlinked directory. After writing down this I think this is good enough. Although it might be a minor breaking change but I doubt there are many people does this. In this case, this also fixes #2364

@dummdidumm
Copy link
Member

Interesting! I think the behavior of not following symlinks is better, and instead walk the tree like the user sees it visually. In that sense I would acutally count this as a nice side-effect bugfix.

@dummdidumm dummdidumm merged commit ea8c0bd into sveltejs:svelte-check-major Aug 19, 2024
3 checks passed
dummdidumm added a commit that referenced this pull request Aug 27, 2024
- breaking: make TypeScript a peer dependency
- breaking: require node 18 or later
- breaking: require Svelte 4 or later (devDependencies pinned to Svelte 3 because other packages in this repo still use it. Theoretically we still support Svelte 3 with svelte-check v4 but this gives us the opportunity to adjust that later without a major)
- chore: switch from fast-glob to fdir (#2433)

closes #2397
fixes #2364

---------

Co-authored-by: Simon Holthausen <[email protected]>
Co-authored-by: Ben McCann <[email protected]>
.withPathSeparator('/')
.exclude((_, path) => {
// no / at the start, path could start with node_modules
return path.includes('node_modules/') || path.includes('/.');
Copy link
Member

@brunnerh brunnerh Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the second term necessary?
According to the previous code, the pattern was added as a workaround to exclude node_modules in a . directory.

Copy link
Member

@dummdidumm dummdidumm Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's necessary to not traverse into directories such as .git etc (directories starting with a dot are generally considered to be private), and I can't think of a situation where you would have a svelte.config.js in one of those directories - do you have such a case or why do you ask?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used . directories for caching/generated content but don't think they needed checking/processing so far.
Was just wondering if this could be simplified, but it's probably also faster this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Drop fast-glob dependency from svelte-check
4 participants