fix(fromEvent): improve typings for JQueryStyleEventEmitter #5584
Conversation
src/internal/observable/fromEvent.ts
Outdated
| @@ -20,8 +20,8 @@ export interface NodeCompatibleEventEmitter { | |||
| } | |||
|
|
|||
| export interface JQueryStyleEventEmitter { | |||
| on: (eventName: string, handler: Function) => void; | |||
| off: (eventName: string, handler: Function) => void; | |||
| on: (eventName: string, handler: NodeEventHandler) => void; | |||
There was a problem hiding this comment.
NodeEventHandler appears to be from @typings/node. @cartant, am I wrong in saying that this would make us dependent on @typings/node?
There was a problem hiding this comment.
Since that was already used in this file I thought I'd use it again. The important part is that it's (...args: any[]) => void, could create a local type for that.
There was a problem hiding this comment.
... this would make us dependent on
@typings/node?
Nah, this looks okay. The NodeEventHandler is declared in this file. I suppose this is okay, but I thought jQuery events used a single object parameter - more like the DOM event handlers - rather than multiple parameters. It has been some time since I've used jQuery, so IDK.
Whatever the situation, Function is wrong, here, as it's pretty much totally useless, these days, being declared like this:
interface Function {
/**
* Returns the name of the function. Function names are read-only and can not be changed.
*/
readonly name: string;
}
|
I'd feel a whole lot better about this change with some sort of dtslint tests or something. |
|
I've not written those before, does this work? |
|
@evertbouw Regarding the dtslint tests, once #5724 is merged, you'll find a The PR with the tests has been merged. If you rebase your PR and run them, we'll see where we're at, I suppose. |
|
As I mentioned here, I think the jQuery handler type could/should be changed to better reflect the actual jQuery typings - i.e. be more like Relevant types: However, this is jQuery, so it's not something about which I'm especially passionate. I've tweaked the jQuery types - in #5726 - in the dtslint test and I've commented out the jQuery test - as it fails with the more accurate jQuery types. After it's merged and after you rebase, you should be able to uncomment it and it should pass. |
|
Rebased and uncommented the test |
|
It seems these changes were somehow (inadvertently?) included in this PR: #5783 |
Description:
The handler in JQueryStyleEventEmitter is typed as a Function, meaning it takes no arguments. Overwriting this requires a pretty awkward cast.
Related issue (if exists):
#4496