Closed kevinbeal closed 6 years ago
That makes perfect sense. It will be implemented in the next version, which should be ready this week.
I really appreciate you taking the time to answer my issue and update the types file, but could I bother you to do it again? 😅
debounce<T>(fn: Function, ms: number): T
has an advantage over the previous definition, but it has a disadvantage over
debounce<T>(fn: T, ms: number): T
In the first example, if you want type safety, you have to define T
or else it assumes that T === {}
instead of a function. (Demonstration here%20as%20%3CT%3E(fn%3A%20Function%2C%20ms%3A%20number)%20%3D%3E%20T)((s%3A%20string)%20%3D%3E%205%2C%206)%3B%0D%0Aconst%20r%20%3D%20v('string')%20%2B%205%0D%0A%0D%0A))
The way it was could be considered better that the current definition in that at least it would know (without adding type information) that the result was a function.
The second definition I wrote (above w/ fn: T
) has an advantage over the old and current definitions in that no additional type information need be provided for type safety. The function passed in is assumed to be passed right on through as the return value.
I realized later though that my solution has a disadvantage of its own. If the function passed into the debounce has a return value other than void
, then the debounced version of that function will think the return value will be the same when it's actually supposed to be void
.
Assuming that the return value becomes void (or undefined) when a function becomes debounced (or throttled), then the function passed in would need to have the return value type information updated to void
.
Since Typescript 2.8 you can do something like this (taken from here):
type ArgumentTypes<T> = T extends (... args: infer U ) => infer R ? U: never;
type ReplaceReturnType<T, TNewReturn> = (...a: ArgumentTypes<T>) => TNewReturn;
interface X {
debounce<T>(fn: T, ms: number): ReplaceReturnType<T, void>;
throttle<T>(fn: T, ms: number): ReplaceReturnType<T, void>;
}
Demonstration of how it works here%20as%20any)%20as%20X%5B'throttle'%5D)%3B%20%2F%2F%20%3C-%20trickery%20for%20demonstration%20purposes%20only%0D%0Aconst%20throttledFn%20%3D%20throttle(myFn%2C%2010)%3B%0D%0A%0D%0AthrottledFn(3)%20%2F%2F%20%3C-%20won't%20accept%20number%0D%0AthrottledFn('')%20%2F%2F%20%3C-%20takes%20correct%20argument%0D%0AthrottledFn(''%2C%20'')%20%2F%2F%20%3C-%20rejects%20incorrect%20number%20of%20arguments%0D%0A%0D%0AmyFn('')%20%3D%3D%3D%205%20%2F%2F%20%3C-%20original%20return%20value%20is%20number%0D%0AthrottledFn('')%20%3D%3D%3D%205%20%2F%2F%20%3C-%20new%20return%20value%20not%20number%0D%0AthrottledFn('')%20%2B%205%0D%0AthrottledFn('')%20%3D%3D%3D%20undefined%20%2F%2F%20%3C-%20new%20return%20value%20is%20undefined%0D%0A%0D%0A%0D%0A).
If you can afford to support Typescript 2.8+, then I think that last solution is best, otherwise, the one with (fn: T, n: number): T
assuming that the functions passed in will typically not have return values since the whole point of a debounced or throttled function is to produce side effects.
Just my opinion.
Thanks for providing a cool library :)
You really know your Typescript
. I cannot say that I am proficient there. I use it how I use Webpack
- make it work, use its advantages and move on.
I am waiting the variadic types issue to be resolved and this will make compose
and pipe
easier to use with Typescript
.
I appreciate your nice words for the library. I will implement the suggested solution(Typescript 2.8+) until the end of the week. I will close the issue, when the version including the above changes are implemented.
Also, it would be awesome if the definition for flatten
was updated too.
Right now it's flatten<T>(x: T[] | T[][]): T[]
but T[][]
is an example of T[]
.
The consequence is that as long as T[]
is the return type and also the type of x
, typescript can't know that the result is flattened. It assumes that if x = [[1], 1]
then the return type is Array<number|number[]>
.
I think this would be better:
interface X {
flatten<T>(x: Array<T[]|T>): T[];
}
Demonstration here%0D%0Aconst%20flatten2%20%3D%20((1%20as%20any)%20as%20X%5B'flatten2'%5D)%0D%0Aconst%20flatten3%20%3D%20((1%20as%20any)%20as%20X%5B'flatten3'%5D)%0D%0Aconst%20myList%20%3D%20%5B%5B1%2C%201%5D%2C%201%5D%3B%0D%0A%0D%0Aflatten(myList).forEach(n%20%3D%3E%20n.toFixed)%20%20%2F%2F%20%3C-%20assumed%20to%20be%20an%20array%20of%20arrays%0D%0Aflatten2(myList).forEach(n%20%3D%3E%20n.toFixed)%20%2F%2F%20%3C-%20assumed%20to%20be%20array%20of%20numbers%0D%0Aflatten3(myList).forEach(n%20%3D%3E%20n.toFixed)%20%2F%2F%20%3C-%20assumed%20to%20be%20array%20of%20numbers%0D%0A). (flatten3
being the best option).
You are quite right. flatten
can be improved and your suggestion is spot on. I will commit the change, but you will be able to use it when new version is ready.
Version 0.12.6
holds all your suggestions.
I am closing the issue, but feel free to reopen it if necessary.
It would be great if the definition were more like this:
throttle<T>(fn: T, time: number): T;
Casting the first argument and return type as
Function
helps very little.