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

Replace fast-glob with a lighter tinyglobby #165

Merged
merged 9 commits into from
Sep 8, 2024

Conversation

ziebam
Copy link
Contributor

@ziebam ziebam commented Sep 8, 2024

This PR is a suggestion to switch from fast-glob (17 subdependencies) to a lighter tinyglobby which aims to behave the same way with a much smaller footprint (at present it only depends on fdir and picomatch).

Please check #6ee4084's commit message for rationale regarding removing the distinction between Windows and Unix systems regarding case sensitivity when matching.

`tinyglobby` uses `picomatch` under the hood which default to
case-sensitive matching. Not sure what are the performance
implications of performing such matching on Unix systems, but in case
we want to preserve the distinction, an update in `tinyglobby` would be
needed to expose the underlying `picomatch` options or add a new
top-level one that'd control the `nocase` flag.
package.json Outdated Show resolved Hide resolved
@ai
Copy link
Member

ai commented Sep 8, 2024

I love idea of reducing footprint a lot. I am open to this idea.

  1. We need to fix CI

Please check #6ee4084's commit message for rationale regarding removing the distinction between Windows and Unix systems regarding case sensitivity when matching.

  1. The link is broken. Can you explain the question?

@ziebam
Copy link
Contributor Author

ziebam commented Sep 8, 2024

  1. The exact same tests are failing locally even on main (before I've made any changes and installed a clean fork), could this be an issue with postcss?
  2. The gist of it is that tinyglobby currently doesn't expose the caseSensitiveMatch option similar to fast-glob, so with my change here we would perform case-sensitive matching even on Unix systems. Such option is gonna be added in the next release of tinyglobby though (SuperchupuDev/tinyglobby@0488df3), so maybe we should just wait a bit?

@ai
Copy link
Member

ai commented Sep 8, 2024

OK, I will with main first.

Anyway we will do breaking change release, so small changes is not a big deal. But if the release is soon, we may wait.

@ai
Copy link
Member

ai commented Sep 8, 2024

The main runs on CI without issues https://github.com/postcss/postcss-mixins/actions/runs/10763514076

Can you rebase branch (I push dependencies update, maybe it will help)?

@ziebam
Copy link
Contributor Author

ziebam commented Sep 8, 2024

I pulled the latest changes from main locally, and the same 3 tests are failing after a clean install. Same with my branch after merging. I noticed that changing c.CSS to lowercase c.css helps, but it shouldn't matter for CI since it runs on Ubuntu. I wonder what's going on here, gonna investigate further tomorrow as I'm off to sleep now. 😄

@ziebam
Copy link
Contributor Author

ziebam commented Sep 8, 2024

Should be good now.

I think the problem stemmed from the caseSensitiveMatch option evaluating to true on my machine (I'm on Windows), and also defaulting to that on CI after my initial changes (since there was no such option in tinyglobby yet), and because the MIXINS_GLOB only includes lowercase extensions, it wouldn't catch the .CSS file.

My suggestion that I committed is to always match case-insensitively to avoid such slip ups in the future and catch any odd OS + filename combinations that might occur (e.g. a style.cSs file wouldn't get loaded on Windows with the current implementation on main).

Another solution would be to flip the condition (!IS_WIN) as Windows is case-insensitive, so caseSensitiveMatch: IS_WIN (without the !) seems incorrect.

With either of these solutions, this PR doubles as a bug fix for some rare edge cases on Windows.

What do you think?

@ai
Copy link
Member

ai commented Sep 8, 2024

My suggestion that I committed is to always match case-insensitively

Good idea. Every complex solution could lead to the problem in the future. It is always better to keep everything simple.

@ai ai merged commit dd13095 into postcss:main Sep 8, 2024
3 checks passed
@ziebam ziebam deleted the fast-glob-to-tinyglobby branch September 8, 2024 23:36
@ai
Copy link
Member

ai commented Sep 8, 2024

Thanks for making Node.js ecosystem lighter.

Released in 11.0.0.

@ai
Copy link
Member

ai commented Sep 8, 2024

BTW, can we replace globby with tinyglobby in Size Limit?

@ziebam
Copy link
Contributor Author

ziebam commented Sep 8, 2024

BTW, can we replace globby with tinyglobby in Size Limit?

I think there should be no problems with that! Logged an issue in ecosystem-cleanup so that I get around to it during the week. 🙂

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.

2 participants