Skip to content

Enhancement: replace globby with fast-glob #9453

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

Closed
4 tasks done
SukkaW opened this issue Jun 29, 2024 · 16 comments · Fixed by #9518
Closed
4 tasks done

Enhancement: replace globby with fast-glob #9453

SukkaW opened this issue Jun 29, 2024 · 16 comments · Fixed by #9518
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing.

Comments

@SukkaW
Copy link
Contributor

SukkaW commented Jun 29, 2024

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

typescript-estree

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Previously, typescript-eslint switched from glob to globby for tsconfig.json file searching: #2418

Under the hood, globby is powered by fast-glob with extra features, but none of them are utilized by the typescript-eslint.

Since the typescript-eslint doesn't utilize those extra features, I propose replacing globby with fast-glob to reduce the installation size.

Additional Info

The installation size difference

The installation size of globby is 605 KiB and 23 transitive dependencies are introduced, while the installation size of fast-glob is 503 KiB and 18 transitive dependencies are introduced.

Proof of Concept

I've tested the proposal in my fork: SukkaW@0cec339 and all existing test cases are passed on my machine.

globby's extra features

Here is globby's feature description:

  • Promise API
    • fast-glob also has promise-based API, and typescript-eslint only uses the sync API
  • Multiple patterns
    • globby's multiple patterns support is powered by fast-glob directly
  • Negated patterns: ['foo*', '!foobar']
    • globby's negated patterns support is also powered by fast-glob directly
  • Expands directories: foo → foo/**/*
    • typescript-eslint doesn't rely on this feature since the exact matches of tsconfig.json (-like) files are required
  • Supports .gitignore and similar ignore config files
    • typescript-eslint doesn't rely on this feature since globby disabled this feature by default and typescript-eslint doesn't enable it.
  • Supports URL as cwd
    • Currently typescript-eslint passed the string path to cwd, and we can use Node.js built-in url.fileURLToPath in case we encounter URLs in the future.
@SukkaW SukkaW added enhancement New feature or request triage Waiting for team members to take a look labels Jun 29, 2024
@JoshuaKGoldberg
Copy link
Member

Ooh nice spot! I'm in favor of this personally. 👍

@SukkaW
Copy link
Contributor Author

SukkaW commented Jun 29, 2024

Ooh nice spot! I'm in favor of this personally. 👍

In case the proposal is accepted, I am wondering whether the change should be introduced to v8 only, or backport it to v7 as well. What do you think?

@benmccann
Copy link
Contributor

Both libraries are quite large. What about using something smaller like fdir?

Compare https://npmgraph.js.org/?q=fast-glob vs https://npmgraph.js.org/?q=fdir

See rollup/plugins#1741 for an example of switching

@SukkaW
Copy link
Contributor Author

SukkaW commented Jun 29, 2024

Both libraries are quite large. What about using something smaller like fdir?

The user-land config of typescript-eslint supports extended glob patterns, thus we have to choose one that has first-party glob pattern supports, where picomatch doesn't have.

@fregante
Copy link
Contributor

fregante commented Jul 1, 2024

typescript-eslint is currently Node 18+. Once you reach Node 22 (end of the year?) you will be able to use the native globbing library for a total of 0k and 0 dependencies 😃

@bradzacher
Copy link
Member

bradzacher commented Jul 1, 2024

We (and the entire ecosystem really) won't drop node 18 till it's EOL - 2025-04-30.
Same for v20 - 2026-04-30.

https://github.com/nodejs/Release?tab=readme-ov-file#release-schedule

@SukkaW
Copy link
Contributor Author

SukkaW commented Jul 1, 2024

Also, there might be some behavior differences between fast-glob and Node.js glob. And Node.js built-in glob is not stable yet.

@SuperchupuDev
Copy link
Contributor

just my two cents: after weeks of trying to replace globby with lighter alternatives i've published tinyglobby which only uses two subdependencies. maybe not outright switching to it but it could be an option once it gets more battle tested in other packages

@bradzacher
Copy link
Member

I don't really have a problem with switching out the underlying library for globbing.
If we're not using any of the features globby adds and there isn't any visible change for users in terms of supported glob patterns -- then we're free to do whatever without a major release.

If we can make things faster and / or smaller without too much effort then happy for it to be done!

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Aug 23, 2024
@bradzacher bradzacher linked a pull request Aug 25, 2024 that will close this issue
3 tasks
@bradzacher
Copy link
Member

So we have #9518 and #9878.
Do we replace globby with tinyglobby or fastglob.
Is one better than the other? I don't have an opinion either way, personally.

cc @typescript-eslint/triage-team

@SukkaW
Copy link
Contributor Author

SukkaW commented Aug 25, 2024

Do we replace globby with tinyglobby or fastglob. Is one better than the other? I don't have an opinion either way, personally.

@bradzacher

tinyglobby is more lightweight than fast-glob, but I don't know which one is more performant.

Since finding the corresponding tsconfig.json file for the current linting file is a hotpath, we might wanna merge this one first since fast-glob is consistently faster and more predictable (the globby we want to replace is powered by fast-glob under the hood, hence the behavior match). We can investigate tinyglobby later.

@benmccann What do you think?

@benmccann
Copy link
Contributor

fast-glob has 17 dependencies whereas tinyglobby has 2. It varies based on the input, but from what I've seen tinyglobby is a bit faster than fast-glob

Screenshot_from_2024-08-22_17-51-18

@benmccann
Copy link
Contributor

I've gotten the CI green now on #9878 based on including some of the changes from this PR. I think we could probably just merge that one since tinyglobby is both smaller and faster

I didn't include the change from #9518 (comment) since it was unrelated to switching glob libraries. I could update my PR to include it if desired or maybe that PR could be updated to just have that change or the change could be submitted as a new PR

@JamesHenry
Copy link
Member

fast-glob is very well established and it's very common usage across other heavily used packages likely makes the dependency point less relevant in practice (they will be deduped a lot). I would personally rather go for an established library here. Maybe we can revisit in the future if tinyglobby gains wider adoption (it was literally created during the lifecycle of this issue so it's as nascent as it gets right now)

@benmccann
Copy link
Contributor

While tinyglobby is pretty new, most of the logic is in more established libraries it uses as dependencies such as picomatch, which is also used by fast-glob under the covers. The tibyglobby code is so small there's little room for bugs in it. And it's already used well over 100,000 times/week. fast-glob would have to be used in about 90% of projects with none using tinyglobby for consumers to come out ahead from deduping since it has so many more dependencies. I actually have tinyglobby in my dependency tree multiple times already because a number of other libraries have switched. Nuxt just switched to it a few days ago: https://github.com/nuxt/nuxt/pull/28686/files

@bradzacher
Copy link
Member

As a first step let's go to fast-glob as it's:

  • a natural single-step progression downward
  • is 100% known to handle the same cases as globby
  • is a well-established library with 44m weekly DLs
    • As James mentions -- this means it's highly likely to already be in a project's dep tree.
    • I.e. we already have 9 references to it in our yarn.lock.

In future I'm open to revisiting this and switching to tinyglobby instead when it gains a bit broader adoption.
But yeah let's go with one step-at-a-time.

@github-actions github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Sep 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing.
Projects
None yet
7 participants