Skip to content

Conversation

yslpn
Copy link
Contributor

@yslpn yslpn commented Jul 28, 2025

Improve glob filtering performance by replacing micromatch with picomatch and decrease node_modules size

234kb vs 83kb

https://node-modules.dev/grid/depth#install=picomatch
https://node-modules.dev/grid/depth#install=micromatch

@yslpn yslpn marked this pull request as draft July 28, 2025 15:26
Copy link

changeset-bot bot commented Jul 28, 2025

🦋 Changeset detected

Latest commit: 95c6164

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
steiger Patch
@steiger/integration-tests Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yslpn yslpn marked this pull request as ready for review July 28, 2025 16:10
@illright
Copy link
Member

illright commented Aug 4, 2025

I believe the reason why we switched to micromatch instead of picomatch was because of this:

Picomatch does not do brace expansion. For brace expansion and advanced matching with braces, use micromatch instead. Picomatch has very basic support for braces.

https://github.com/micromatch/picomatch?tab=readme-ov-file#braces

I see the tests are passing with picomatch, which is interesting. I played around a bit with it and it seems like the most interesting use cases of braces (e. g. different extensions like .{test,spec}.{js,ts}) are supported by picomatch. Still, I think it would be a breaking change, so I would hold on with this change until the 0.6 release, which is currently blocked by #197

@illright illright changed the base branch from master to next August 4, 2025 21:21
@illright
Copy link
Member

illright commented Aug 4, 2025

I changed this PRs base to be the next branch, could you please rebase your commits on top of that branch so that we don't have to bother resolving conflicts?

illright
illright previously approved these changes Aug 4, 2025
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Code-wise I think it's great!


const positiveExclusionPatterns =
(thereAreExclusions && exclusions.filter((pattern) => !isNegatedGlob(pattern))) || []
const negativeExclusionPatterns =
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): since "positive exclusion" and "negative exclusion" aren't commonly understood terms, hence they require some time to understand, perhaps we could instead have "positive exclusion" and "re-inclusion"?

Suggested change
const negativeExclusionPatterns =
const reInclusionPatterns =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Fixed

@yslpn yslpn force-pushed the replace-micromatch-with-picomatch branch from 020d12d to 03ebcf0 Compare August 9, 2025 19:54
@yslpn yslpn marked this pull request as draft August 9, 2025 19:55
@yslpn yslpn force-pushed the replace-micromatch-with-picomatch branch from 03ebcf0 to dba2c54 Compare August 9, 2025 20:10
@yslpn yslpn force-pushed the replace-micromatch-with-picomatch branch from dba2c54 to 95c6164 Compare August 9, 2025 20:21
@yslpn yslpn marked this pull request as ready for review August 9, 2025 20:23
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

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

Thanks! The regression in the lilconfig migration made me a bit worried, could you add some extra tests to packages/steiger/src/shared/globs/create-filter-according-to-globs.spec.ts relating to brace expansion? Currently there's only one, and it only tests brace expansion in exclusions, I'd like to make sure it also works in inclusions and negated patterns.

@yslpn
Copy link
Contributor Author

yslpn commented Aug 10, 2025

Thanks! The regression in the lilconfig migration made me a bit worried, could you add some extra tests to packages/steiger/src/shared/globs/create-filter-according-to-globs.spec.ts relating to brace expansion? Currently there's only one, and it only tests brace expansion in exclusions, I'd like to make sure it also works in inclusions and negated patterns.

Ok, thanks for the review. I'll add more tests later this week.

@yslpn yslpn marked this pull request as draft August 10, 2025 21:11
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