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

Improve Stream.merge types #80

Closed
tp opened this issue Jul 15, 2016 · 4 comments · Fixed by #82
Closed

Improve Stream.merge types #80

tp opened this issue Jul 15, 2016 · 4 comments · Fixed by #82

Comments

@tp
Copy link

tp commented Jul 15, 2016

When using Stream.merge I found that I had to manually define the types since TS could not infer them with the current signature, which makes it of course error prone:

const merged = xs.Stream.merge<A | B>(a, b);

From what I have seen so far to get variadic-ish generically typed arguments supported, we could use a function signature like this:

static merge<T1, T2>(s1: Stream<T1>, s2: Stream<T2>): Stream<T1 | T2>;
static merge<T1, T2, T3>(s1: Stream<T1>, s2: Stream<T2>, s3: Stream<T3>): Stream<T1 | T2 | T3>;
[...]

(I am pretty sure this works in .d.ts files, not sure how to annotate a function written directly in TS code with this though)

Code like this is often generated for up to 20 or 30 arguments, and then provides full type safety.

@Hypnosphi
Copy link
Contributor

what is Stream.merge? Since 4.0.0, there's only xs.merge.

Anyway, why do you need to merge streams of different types?

@staltz
Copy link
Owner

staltz commented Jul 15, 2016

I agree with this suggestion, seems easy to do and we should do it.

@xtianjohns
Copy link
Contributor

I can jump on this if no one objects. Two questions:

  1. How far should we go when overloading the method? Rx goes to six, but I feel like I've seen them do more elsewhere.
  2. Should we one-line those overloads or try to keep them to the width throughout the rest of the project?

@staltz
Copy link
Owner

staltz commented Jul 17, 2016

@xtianjohns good! We should do like Combine has: https://github.com/staltz/xstream/blob/master/src/core.ts#L135

xtianjohns pushed a commit to xtianjohns/xstream that referenced this issue Jul 18, 2016
Modify static `merge` method to implement a new interface:
MergeSignature. This interface is constructed similarly to the
CombineSignature interface. Export the new interface.

This change allows consumers invoking the static method to declare types
and leverage TypeScript to verify type safety when merging streams. The
change brings `merge` in line with other static methods which already
include this feature.

Related to staltz#80.
xtianjohns pushed a commit to xtianjohns/xstream that referenced this issue Jul 19, 2016
Modify static `merge` method to implement a new interface:
MergeSignature. This interface is constructed similarly to the
CombineSignature interface. Export the new interface.

This change allows consumers invoking the static method to declare types
and leverage TypeScript to verify type safety when merging streams. The
change brings `merge` in line with other static methods which already
include this feature.

Should resolve staltz#80.
staltz pushed a commit that referenced this issue Jul 20, 2016
Allows xs.merge(streamOfNumbers, streamOfStrings) to return a Stream of numbers or strings, as the return type in TypeScript.

Modifis static `merge` method to implement a new interface:
MergeSignature. This interface is constructed similarly to the
CombineSignature interface. Export the new interface.

This change allows consumers invoking the static method to declare types
and leverage TypeScript to verify type safety when merging streams. The
change brings `merge` in line with other static methods which already
include this feature.

PR #82.

Should resolve #80.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants