Improve type inference for the listener callback

[stof]

Instead of using a generic Event type for the argument, the map of event types to the corresponding Event object types is used by the inference.

This is a BC break for projects specifying the generic types explicitly, as it changes the generic signature. For project relying on type inference, the inferred type becomes more specific.

[fregante]

I'm not sure of how this is different. If you pass FocusEvent to v2, isn't it exactly the same as passing 'focus' here?

[stof]

the difference is that delegate(element, '.my-class', 'click', function (event) {}) will automatically infer the type of event without having to specify it, based on the click event type.
Type inference is all about not passing the type explicitly.

[fregante]

That would be good if it works, but I just tried it locally and it doesn't seem to infer the type:

Inferred ❌

Specified ✅

[stof]

what happens if you run delegate(document, '.js-comment-field, #commit-description-textarea', 'keydown', event => { ... } ? Maybe typescript enables the type inference only when the generic is not specified at all and keeps the default types for partially specified ones.

[fregante]

That's strange:

I'm not sure I like that this forces me to write <HTMLTextAreaElement, 'keydown'> though. . Can you find out why this happens or if there's a way around it?

[stof]

Well, my best guess is that the type inference for generic types is done only when not specifying it at all, but not when specifying part of it and not next ones.

Btw, with the existing code, delegate<HTMLTextarea>(...) would then still report it asdelegate.Event(Event, HTMLTextAreaElement) and not as KeyboardEvent. So I'm not sure whether the partial case is worse then before (the IDE shows a big union type, but all types in it are extending Event which is also part of the union)

[stof]

For the record, my own usage is that I write JavaScript code, not TypeScript code. But I still rely on typescript type definitions for type checking (tsc --checkJS) and IDE intellisense (both PHPStorm and VS Code are using them).
As JS code cannot specify generic types for function calls, my own usage always goes to the delegate(document, '.js-comment-field, #commit-description-textarea', 'keydown', event => { ... }) syntax which benefits from the improvement.
I opened an issue in the TypeScript issue tracker to see whether a future version of TS could enable type inference for partially-specified generics.

[fregante]

I want to merge this but is it possible to achieve the same effect without changing the generic? For 2 reasons:

• ugly <HTMLTextAreaElement, 'keydown'> due to Proposal: Partial Type Argument Inference microsoft/TypeScript#26242
• this would be a breaking change and it's a very minor improvement to justify it for now, so it'd probably wait here for a long while before being merged

[stof]

Well, I don't think it is possible without changing it. The change of generic is precisely what allows the event type argument to specify the event class in the generic.

[fregante]

Can you change it so the generic takes 2 strings? At least it's both consistent and shorter

Before

<HTMLTextAreaElement, KeyboardEvent>

This PR

<HTMLTextAreaElement, 'keydown'>

Suggested

<'textarea', 'keydown'>

[stof]

@fregante your suggestion using tag names rather than the element type will be harder to implement: HTML and SVG elements are in separate maps in the dom lib of typescript. And they even have some common tag names associated to different element types, which would then forbid using one of them. And things would also become harder to support custom elements.

Currently, the library supports any kind of Element without any additional need to register the custom element types in a special way. And this will not improve type inference AFAICT (maybe even regress it)

[fregante]

It can be done, querySelector maps tag selectors to their constructors and in my select-dom I use a dependency to do the same (but supporting whole selectors), but yeah it might be a little tough. However it’s basically the same concept as this change, nobody guarantees that an input event is actually InputEvent and not a CustomEvent

I’ll test this in my other project and I’ll merge it as is for now. Then I’ll think if it’s worth the other part of the change.

[fregante]

💙

[fregante]

Now that I think of it, I could use the same package exactly the same way since this module also takes a selector. This way no one probably even needs to specify anything via generics.

[fregante]

Thank you for this and sorry for the long wait! I'll open a new issue for the other part of the feature