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

Added spreadability to sample #412

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

RamIdeas
Copy link
Contributor

This change adds a spreadable overload for sample.

This allows an arbitrary number of streams (more than the 2-5 currently typed) to be sampled, which matches the implementation.

If you have an array of streams with a common type, you can also spread that into the call and expect typing to flow through.

Further, I've added spreadability to all overloads as there can commonly be scenarios where you want to sample distinct streams and then a bunch of common streams.

@briancavalier
Copy link
Member

My knowledge of best practices in TS is pretty limited, so sorry if my questions are off base.

Do we need to add the trailing varargs to all of the variants, or only to the highest arity variant? For example, in my limited TS knowledge, it seems like adding the trailing var args only to this variant would cover all the cases, and calling it with fewer than 4 args would be handled by one of the other fixed arity variants.

I hope that made sense!

@RamIdeas
Copy link
Contributor Author

RamIdeas commented Feb 23, 2017

That's what I had originally, as you can see in first commit of this pull request.

But I had a scenario where I sampled the sampler and an array of streams like this:

const sampler: Stream<A> = getSampler();
const streams: Stream<B>[] = getStreams();
sampler.sample( (a, ...b) => {}, sampler, ...streams );

I know the varargs make it look a bit messy, but it was nice to get typings in this scenario.

Edit: Removed confusing '...' placeholder (I didn't see the clash - oops!)

@briancavalier
Copy link
Member

Ah, interesting. So, if I understand correctly, TS isn't able to match the types of the spreaded array with the parameter types of fixed arity variants. Or to say it another way, TS will only match the types of an actual spreaded array argument with those of formal rest parameters. Is that right?

@RamIdeas
Copy link
Contributor Author

Ah, interesting. So, if I understand correctly, TS isn't able to match the types of the spreaded array with the parameter types of fixed arity variants.

That's right, I don't think it can. (But it should be able to in certain cases so I may raise an issue there - but that's unrelated to this)

Or to say it another way, TS will only match the types of an actual spreaded array argument with those of formal rest parameters. Is that right?

Not exactly. It will try to find a common super type of any arguments passed in, whether individual or spread, and assign that type to the function rest parameters.

NB: I hope I've used 'parameters' and 'arguments' correctly. 'Arguments' being those passed into sample, and 'parameters' being those taken by the function which is the first argument passed to sample.

@briancavalier
Copy link
Member

I hope I've used 'parameters' and 'arguments' correctly

Yep, you have. Thanks for the explanations.

If this improves the overall usability for TS users, which it seems to do, and is the simplest solution, I'm in favor. However, I'll defer to @TylorS's superior TS knowledge.

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.

2 participants