replace micromatch with picomatch#1953
Conversation
🦋 Changeset detectedLatest commit: a8e32c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| } from "@changesets/types"; | ||
| import { getPackages } from "@manypkg/get-packages"; | ||
| import micromatch from "micromatch"; | ||
| import picomatch from "picomatch"; |
There was a problem hiding this comment.
I wonder, how does it compare to the builtin? https://nodejs.org/docs/latest-v22.x/api/fs.html#fsglobpattern-options-callback
There was a problem hiding this comment.
picomatch wayyyy faster iirc as the builtins use minimatch
but i don't have any benchmarks saved so i'll try to find or make some
i also mentioned this on bsky recently :^)
There was a problem hiding this comment.
We need to use this instead so we don't touch the fs: https://nodejs.org/api/path.html#pathmatchesglobpath-pattern
Open to trying it though but we still kinda need a wrapper to handle array of files and patterns.
There was a problem hiding this comment.
It would be nice to know the benchmark results. However, that wouldn't really be a bottleneck in the case of Changesets - so we could just favor less dependencies over a more performans library here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #1953 +/- ##
==========================================
+ Coverage 81.90% 81.99% +0.09%
==========================================
Files 54 54
Lines 2432 2461 +29
Branches 706 718 +12
==========================================
+ Hits 1992 2018 +26
- Misses 398 400 +2
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I updated import picomatch from 'picomatch'
import micromatch from 'micromatch'
const pkgs = ['pkg-a', 'pkg-b', '@scope/pkg-c', '@scope/pkg-d']
const globs = ['pkg-*', '!pkg-b', '@scope/*']
const matcher = picomatch(globs)
console.log('picomatch:', pkgs.filter((pkg) => matcher(pkg)))
// => [ 'pkg-a', 'pkg-b', '@scope/pkg-c', '@scope/pkg-d' ]
console.log('micromatch:', micromatch(pkgs, globs))
// => [ 'pkg-a', '@scope/pkg-c', '@scope/pkg-d' ]The negation is order-sensitive. picomatch simply returns true if any pattern returns true. |
|
Hm, shouldn't we inherit picomatch semantics though? We do mention what matcher library gets used under the hood - so people can reason about their glob patterns etc. Diverging from that could feel surprising. |
|
i would prefer leaving the behavior to picomatch, less code for us to worry about |
|
If we leave it entirely to picomatch, they'd have to craft every pattern to not include it, like Should we consider documenting instead? I also think the link to picomatch also mostly refers to its pattern syntax than the array matching behaviour. |
|
If we'd decide to use the builtin node API then this discussion would become obsolete :p I'm fine either way, so I'll leave you both to decide if we should use the node API or picomatch. |
|
If |
currently stuck on the default config no longer parsing