Support negated ignore patterns

[webpro]

Fixes #32

This PR adds support for negated ignore patterns. Please see #32 for more details and use cases.

This is a solution that should not impact performance at all if no negated ignore patterns are used. However, it does require a patched picomatch, and I'm not sure whether there's interest in a PR upstream. But before I'm even trying to go there I wanted to gauge interest here.

The idea/patch: we could leverage picomatch's onIgnore function to un-ignore if ignored matches also match a negated ignore pattern.

The problem: it requires to un-ignore matches within picomatch and this is a relatively low-impact solution for picomatch, but I'm afraid even requiring an explicit true (or false?) return value might still be a breaking change looking at the scale it's being used. Yet at the same time it could also actually mean a fix in certain cases and removing a major hurdle when looking to migrate from globby to tinyglobby.

This PR still needs some work:

• Should we pass result.output to unignoreMatcher? I also saw result.input and I'm not sure about the difference.
• Another thing that needs work: the options to pass to picomatch when creating unignoreMatcher.

[pkg-pr-new]

Open in Stackblitz

npm i https://pkg.pr.new/tinyglobby@70

commit: b603afc

[SuperchupuDev]

oh, wow. first of all, thanks a lot for taking your time into implementing this, such a nice solution. how would patching picomatch work for users who download tinyglobby from npm? sadly, i don't think patching it is viable long term, but we can figure out something else or send a PR to picomatch

[webpro]

how would patching picomatch work for users who download tinyglobby from npm? sadly, i don't think patching it is viable long term

Oh no, bad idea indeed, this wasn't my intention at all

but we can figure out something else or send a PR to picomatch

Yes, this PR is mostly to keep the conversation going. And for you and others the ability to actually try it out e.g. when looking to migrate away from globby and see if this is a viable route.

Would you be interested in this solution given that picomatch would accept my PR? Tbh I don't fancy publishing (and maintaining) a fork of picomatch, so I'm not sure what other options we have here.

[SuperchupuDev]

i think i'm fine with anything that doesn't involve patching, whether it is a picomatch PR or something more complex

[webpro]

Sorry I phrased that poorly. With "this solution" I meant this PR without the patch part. Thanks! WIP.

[SuperchupuDev]

i think so? i think i can see some things that could be refactored (unsure though) and probably needs a small cleanup when that happens but other than that yes

[webpro]

Here's the PR: micromatch/picomatch#137

[benmccann]

When this is ready we should test it against cspell to make sure we don't break it since it uses a negated ignore pattern and the behavior here will be changing

[webpro]

Rebased the changes in this PR on top of main after the recent updates there.

[webpro]

Since the picomatch repo doesn't seem to have a lot of motion anymore I'd be OK with using my fork unmatch (which is in this PR already).

When this is ready we should test it against cspell to make sure we don't break it since it uses a negated ignore pattern and the behavior here will be changing

Sometimes a fix means a breaking change, and that's OK? As long as we're clear about it.

[benmccann]

I'm not sure I like the idea of using a fork of picomatch. picomatch itself dedupes really well. If we use a fork then it won't dedupe. It might be interesting to try also supporting Node's built-in matcher as an optional second matcher since that doesn't add a dep and it will be in all LTS in April

[webpro]

I'm not sure I like the idea of using a fork of picomatch.

Forking is some sort of last resort. Rest assured I don't fancy it either, but I'm OK with maintaining it so users can enjoy tinyglobby to the greatest extend possible as I believe it's a great initiative.

Then I guess the alternative is to be more transparent about the fact that tinyglobby isn't a drop-in replacement of globby. Would also love to start using tinyglobby in Knip and other projects, and I could imagine plenty of maintainers relying on globby to be in the same boat. Globby features the gitignore option since v7 came out in 2017 (yet it has always been false by default).

This PR with the fork does not even add support for .gitignore files directly, but with it, at least it's possible to get the behavior that many users may expect/need (i.e. by converting .gitignore patterns to ignore glob patterns). Eventually this should lead to the best of both worlds: all of the functionality with great performance.

picomatch itself dedupes really well.

Do you think by relying on picomatch the situation will improve overall? Different users value different aspects.

It might be interesting to try also supporting Node's built-in matcher as an optional second matcher since that doesn't add a dep and it will be in all LTS in April

Indeed, the Node.js built-in glob matcher has an exclude option that can be a function, let's see if and how we can leverage that and wait for Node.js and other runtimes to have that in LTS.

Could easily agree using a fork is a price too high to pay. But then in the meantime, let's update the docs with the fact that tinyglobby does not have an option to respect .gitignore files (i.e. does not have the gitignore option)?

[benmccann]

Is this PR enough to use tinyglobby in knip? You need streaming support first, right? (#24). Perhaps we should focus there first as that's needed by multiple projects

[Torathion]

Tbh, the unmatch fork could easily be justified by implementing the open pull requests of picomatch, namely typescript rewrite and esm support, while also pointing out the problem that the last real update is already over a year old and activity has died down. I would love to help extend unmatch with additional micro-optimizations and bundle size reductions as well, as I have done here. But only if we end up using this fork.

[webpro]

Is this PR enough to use tinyglobby in knip? You need streaming support first, right? (#24). Perhaps we should focus there first as that's needed by multiple projects

This isn't about Knip, but rather about whether tinyglobby wants to be a fully featured replacement for globby. Here's a GitHub Search to give a very rough idea of usage: https://github.com/search?q=globby+%22gitignore%3A+true%22+%28language%3ATypeScript+OR+language%3AJavaScript%29&type=code

I appreciate the team has the information to decide on their own terms. My offer to maintain and improve the fork still stands whenever you might need it.

[Torathion]

globby uses some insane and slow workarounds for the gitignore option, but it essentially uses ignore under the hood. ignore warns about: "Pay ATTENTION that minimatch (which used by fstream-ignore) does not follow the gitignore spec."

So it's either:

• add ignore as a dependency, which would add 4.3kb to the final package size according to bundlephobia
• use the fork that combines both picomatch and ignore

[benmccann]

I've filed an issue to discuss the gitignore issue: #92