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 fast-glob to tinyglobby #18243

Merged
merged 21 commits into from
Oct 17, 2024

Conversation

ziebam
Copy link
Contributor

@ziebam ziebam commented Sep 30, 2024

Description

As discussed in the Vite Discord (https://discord.com/channels/804011606160703521/804439875226173480/1282752023930081310), this PR is a suggestion to replace heavier fast-glob with lighter tinyglobby. The supported patterns are the same except for the incremented braces.

Copy link

stackblitz bot commented Sep 30, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

packages/vite/src/node/optimizer/resolve.ts Outdated Show resolved Hide resolved
packages/vite/src/node/optimizer/resolve.ts Show resolved Hide resolved
@@ -347,8 +348,6 @@ function globEntries(pattern: string | string[], environment: ScanEnvironment) {
? []
: [`**/__tests__/**`, `**/coverage/**`]),
],
absolute: true,
suppressErrors: true, // suppress EACCES errors
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such errors are suppressed by default in tinyglobby.

@ziebam ziebam marked this pull request as draft September 30, 2024 14:49
@ziebam ziebam marked this pull request as ready for review October 1, 2024 00:34
@@ -1385,6 +1385,21 @@ Repository: git+https://github.com/mcollina/fastq.git

---------------------------------------

## fdir
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should fast-glob be removed? Or does that depend on my PR to the rollup plugins?

Copy link
Member

Choose a reason for hiding this comment

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

It's still bundled because the dynamic import plugin depends on it. If your PR doesn't get merged before v6 release, I think we can patch it since we only use the dynamicImportToGlob function.

@sapphi-red sapphi-red added dependencies Pull requests that update a dependency file p1-chore Doesn't change code behavior (priority) breaking change and removed dependencies Pull requests that update a dependency file labels Oct 1, 2024
@sapphi-red sapphi-red changed the title chore(deps): migrate fast-glob to tinyglobby chore(deps)!: migrate fast-glob to tinyglobby Oct 1, 2024
@sapphi-red sapphi-red added this to the 6.0 milestone Oct 1, 2024
sapphi-red
sapphi-red previously approved these changes Oct 3, 2024
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks!

bluwy
bluwy previously approved these changes Oct 4, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I checked @rollup/plugin-dynamic-import-vars and it looks like the dynamicImportToGlob API that we only use still indirectly relies on fast-glob escapePath, so we actually need to have upstream swap to tinyglobby to deduplicate it, or fork the code locally as I guess it could take a while before the changes are accepted upstream. Maybe we can followup with that separately.

@ziebam
Copy link
Contributor Author

ziebam commented Oct 4, 2024

I checked @rollup/plugin-dynamic-import-vars and it looks like the dynamicImportToGlob API that we only use still indirectly relies on fast-glob escapePath, so we actually need to have upstream swap to tinyglobby to deduplicate it, or fork the code locally as I guess it could take a while before the changes are accepted upstream. Maybe we can followup with that separately.

There's a PR open in the upstream to make the migration, but as you said, it might take a while before it's merged and released: rollup/plugins#1780.

@benmccann
Copy link
Collaborator

@rollup/plugin-dynamic-import-vars version 2.1.4 now uses tinyglobby!

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as duplicate.

@vite-ecosystem-ci
Copy link

@sapphi-red sapphi-red merged commit 6f74a3a into vitejs:main Oct 17, 2024
24 checks passed
@ziebam ziebam deleted the fast-glob-to-tinyglobby branch October 17, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p1-chore Doesn't change code behavior (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants