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

Enhancement: replace fast-glob with tinyglobby #10533

Open
4 tasks done
benmccann opened this issue Dec 21, 2024 · 11 comments
Open
4 tasks done

Enhancement: replace fast-glob with tinyglobby #10533

benmccann opened this issue Dec 21, 2024 · 11 comments
Labels
blocked by another issue Issues which are not ready because another issue needs to be resolved first enhancement New feature or request

Comments

@benmccann
Copy link
Contributor

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

fast-glob has 17 dependencies with 12 people having publish access to those libraries and takes 505kb after installation
https://npmgraph.js.org/?q=fast-glob
https://pkg-size.dev/fast-glob

tinyglobby has only 2 dependencies with 6 people having publish access to those libraries and takes 155kb after installation
https://npmgraph.js.org/?q=tinyglobby
https://pkg-size.dev/tinyglobby

I would be happy to send a PR for this

Additional Info

This was discussed a bit in #9453, but I thought it would be a good time to revisit as tinyglobby usage has grown 30x since that time having been adopted by projects like vite, nx, and nuxt. tinyglobby is likely to dedupe in user projects now. E.g. this repo already pulls in tinyglobby multiple times (via nx and cspell). And other projects in the eslint ecosystem like eslint-import-resolver-typescript (downloaded 10m times per week) depend on tinyglobby already. While fast-glob is still downloaded more frequently, 80% of those downloads come from this project, so whatever library this project uses will end up being the most used.

@benmccann benmccann added enhancement New feature or request triage Waiting for team members to take a look labels Dec 21, 2024
@james-pre
Copy link

james-pre commented Dec 23, 2024

@benmccann

I'm not a maintainer, but I think replacing fast-glob would be great. Going off of the replacement of is-number in https://github.com/micromatch/to-regex-range (which hasn't been released after 5 months), I think this could save a few Terabytes of weekly traffic on npm, which is a huge win.

Just a thought, it may be worth-while to use fs.globSync rather than another package- this has some pretty big benefits considering it completely eliminates the dependency. It would require a change to the Node.js version needed though (>= 22.0.0).

I'd also be willing to work on a PR for this, please let me know if you're interested.

@bradzacher
Copy link
Member

it may be worth-while to use fs.globSync rather than another package- this has some pretty big benefits considering it completely eliminates the dependency. It would require a change to the Node.js version needed though (>= 22.0.0).

We cannot do that until our minimum version is v22 - which won't happen until v20 is out of LTS

@james-pre
Copy link

Thanks for the quick reply.

It looks like Node v20 is out of active LTS (as of Oct. 2024) and will be in maintenance until April 2026.

It was trivial for me to make the change to using the built-in globSync, so I went ahead and made the change locally. We can always put this on ice: james-pre@d555589.

I'll look into switching to tinyglobby tomorrow if I have time.

@JoshuaKGoldberg
Copy link
Member

I'm mildly 👍 on this. It's a fine idea. I see no downside now that the library is widely depended-upon.

@ronami
Copy link
Member

ronami commented Dec 23, 2024

I like this idea, having less dependencies is always nice.

However, I think it may take some time for tinyglobby to be as stable as fast-glob, and using it now may potentially introduce bugs (as in 1, 2, 3).

From what I can see, tinyglobby is getting more popular, and those bugs are getting fixed, so I think it depends how stable it currently is.

@JoshuaKGoldberg
Copy link
Member

Oof, yeah, good call @ronami. I think we should wait a few months for this to all settle down.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by another issue Issues which are not ready because another issue needs to be resolved first and removed triage Waiting for team members to take a look labels Dec 23, 2024
@james-pre
Copy link

It looks like the problems referenced by @ronami have either been resolved or don't affect this project, since all of the tests in the draft PR passed.

@JoshuaKGoldberg
Copy link
Member

True, but the fact that there were problems in the first place shakes confidence in the project.

This isn't a pressing issue IMO. It's nice to reduce dependency sizes, but given how many projects are still using fast-glob -including those linked ones- I don't think we're the only blocker to a bunch of npm savings.

@benmccann
Copy link
Contributor Author

I think waiting a few months is fair as tinyglobby is still a somewhat new library.

I will say that the tinyglobby author has fixed any issues very quickly as they've been reported and has added a test each time to prevent any regressions. E.g. in that link to the cspell repo, the issue was promptly fixed and then they upgraded to the latest rather than rolling back and vitest is also in process of switching back to tinyglobby again. I'm also talking to the tinyglobby author about seeing if we can automatically run the test suites of all the major projects that depend on it to instill more confidence as well (this is something we've done for Svelte and Vite).

Btw, globby / fast-glob are not without issues. I've hit multiple issues with it when switching where tinyglobby is returning something different and it's very clearly a bug in globby / fast-glob, but people have just gotten used to working around it. New bugs tend to be more noticeable than old bugs though.

Anyway, I think it makes sense for a project as large as this one to be more conservative and wait for any issues to shake out with other consumers first. Let's check back in a few months from now and see if everything has been quiet with others adopting the library.

@mrmlnc
Copy link
Contributor

mrmlnc commented Jan 5, 2025

Btw, globby / fast-glob are not without issues. I've hit multiple issues with it when switching where tinyglobby is returning something different and it's very clearly a bug in globby / fast-glob, but people have just gotten used to working around it. New bugs tend to be more noticeable than old bugs though.

To me, this looks like too aggressive marketing. In many issues and PR's mentioning fast-glob, I see your comment. New glob library come along every year, that has only a subset of the functionality of the old one and claims to be better. When these libraries start to be used, people start asking to add the same options that are available in fast-glob. In your case, it will be brace expansion, stream API, error handling, smart complex pattern processing when reading deeply, …. Then the maintainers face a choice – to write a solution themselves (without dependencies) or to use already existing and tested solutions (like braces, merge2, …).

I do not share the opinion that the number of dependencies (under reasonable assumptions) is a selling point. Stability, functionality and maintainability – yes. For me. Some rational balance is needed here. People may think differently.

…it's very clearly a bug in globby / fast-glob

Please report any issues you find with fast-glob. This is the main purpose of this comment. Yes, I don't fix them as quickly as people would like, but the critical problems have long been resolved.

No negativity. Just my thoughts. Having competitors is a positive thing. That's how I started this project [fast-glob] in 2017. I was not happy with the speed, complexity and maintainability of the existing solutions.

@bradzacher
Copy link
Member

My 2c:
It was not even 6 months ago that we switched from globby to fast-glob.
We did this for two reasons:

  • globby is built on top of fast-glob, but we weren't using the extra features globby added.
  • globby (as with all sindresorhus packages) switched to ESM - meaning we were unable to upgrade and get new patches.

These were good reasons to migrate - with a quick, 1:1 migration we cut one layer of indirection from our dependencies and we unlocked patches for a dependency.

Note that install size simply isn't a concern for us - we're a local, dev-only tool. You realistically install our dependencies once and never again as we very rarely actively bump our required minimum ranges. We're also talking about a 353,122b difference. That's nothing - it won't impact anyone's download/install times.
For reference when you install the typescript-eslint package the total installed size is 40,367,158b. So by cutting 353,122b we've reduced the total weight by just 0.8% -- and that's IF AND ONLY IF no other package in the repo's closure depends on fast-glob (which is unlikely - eg eslint-import-resolver-typescript and @next/eslint-plugin-next both depend on it and are widely used).

In order for me personally to back this -- what I'd want to see is some benefit beyond just "it's smaller". Is it faster? Does it provide some killer API that makes our lives better? Is it safer and more secure? etc.
A minimal shift in install size is not good enough reason for us to action this, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked by another issue Issues which are not ready because another issue needs to be resolved first enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants