Change order of overloads so that user code doesn't need parameter type declarations.#12389
Conversation
|
angular/index.d.ts to author (@diegovilar). Could you review this PR? Checklist
d3-array/index.d.ts to authors (@gustavderdrache @borisyankov @tomwanzek). Could you review this PR? Checklist
flickity/index.d.ts to author (Chris McGrath (account can't be detected)). Could you review this PR? Checklist
fromjs/index.d.ts to author (@glenndierckx). Could you review this PR? Checklist
jasmine/index.d.ts to authors (@borisyankov @theodorejb @davidparsson). Could you review this PR? Checklist
mongoose-promise/index.d.ts to author (@simonxca). Could you review this PR? Checklist
mongoose/index.d.ts to authors (@simonxca @horiuchi). Could you review this PR? Checklist
multiplexjs/index.d.ts to author (@KamyarNazeri). Could you review this PR? Checklist
nightmare/index.d.ts to author (@horiuchi). Could you review this PR? Checklist
phantom/index.d.ts to authors (@horiuchi @llRandom). Could you review this PR? Checklist
pinkyswear/index.d.ts to author (@chances). Could you review this PR? Checklist
prelude-ls/index.d.ts to author (@AyaMorisawa). Could you review this PR? Checklist
webtorrent/index.d.ts to author (Bazyli Brzóska (account can't be detected)). Could you review this PR? Checklist
yargs/index.d.ts to authors (@poelstra @mizunashi-mana). Could you review this PR? Checklist
|
| * @param callback callback funtion to execute when event fires | ||
| */ | ||
| on(eventname: string, callback: (eventt?: Event, cellElement?: Element) => any) : void; | ||
| on(eventname: string, callback: (event?: Event, pointer?: Element | Touch, cellElement?: Element, cellIndex?: number) => any): void; |
There was a problem hiding this comment.
I've just changed the order of these, but these overloads don't really make sense -- they all take a string and a function, so how does the library know what kind of values to pass to the function? Can we use string literal types here? @clmcgrath
| close(): Promise<void>; | ||
|
|
||
| evaluate<R>(callback: () => R): Promise<R>; | ||
| evaluate<T>(callback: (arg: T) => void, arg: T): Promise<void>; |
There was a problem hiding this comment.
This is handled by the case following it.
| * Adds a listener to the complete (success) event. | ||
| * @deprecated Adds a listener to the complete (success) event. | ||
| */ | ||
| addCallback(listener: (arg: T) => void): this; |
There was a problem hiding this comment.
This is just (...args: T[]) where the length happens to be 1.
sandersn
left a comment
There was a problem hiding this comment.
These changes look technically correct, although you might want to keep some short overloads for documentation. Most people don't know that i =>i is assignable to cb: (item: T, key: TKey) => void, for example, so having redundant overloads can make that clearer.
|
We don't do that for e.g. |
|
It's up to you; VS and VS Code do put multiple overloads behind an up/down arrow interface. But Emacs only shows the first. I don't know what other editors do. |
|
@Andy-MS 👍 for the yargs changes. |
Undoes the changes in #12344 and changes the order of overload declarations instead.
Does not undo the change to
zepto-tests.tssince that would genuinely be an implicit any.