-
Notifications
You must be signed in to change notification settings - Fork 207
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
base: master
Are you sure you want to change the base?
Conversation
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! |
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!) |
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? |
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)
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 |
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. |
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.