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

Add mergeMapConcurrently type definition #288

Conversation

gitusp
Copy link
Contributor

@gitusp gitusp commented Jun 17, 2019

mergeMapConcurrently seems to be missing its type definition.
I have checked that copying-and-pasting these definitions into my project's node_modules/@most/core/type-definitions/combinator/mergeConcurrently.d.ts seemed suggest types as documented, but not tested seriously.

@TylorS
Copy link
Member

TylorS commented Jun 17, 2019

Hey @gitusp thank you for taking the time to open this PR! I have a few small comments that I think will make these definitions ready for everyone.

  1. The 2nd and 3rd overload should be reversed. If 3 parameters are not given, TypeScript will always match the 2nd overload given that one will always be able to match the first argument. Here's a quick example of what I mean. In general you'll want the most initial params declared towards the top and the least initial params declared towards the bottom.

  2. After the current 2nd and 3rd overloads are switched, the then 3rd & 4th overloads likely should be merged in order to properly take advantage of them. Here's one more example of how the situation (after addressing the first point) that the types get us into when trying to use the parameters one-by-one. I believe the 3rd overload should become something like

export function mergeMapConcurrently<A, B>(f: (a: A) => Stream<B>): {
  (concurrency: number, s: Stream<A>) => Stream<B>
  (concurrency: number): (s: Stream<A>) => Stream<B>
}

This will allow TypeScript to follow the overloads like a tree structure and allow all valid use of the function.

@briancavalier
Copy link
Member

briancavalier commented Jun 17, 2019

Hey @gitusp, thanks! I just opened #291 to fix an issue with the mergeMapConcurrently flow defs. It uses the same partial application technique @TylorS recommends, so even though it's Flow rather than TS, may be a good reference.

It fixes `mergeMapConcurrently`'s type definitions to accept various
forms of calling curried function.
It applies the following reviews:
- mostjs#288 (comment)
- mostjs#288 (comment)
@gitusp
Copy link
Contributor Author

gitusp commented Jun 18, 2019

Thanks so much @TylorS and @briancavalier! I played around @TylorS's examples and confirmed what you mentioned. That was really good to know.

I ported @briancavalier's new flow defs into TypeScript and confirmed that the new definitions works on all the forms of partial application.

Before
Screen Shot 2019-06-18 at 10 16 55

After
Screen Shot 2019-06-18 at 10 17 07

Copy link
Member

@briancavalier briancavalier left a comment

Choose a reason for hiding this comment

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

👍 Looks good. Thanks!

@briancavalier briancavalier merged commit eab40c6 into mostjs:master Jun 18, 2019
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.

3 participants