add event bindings to no-incompatible-type-binding rule

[43081j]

here's why i wanted runem/web-component-analyzer#165

the types are always ANY and i had to put the jsdoc the wrong way around (compared to anywhere else i saw it in the wild at least).

so it doesn't do much at the min but here it is in case WCA starts picking those types up or you have a suggested way i can do it here

i wonder if it should also default to Event not any, but not sure how to do that with simpletype...

[43081j]

@runem is this still a thing you haven't done in one of your branches yourself? its been a while since i started work on it so its very possible the functionality just exists now.

if not, im happy to finish it off if you can point me in the right direction

[runem]

I haven't yet built functionality, so this PR is still relevant :-)

Here are my initial thoughts about what needs to be done in order to support this rule:

ts-simple-type

There is still an outstanding issue with how to handle lib-types. Right now, lib-types such as Date and Promise are their own ts-simple-types, but this is not a viable solution (you can read more about it here: runem/ts-simple-type#32). We need to be able to construct custom types because the the JSDoc parser has to return an Event lib type from parseJsDocTagString as you also mention here: runem/web-component-analyzer#165. However, adding full support for lib-types is a huge task in itself and needs some thinking.

web-component-analyzer

A mentioned, we need to extend parseJsDocTagString with support for event types. If we want to make a solution without extending support for lib-types in ts-simple-type we could try to explore the following suggestion for a solution. We could return a named interface type like: { kind: "INTERFACE", name: "MouseEvent"} or something like this for the CustomEvent type: { kind: "GENERIC_ARGUMENTS", typeArguments: [{ kind: "STRING" }], target: { kind: "INTERFACE", name: "CustomEvent" } } these types must be custom type-checked in lit-analyzer. We also need to do the same in the custom element WCA flavor.

lit-analyzer

When type checking events, we could extend the type checking using the isAssignable config. This will allow us to hook into the functionality of ts-simple-type just like we already do here for attribute bindings:

lit-analyzer/packages/lit-analyzer/src/analyze/util/type/is-assignable-in-attribute-binding.ts

Line 50 in b7fe1b4

if (!isAssignableToType({ typeA, typeB }, request, { isAssignable: isAssignableToTypeWithStringCoercion })) {

Inside the type checking we could have some custom logic that checks for named interfaces like what we returned in web-component-analyzer. I'm now exactly sure about how the logic could look, but it would be interesting to explore. The weakness of this solution is that the type checking wouldn't be fully structural type checking, but instead take into account the names of the event-types in use. However, the strength would be that it's a relatively easy solution to built because ts-simple-type doesn't need to be extended with full lib-type support.

[runem]

I just got a simple (and perhaps better) idea we could explore that's even easier to implement. We can implement a function in ts-simple-type or web-component-analyzer with the signature getLibType(name: string, program: Program): Type. Calling this type with eg. "MouseEvent" would search through lib.dom.d.ts (and other lib files) from program.getSourceFile(...) and find a type with the name "MouseEvent". Finally it would return the type of the AST node. We could also maintain a cache between name => Type for this function, so we avoid traversing lib files for every call to this function, - this can't be a WeakMap, but it might not matter for memory because lib types never mutate during the program. parseJsDocTagString would then use getLibType (and some custom logic for syntax like CustomEvent<string>) in order to return the correct type.

A nice side-effect of this solution is that all lib-types would all be support in JSDoc at the same time.

[43081j]

You could probably make use of program.isSourceFileFromExternalLibrary rather than guessing whats part of the lib and what isn't, too. Although a list of known TS files could work for now too.

That does sound like a possible solution, though. Implementing full knowledge of the built-in types would be a huge job so one way or another i think a workaround/shortcut has to exist for now.

[runem]

I just published v1.1.1 of web-component-analyzer which includes support for event lib types as mentioned above. I went with the solution where event types are generated directly from lib declaration files so we don't have to extend the type checking function for events. I maintain a list of lib-filenames of interest for performance reasons, and right now it only includes "lib.dom.d.ts".

The type of the event is now Event, some subtype of Event or any. Therefore the event type can be simply be used as the parameter here: https://github.com/runem/lit-analyzer/pull/101/files#diff-0d8141ea67c4f3ae55cd558ea531f650R15-R22. I already updated the WCA dependency on branch 1.2.0 so you might want to rebase on that branch :-)

[runem]

I just merged the 1.2.0 branch into master, so you can safely rebase this branch on master and choose that branch as the target of this PR.

[runem]

@43081j Is it okay if I add some commits to this PR? I would love to get this awesome feature into a release :-)

[43081j]

I'm just about to rebase, but once i do that, go for it

[43081j]

I rebased.

You seem to have it in your head already what we need to do here so feel free to have a go at it. Especially since it crosses over with other changes you've been making recently

[runem]

Thanks! I'll take a stab at it now then 👍

[runem]

I've now improved type checking for events (also when we don't know the event type for a given event). In addition, lib.dom.d.ts is now scanned for global events and event types, so we for example know the "click" event comes with the MouseEvent type. I also added support for type checking handleEvent (html<button @click="${{handleEvent: console.log}}"`). All in order, it's looking pretty well!

Two considerations I had:

1. If we don't know the exact type of event that a given event handler needs (typeA = ANY), I check for assignability to (evt: Event) => void instead of (evt: any) => void, because it's better than opting out. However, I built it so we always type check the event bivariantly when this happens. In this case we only care if the parameter in typeB is an Event, and this is achieved by forcing "strictFunctionTypes" to false in the type checking (to have bivariant type checking for parameters).

Examples on invalid error message we get if "strictFunctionTypes" is true and we check against an event with unknown type:
Type '(event: MouseEvent) => void' is not assignable to '(event: Event) => void'
Type '(event: CustomEvent) => void' is not assignable to '(event: Event) => void'

2. I'm in doubt if the "no-noncallable-event-binding" rule is useful anymore. We can simply build into "no-incompatible-type-binding" that it starts by checking if typeB is callable, and if not, give an error message similar to the existing "no-noncallable-event-binding" rule, - just to give a more specific error message like You are setting up an event listener with a non-callable type ... which I think is more informative than .. is not assignable to .... What do you think?

[runem]

Here's another point that we need to take into account.

When collecting events, I merge events with the same name because events bubble (see picture). Therefore the type of "change" event ends up being a union of all merged events.

However, when type checking a parameter with type union, the type checking will fail because parameters are checked contravariant. Here's an example:

@customElement("my-element")
export class MyElement extends LitElement {
  didUploadFile(file: File) {
    this.dispatchEvent(new CustomEvent("change", { detail: file }));
  }

  render() {
    return html`<div @change="${(evt: CustomEvent<File>) => console.log(evt.detail.name)}"></div>`;
  }
}

The type of the event change will become CustomEvent<File> | Event (Event because we already have an event called change that can come from for example an 'input' element). When type checking this, it will result in the following error:

Type '(evt: CustomEvent<File>) => void' is not assignable to '(event: CustomEvent<File> | Event) => void'

To fix this, I introduced a check for "typeA" being equals to UNION. If so, I will check each type in the union against "typeB" resulting in the following type checks:

Is (evt: CustomEvent<File>) => void assignable to (event: Event) => void? false
Is (evt: CustomEvent<File>) => void assignable to (event: CustomEvent<File>) => void? true

One of the types was assignable, so the rule wouldn't report a diagnostic for this type.

What do you think about this approach? The nature of event bubbling unfortunately forces the type checking to be a bit more loose, so I don't think that there is a way around this :-)

[43081j]

awesome ill have a read through the commits.

strictFunctionTypes

This stuff is an interesting point. I suppose it is actually right, though... you should strongly type your handlers to the exact subtype of event needed.

But i can imagine most people (incl. myself) have plenty of handlers typed that way (just Event rather than a specific type).

no-noncallable-event-binding

i noticed this too, it should go away IMO. it is replaced by the fact that these type checks will now cover it. if we can have a custom error message in there for non-callable handlers, though, i agree it would be a nice solution. For sake of having specific error messages.

bubbling events

I think iterating over the possible events in the union and checking each one makes sense.

In most cases, we already make these assumptions when writing code. If i listen on change of a custom input, i assume it is only going to fire its own change event and will stop any inner ones from bubbling up. This can't really exist in the type system without knowing the implementation of the components you're using.

So i think best effort is to do what you say and test against each individual signature.

[43081j]

@justinfagnani @rictic this feels like something we should still get merged in so i have rebased onto master. but its a mixture of rune's rework and my changes, so i cant say i have a full understanding of whats going on here.

from what i remember, we removed the non callable events rule and added support for events to the incompatible types rule (which catches that same thing). but there's also a bunch of stuff rune added for handling unions of events based on bubbling etc.