Add typings for tapable #39
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
=======================================
Coverage 91.04% 91.04%
=======================================
Files 14 14
Lines 413 413
Branches 73 73
=======================================
Hits 376 376
Misses 34 34
Partials 3 3Continue to review full report at Codecov.
|
|
@HerringtonDarkholme This looks so great thank you!!!! I have a question for you. Would we solve the variadic issue if we instead passed an object with those parameters, and to make it easy to pull into the cc @sokra |
|
@TheLarkInn Yes, it will. // use generic annotation
const hook = new SyncHook<{source: string, target: string, routeList: List<Route>}>()
// or use runtime object as declaration
// const hook = new SyncHook({ source: String, target: String, routeList: List })
hook.call({source, target, routeList}) // checked against the type declared above
hook.tap("MapPlugin", ({source, target, routeList}) => { // argument type inferred
const route = maps.findRoute(srouce,target)
routeList.add(route)
})Everything checked. |
|
I see the problem. As the current TypeScript lacks generic overloads you could do it like this: type AsyncHook2<A, B> = {
tap(name: string, fn: (a: A, b: B) => void) : void;
tapAsync(name: string, fn: (a: A, b: B, callback: (err?: Error) => void) => void) : void;
tapPromise(name: string, fn: (a: A, b: B) => Promise<void>) : void;
callAsync(a: A, b: B, callback: (err?: Error) => void): void
promise(a: A, b: B): Promise<void>;
}
type AsyncHook1<A> = {
tap(name: string, fn: (a: A) => void) : void;
tapAsync(name: string, fn: (a: A, callback: (err?: Error) => void) => void) : void;
tapPromise(name: string, fn: (a: A) => Promise<void>) : void;
callAsync(a: A, callback: (err?: Error) => void): void
promise(a: A): Promise<void>;
}As this lacks argument names 😢 it would be cool if TypeScript supports this: type AsyncHook<T> {
tap(fn: ([P in keyof T]: T[P]) => void): void
tapAsync(fn: ([P in keyof T]: T[P], callback: (err?: Error) => void) => void): void
tapPromise(fn: ([P in keyof T]: T[P]) => Promise<void>): void
callAsync([P in keyof T]: T[P], callback: (err?: Error) => void): void;
promise([P in keyof T]: T[P]): Promise<void>;
}
x = {} as AsyncHook<{source: string, target: string, routeList: List<Route>}>Maybe we propose this syntax. Should be easier to implement than variadic generics |
|
@sokra Thanks for the reply. However, I don't think I would still vote for variadic generics. |
|
@HerringtonDarkholme I had not realized it when I originally mentioned the object syntax as an alternative but @sokra reminded me that in this case, object creation would be pretty costly and slow (and given that this library is passing around a bunch of these in the 10s-100s of thousands of times, it potentially could add up. Maybe there is something I can key in @DanielRosenwasser about |
|
I was hoping object allocation wouldn't be a problem. I guess it is... 😅 |
It's officially ordered since es2015.
Fine for me too. Whatever solves the issue. |
|
You could also use declarations for automatically inferring the variadic arguments. I personally think that this would be a really elegant solution that doesn't change much of the implementation details. This is based off how the TypeScript team implemented their DOM EventMap types and the ideas sokra mentioned. type AsyncHook3<A, B, C> = {
tap(name: string, fn: (a: A, b: B, c: C) => void) : void;
tapAsync(name: string, fn: (a: A, b: B, c: C, callback: (err?: Error) => void) => void) : void;
tapPromise(name: string, fn: (a: A, b: B, c: C) => Promise<void>) : void;
callAsync(a: A, b: B, c: C, callback: (err?: Error) => void): void
promise(a: A, b: B, c: C): Promise<void>;
}
type AsyncHook2<A, B> = {
tap(name: string, fn: (a: A, b: B) => void) : void;
tapAsync(name: string, fn: (a: A, b: B, callback: (err?: Error) => void) => void) : void;
tapPromise(name: string, fn: (a: A, b: B) => Promise<void>) : void;
callAsync(a: A, b: B, callback: (err?: Error) => void): void
promise(a: A, b: B): Promise<void>;
}
type AsyncHook1<A> = {
tap(name: string, fn: (a: A) => void) : void;
tapAsync(name: string, fn: (a: A, callback: (err?: Error) => void) => void) : void;
tapPromise(name: string, fn: (a: A) => Promise<void>) : void;
callAsync(a: A, callback: (err?: Error) => void): void
promise(a: A): Promise<void>;
}
// the secret sauce
declare function asyncHook<A>(args?: [string]): AsyncHook1<A>;
declare function asyncHook<A, B>(args?: [string, string]): AsyncHook2<A, B>;
declare function asyncHook<A, B, C>(args?: [string, string, string]): AsyncHook3<A, B, C>;
asyncHook<number>(["count"]).callAsync(100, function () {})
// same thing wrong types
asyncHook<number>(["count"]).callAsync("100", function () {})
asyncHook<number, {delta: number}>(["count", "timing"])
.callAsync(100, {delta: 100}, function () {})
// wrong number of names
asyncHook<number, string, number[]>(["number", "name"])
// right number of names
const async4 = asyncHook<number, string, number[]>(["count", "name", "tries"]);
// wrong number of args
async4.callAsync(100, "percent", function () {})
// right number of args
async4.callAsync(100, "percent", [100], function () {})For each of the comments labeled "wrong," TypeScript will throw compiler errors. |
|
@sokra, @HerringtonDarkholme What are your thoughts here on @colelawrence 's idea. It feels like this might be suitable to me. |
|
Overloading is a good solution. Current implementation is using |
|
Why can Can we do something like this? this.hooks = {
/** @type {AsyncHook1<Compiler>} */
myHook: new AsyncHook(["compiler"])
}; |
Sorry, let me clarify it. Currently To specify the concise arity of So @colelawrence uses a different approach: overload one plain function to return different types e.g. |
|
We might be able to still support using `new` for backwards compatibility,
but it would have imprecise typing. Then, just calling the function would
result in precise types.
<Edit: Never mind, I think we can support it. See next comment>
…On Mon, Dec 11, 2017, 1:38 AM Herrington Darkholme ***@***.***> wrote:
Why can new not be overloaded?
Sorry, let me clarify it. new is overloadable in terms of argument types,
but it cannot be overloaded to return different type.
Currently Hook has only one type constructor which has eight type
parameter. So we cannot specify how many arguments call or tap will
receive.
To specify the concise arity of call / tap, we need to have several type
constructors with different number of type arguments.
So @colelawrence <https://github.com/colelawrence> uses a different
approach: overload one plain function to return different types e.g.
AsyncHook1 / AsyncHook2 ...
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACyjUwMMM2C2FA9HF3Kwn-wsUVWurMhqks5s_NwLgaJpZM4QTb5x>
.
|
|
With a little fiddling, I think I found a way for us to do the same thing with new. // the secret sauce
interface AsyncHookable {
new <A> (args?: [string]): AsyncHook1<A>
new <A, B>(args?: [string, string]): AsyncHook2<A, B>
new <A, B, C>(args?: [string, string, string]): AsyncHook3<A, B, C>
}
// the magic
declare const AsyncHook: AsyncHookable
let a1 = new AsyncHook(["hello"])
new AsyncHook<number>(["hello"]).callAsync(100, function () {})
new AsyncHook<number, {time: number}>(["hello", "timing"])
.callAsync(100, {time: 100}, function () {})
// wrong number of names
new AsyncHook<number, string, number[]>(["number", "name"])
// right number of names
const async4 = new AsyncHook<number, string, number[]>(["number", "name", "number"]);
// wrong number of args
async4.callAsync(100, "hi", function () {})
// right number of args
async4.callAsync(100, "hi", [100], function () {})
// same thing wrong types
new AsyncHook<string>(["hello"]).callAsync(100, function () {})Here, I want to demonstrate that we can "overload" the I'm thinking out loud a little bit, here. |
|
Construct signatures can be overloaded, but you can't use So the problem is really that you can describe some of these things to TypeScript, but you can't extends from these types and you can't easily implement them. |
|
@colelawrence Your approach looks great. |
|
@HerringtonDarkholme Would you be willing to make the following suggestions from @colelawrence 's feedback? Looking forward to land this :-D |
|
I just noticed tapable has new documentation shipped and I have read it. I also tried to read the source code and test case. But it turns out compiling is hard to grok... I believe @colelawrence 's approach is feasible. However I think it should be better if some one more familiar with tapable can add the typing. |
|
No problems at all. @colelawrence are you open to PR? |
|
Sure, Sean.
I'll start working on declarations over the next couple days and report
back with a PR.
…On Fri, Dec 22, 2017, 2:13 PM Sean Larkin ***@***.***> wrote:
No problems at all. @colelawrence <https://github.com/colelawrence> are
you open to PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#39 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACyjUxCenhZJF566hz_rwk5iN0Wdh0t8ks5tDBtngaJpZM4QTb5x>
.
|
|
Thank @colelawrence for taking over this. Sorry about not shipping this. I got quite busy these days so I cannot follow this. Closing. |
This pull request is for typing tapable's new API.
The typing is shipped within the package of tapable, rather than on DT. This is better in terms of developer experience since users only need to install one package.
The typing is summarized from both README and source code. If something is wrong, please tell me!
For implementation detail, I'm using generic default to provide sensible type inference and moderate checking. Tapable's new API uses variadic arguments a lot, which is hard to model in type system. (Not that hard if microsoft/TypeScript#5453 is implemented).
I have to pack more type parameters into one class in order to support multiple argument types. So users can write
const hook = new SyncHook<string>(["param"]), andhook.call('param')will be correctly typed. On the other hand, passing more arguments won't trigger compiling error, so callback function can be used.We can, however, make the export API as function instead of class. TtypeScript/FlowType can support overloaded function, so we can add to one hook more signatures that return different types respectively.
cc @TheLarkInn