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

feat: drop glob usage #1742

Merged
merged 1 commit into from
Jun 21, 2024
Merged

feat: drop glob usage #1742

merged 1 commit into from
Jun 21, 2024

Conversation

43081j
Copy link
Contributor

@43081j 43081j commented Jun 8, 2024

Removes glob and uses fdir instead with a basic directory traversal (no globs).

Reduces the complexity of the file reading logic and the install size (~3MB glob vs 56KB fdir).

Removes `glob` and uses fdir instead with a basic directory traversal
(no globs).

Reduces the complexity of the file reading logic and the install size
(~3MB to 56KB).
@43081j
Copy link
Contributor Author

43081j commented Jun 10, 2024

@mjeanroy any chance you can take a look at this?

it'll reduce install size upstream in a fair few packages (which ill contribute the version bump to once/if this lands)

@mjeanroy
Copy link
Owner

Hi @43081j,

Thanks for the PR!
Could you elaborate a bit more on what benefits it would bring?

I'm not against changing one dependency to another, but I'd like to understand pros & cons before, if you can give a bit more details that would be great.

@43081j
Copy link
Contributor Author

43081j commented Jun 11, 2024

sure 👍

this is partly related to the ecosystem cleanup im leading

in this particular case, we're only using glob for case-insensitive file matching and wildcard extensions. so we don't actually need a glob implementation to do what we want to do

glob libraries come at a cost (mostly package size and general cpu performance). by switching to something like fdir, we will see some perf gains and reduce the install footprint a fair chunk

for example, if you look at the tree, fdir has 0 dependencies while glob pulls in 15+ packages. this adds up in the wider picture

every little helps. this will improve some dependents which will gain from other cleanups i've been doing around other packages too

@mjeanroy
Copy link
Owner

mjeanroy commented Jun 21, 2024

Thanks for all the details!
I think it's worth replacing it, thanks for that!

Sorry for the delay, I have a lot of work these days :)

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