gulpjs / glob-stream

Readable streamx interface over anymatch.
MIT License
178 stars 53 forks source link

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

Closed es128 closed 8 years ago

es128 commented 8 years ago

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 commented 8 years ago

:+1: do you think this warrants a major bump?

es128 commented 8 years ago

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 commented 8 years ago

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

phated commented 8 years ago

The edge cases concern me.

yocontra commented 8 years ago

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

phated commented 8 years ago

@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 commented 8 years ago

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

jonschlinkert commented 8 years ago

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 commented 8 years ago

@es128 thanks!

thepian commented 8 years ago

super nice work