effection icon indicating copy to clipboard operation
effection copied to clipboard

Add stream merge function

Open jnicklas opened this issue 4 years ago • 3 comments

This allows us to merge multiple streams of the same name into one stream. This is useful in many cases where streams from multiple sources need to be combined.

For example, to combine the stdout and stderr streams of a process, we could do this:

let myProcess = yield exec('./my-program');
let output = Stream.merge([myProcess.stdout, myProcess.stderr]);

jnicklas avatar Sep 23 '21 11:09 jnicklas

what about stream.merge() as in myProcess.stderr.merge(myProcess.stdout).forEach()

Also, is there the requirement that they all be of the same type? seems to me that merging Stream<A> and Stream<B> would result in Stream<A|B> with the nice side effect that if all streams share a type then, the resulting merged stream will be of that type.

cowboyd avatar Sep 23 '21 13:09 cowboyd

what about stream.merge() as in myProcess.stderr.merge(myProcess.stdout).forEach()

Totally an option. I preferred the function style, since no stream is preferred over the other, so the result looks more symmetrical to me. It also feels somewhat similar to our all and race functions. That being said, I don't have a very strong opinion on this. If you'd prefer the method, I'm happy to change it.

Also, is there the requirement that they all be of the same type? seems to me that merging Stream<A> and Stream<B> would result in Stream<A|B> with the nice side effect that if all streams share a type then, the resulting merged stream will be of that type.

IIRC this does not work particularly well with the TypeScript type system. I tried doing this when implementing race, since it has similar typing, but was unable to achieve this.

jnicklas avatar Sep 24 '21 10:09 jnicklas

Totally an option. I preferred the function style, since no stream is preferred over the other, so the result looks more symmetrical to me. It also feels somewhat similar to our all and race functions. That being said, I don't have a very strong opinion on this. If you'd prefer the method, I'm happy to change it.

That makes sense. Currently we have a lot of combinators on stream, so it did strike me as a bit odd that this wouldn't also be available via the stream. But then again, the other combinators don't involve another stream. Having both is also an option with the standalone being the primitive. Still, let's hold off on that unless actual usage makes that feel needed.

In addition to on the Stream can we export merge() as a standalone function? It's not crucial at this point since there is only one, but it is good etiquette for tree-shakable libraries, and personally, I always prefer the option of consuming the function directly without an object intermediary.

As for the typing, it looks like it isn't smart enough to infer the union, but it will successfully type-check as-is if you explicitly annotate it. E.g.

export function concat<T>(arrays: T[][]) {
  return arrays.reduce((all, array) => all.concat(array),[] as T[]);
}

// typeschecks
let numsAndStrings = concat<number | string>([[1,2,3], ['hello', 'world'], [12]]);

Also, are we sure we want to throw away the return values like this? I don't have a use-case on hand where you would want to consume both (you can always still consume both individually), but wanted to at least bring it up.

cowboyd avatar Sep 27 '21 07:09 cowboyd

We've removed the stream helper functions from 3.0 in order to focus on core JavaScript compatibility. The idea is that this is a competency best elaborated by the community.

cowboyd avatar Dec 18 '23 19:12 cowboyd