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(deps): migrate globby to tinyglobby #938

Closed
wants to merge 10 commits into from

Conversation

pralkarz
Copy link

@pralkarz pralkarz commented Oct 5, 2024

This PR is a suggestion to replace globby with tinyglobby – a lighter alternative with less transitive dependencies.

);
return [];
}

const globbed = await globby(glob, {
cwd,
expandDirectories: false, // TODO Temporary workaround for https://github.com/mrmlnc/fast-glob/issues/47
Copy link
Author

Choose a reason for hiding this comment

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

The issue doesn't happen with tinyglobby, so we don't need to disable expanding directories.

onlyFiles: false,
})
).map((result) =>
result.endsWith("/") ? result.slice(0, -1) : result
Copy link
Author

Choose a reason for hiding this comment

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

This stems from the way tinyglobby returns the results:
366205111-4bfd55b6-014d-4781-b7d8-fbe6776228b4

In case returning a trailing slash wouldn't break users, and you decide to go forward with this migration, I'm open to updating the tests instead!

@benmccann
Copy link

@pralkarz it looks like this PR has a merge conflict

@pralkarz pralkarz force-pushed the globby-to-tinyglobby branch from 6772a44 to b084e88 Compare February 20, 2025 10:12
@pralkarz
Copy link
Author

@pralkarz it looks like this PR has a merge conflict

@benmccann Fixed!

Copy link
Member

@babblebey babblebey left a comment

Choose a reason for hiding this comment

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

Hey @pralkarz,

I'm of the belief that globby is still very much supported in the hands of @sindresorhus, which gives the package a good level of integrity, and it serves it purpose very well without issues.

Is their a specific reason - apart from the package size - why you suggest we swap globby for tinyglobby?? 🤔

@pralkarz
Copy link
Author

pralkarz commented Feb 21, 2025

Hey @pralkarz,

I'm of the belief that globby is still very much supported in the hands of @sindresorhus, which gives the package a good level of integrity, and it serves it purpose very well without issues.

Is their a specific reason - apart from the package size - why you suggest we swap globby for tinyglobby?? 🤔

Hey @babblebey!

globby is supported indeed, and admittedly more feature-rich than tinyglobby, but for simple use cases, the latter brings with it a smaller bundle size and less transitive dependencies, both of which I personally value quite highly. It also uses fdir under the hood which is much faster than globby.

It's completely valid to want to stick with a battle-tested solution though! This PR is just a suggestion, up to you whether you want to go ahead with the migration or not. 😄

@babblebey
Copy link
Member

Very well then @pralkarz, thank you for the suggestion.

I just wanted to understand whether there was a certain issue you experienced with globby hence the change with my question 😉

This being the case, I'm certain we would rather continue with globby considering our familiarity with it for now.

@babblebey babblebey closed this Feb 21, 2025
@pralkarz pralkarz deleted the globby-to-tinyglobby branch February 21, 2025 15:23
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.

3 participants