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

Introduce xs.merge type declaration #82

Merged
merged 1 commit into from
Jul 20, 2016

Conversation

xtianjohns
Copy link
Contributor

Support type declarations when calling xs.merge(). This change allows consumers to leverage TypeScript to verify type safety when merging streams. It also brings merge in line with other static methods which already support this feature.

Description

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

There are two areas I think merit discussion.

First: I'm not exactly sure what the intended behavior of this method is when called without arguments. The interface says a stream is returned that emits items of any type. And I suppose that's true. However, when I call this without any arguments, I get a stream that doesn't emit anything ever (or complete). Obviously my fault as the caller, but perhaps we want to do something else when it's called without arguments.

Second: I don't know how to test this or if it deserves a test. I imported core into a test file and was able to get type support for the method.

Motivation and Context

This should resolve #80.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

s4: Stream<T4>,
s5: Stream<T5>,
s6: Stream<T6>): Stream<T1 | T2 | T3 | T4 | T5 | T6>;
(...stream: Array<Stream<any>>): Stream<any>;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the catch-all case should be

<T>(...stream: Array<Stream<T>>): Stream<T>;

Because that's what we had previously in the Stream class, under static merge etc.

@staltz
Copy link
Owner

staltz commented Jul 19, 2016

First: I'm not exactly sure what the intended behavior of this method is when called without arguments. The interface says a stream is returned that emits items of any type. And I suppose that's true. However, when I call this without any arguments, I get a stream that doesn't emit anything ever (or complete). Obviously my fault as the caller, but perhaps we want to do something else when it's called without arguments.

As far as this PR is concerned, I don't think you need to change anything. It would be a breaking change, and we don't know if we want to do that.

Second: I don't know how to test this or if it deserves a test. I imported core into a test file and was able to get type support for the method.

Check this test for an inspiration: https://github.com/staltz/xstream/blob/master/tests/factory/combine.ts#L26

The trick is to define the type of the variable, e.g. const combined: Stream<[string, string]> at the same time as assigning to it. TypeScript compilation will fail (hence the test will fail) if the right-hand side of the assignment doesn't match the expected type on the left-hand side.

Nitpick: the commit message should follow our convention. Just use npm run commit instead of git commit. But anyway I can easily rename the commit myself.

PS: good PR. :)

@xtianjohns
Copy link
Contributor Author

No problem - I'll get those fixed up this afternoon. Thanks!

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.
@xtianjohns
Copy link
Contributor Author

Alright, the catch all signature should look better now. I'm not sure what happened there.

Commit message should be improved, and when I amended the commit I threw in a test.

@staltz
Copy link
Owner

staltz commented Jul 20, 2016

Perfect! Thanks!

@staltz staltz merged commit 5327cb0 into staltz:master Jul 20, 2016
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.

Improve Stream.merge types
2 participants