Should glob-stream and globby be merged?

UltCombo commented 9 years ago

globby has the same core functionality as glob-stream, except the streaming part. Should these modules be merged, or globby become a dependency of glob-stream?

As far as I can see, globby lacks a streaming API and regex filtering logic. Once those are implemented, both packages should be nearly identical. Then it should be possible to merge their unit tests and that's about it. What do you guys think?

I've also considered keeping globby as non-streamy and glob-stream depend on it, but then globby would still need to implement a streaming or events API to be properly consumed by glob-stream. If globby implements a streaming API, then both packages would be nearly identical (globby with added non-streaming async and sync APIs).

//cc @contra @sindresorhus

yocontra commented 9 years ago

Why? glob-stream already had all of these features and better perf. What's the point in putting a bunch of time to turn globby into an identical module? I'm not seeing it

UltCombo commented 9 years ago

It is because every improvement I find must be pushed to two different packages, and that seems like a waste of time. sindresorhus/globby#6 fixes the perf issues.

yocontra commented 9 years ago

@UltCombo Who said you had to do that? I know I never did. I think you decided on your own to update globby to achieve parity with glob-stream

yocontra commented 9 years ago

If @sindresorhus wants to deprecate his module in favor of this one that is up to him, but from my perspective this one has more features.

UltCombo commented 9 years ago

@contra more precisely, I've updated globby to fix perf issues only so far. I haven't decided to achieve parity (yet), that is why I've opened the issue in the first place.

It would be nice if we could at least abstract away some common logic so that I don't have to push updates to both packages. But no hard feelings if that doesn't work out.

yocontra commented 9 years ago

@UltCombo Why are you applying the same changes to both though? I'm still not understanding your logic here. Which one do you use?

UltCombo commented 9 years ago

@contra I believe @sindresorhus will not deprecate globby as it has a very different API at the moment (async and sync instead of streaming).

yocontra commented 9 years ago

Can you explain in detail what your thought process is concerning why the modules should be merged? Like really walk me through it, I'm not grasping it yet

UltCombo commented 9 years ago

(hah, spaghetti replies order)

Why are you applying the same changes to both though?

Because there are improvements that apply to both. E.g. the Minimatch class usage for negative globs (wearefractal/glob-stream#30 and sindresorhus/globby#6). After thinking a bit more, a direct dependency relation most likely wouldn't work out, but such common logic could possibly be abstracted away to another module. (I haven't examined this throughout yet though)

Which one do you use?

Not sure what you mean. If you recall, I'm using gulp-src-ordered-globs, which is another package duplicating the very same logic applied to glob-stream and globby. I'm using gulp-src-ordered-globs because this logic hasn't landed on stable gulp yet, and gulp-src-ordered-globs wraps gulp.src() which uses the old glob-stream logic.

UltCombo commented 9 years ago

Oh I thought a comment was deleted but it was just out of order (different comment order when I opened this thread in another tab somehow).

Anyway, yes, a direct dependency on globby wouldn't work. I'd just like to abstract away the common pain points (extend node-glob to accept multiple ordered globs), so that the same code can be applied to glob-stream, globby and perhaps gulp-src-ordered-globs.

UltCombo commented 9 years ago

@contra

Can you explain in detail what your thought process is concerning why the modules should be merged? Like really walk me through it, I'm not grasping it yet

Here's my line of thought:

I thought globby (a node-glob extension with multiple patterns support) was bare bones enough that it could be used here, since they share some of the same core functionality (extend node-glob to support multiple patterns).
But for globby to be properly consumed here, it would need a streaming or events API.
By adding a streaming API to globby, then both packages would be pretty much identical.

Can you see my point now? (I'm slightly sick, not sure if my thoughts are making sense :P)

Anyway, you helped me realize that all of this would be pretty much useless (and we already had reached this conclusion in this thread as well).

But it would be really nice to be able to abstract away the "node-glob multiple ordered patterns" feature so that it can be consumed by both packages. This is what I was trying to achieve since the beginning, but ended up overcomplicating it (see line of thought bullet list above).

Both packages create a glob per positive match and then filter out all posterior negative globs. Right now, this is done via the Minimatch class, but soon enough node-glob will implement an ignore API and then I (or someone else) will have to PR this change to both glob-stream and globby. See what I mean?

yocontra commented 9 years ago

Yeah, I think that's fine though.

UltCombo commented 9 years ago

Yeah, it is not a huge thing, I just thought it'd be nice to have this common logic maintained in a single repository.

Though yeah, globby itself implements this logic in two different ways (for async and sync methods), and glob-stream consumes a complete differently API (node-glob's events API instead of the callback one), so an abstraction wouldn't work well.

UltCombo commented 9 years ago

If such an abstraction was made, it would most likely end up as another clone of glob-stream.

gulpjs / glob-stream

Should glob-stream and globby be merged? #31