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

Use micromatch & glob-parent instead of minimatch & glob2base #56

Merged
merged 3 commits into from
Nov 17, 2015

Conversation

es128
Copy link
Contributor

@es128 es128 commented Nov 9, 2015

Resolves #44

Let me know if you'd prefer I split this into two separate PRs

glob-parent v2 achieves parity with glob2base's handling of non-glob paths (stripping the filename) with a simpler algorithm and better performance. The only difference between them now is the trailing slash in the result, resolved simply enough here. I also added the patterns from the glob2base tests to the glob-parent repo to provide some extra assurance.

The caveat to swapping micromatch for minimatch here is that it only applies to negative globs. Positive globs are still just passed to node-glob, which of course continues to use micromatch. This does introduce an increased chance of new edge cases and inconsistencies which should be taken into consideration. But chokidar has been living with a similar inconsistency for a quite a while now (minimatch via readdirp for file tree walking and micromatch within chokidar for event filtering) and it hasn't been the source of any notable problems. (I also do have something in the works that will be able to replace node-glob and readdirp to resolve this, but that isn't an available solution for today)

@phated
Copy link
Member

phated commented Nov 9, 2015

👍 do you think this warrants a major bump?

@es128
Copy link
Contributor Author

es128 commented Nov 9, 2015

Depends on your semver interpretation. IMO not really because API and behavior haven't changed except in some rare cases that could likely be chalked up to being bugs in minimatch.

Downstream users likely want to stay aligned with whatever gulp is using, but I suppose the counter-argument is that not bumping major could possibly upset users who fall outside that characterization. Some people get touchy about what their extended dep tree looks like - in my experience a very small, but sometimes vocal minority.

Take note that in contrast to minimatch, micromatch takes the micro-module composition approach and has quite a few more deps - most of them by @jonschlinkert.

@yocontra
Copy link
Member

yocontra commented Nov 9, 2015

If the usage and behavior didn't change then no need for a major bump

@phated
Copy link
Member

phated commented Nov 9, 2015

The edge cases concern me.

@yocontra
Copy link
Member

yocontra commented Nov 9, 2015

@phated What are the edge cases? If there is usage not covered in our tests we should add those

@phated
Copy link
Member

phated commented Nov 9, 2015

@contra

This does introduce an increased chance of new edge cases and inconsistencies which should be taken into consideration.

I'm taking the increased chance for edge cases into consideration. Completely swapping out underlying libraries usually makes me weary of not bumping major.

@phated
Copy link
Member

phated commented Nov 9, 2015

To be clear, I don't know of any cases that might crop up.

@jonschlinkert
Copy link
Contributor

just wanted to let you know you can ping me any time if you have questions or need support with this.

fwiw I'm always afraid of edge cases. I think we have close to 1,500 patterns/test cases in micromatch, but we've still had 3 or 4 edge cases (I think all or most were windows related), but more will crop up from time to time as they do with minimatch as well.

in case this helps, we've only had a handful of issues with ~11 million downloads since micromatch was released earlier this year. the vast majority of downloads come from gulp-watch, which, from a learning curve standpoint has been great since watch configs tend to produce some interesting glob patterns and has helped weed out some edge cases.

phated added a commit that referenced this pull request Nov 17, 2015
Use micromatch & glob-parent instead of minimatch & glob2base
@phated phated merged commit b1a8de6 into gulpjs:master Nov 17, 2015
@phated
Copy link
Member

phated commented Nov 17, 2015

@es128 thanks!

@es128 es128 deleted the swap-deps branch November 17, 2015 00:25
@thepian
Copy link

thepian commented Nov 19, 2015

super nice work

phated added a commit that referenced this pull request Feb 16, 2017
Use micromatch & glob-parent instead of minimatch & glob2base
phated added a commit that referenced this pull request Feb 16, 2017
Use micromatch & glob-parent instead of minimatch & glob2base
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.

Evaluate Micromatch
5 participants