Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EventListener on a union of Element types should accept a specific event type as parameter #46819

Closed
bennypowers opened this issue Nov 16, 2021 · 5 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@bennypowers
Copy link

bennypowers commented Nov 16, 2021

Bug Report

� Search Terms

HTMLElement, HTMLButtonElement, HTMLAnchorElement, HTMLElementEventNameMap, EventListener, MouseEvent, KeyboardEvent, Event, parameter, EventListenerOrEventListenerObject

� Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about strictFunctionTypes (this affects functions as well as methods)

⏯ Playground Link

Playground link with relevant code

� Code

document.createElement("button").addEventListener('click', onClick);
document.createElement("a").addEventListener('click', onClick);
const clicker = document.querySelector<HTMLAnchorElement|HTMLButtonElement>('#clicker');
// @ts-expect-error: should not error, but gives type error below
clicker?.addEventListener('click', onClick);
function onClick(e: MouseEvent) {}

� Actual behavior

Argument of type '(e: MouseEvent) => void' is not assignable to parameter of type 'EventListenerOrEventListenerObject'.
  Type '(e: MouseEvent) => void' is not assignable to type 'EventListener'.
    Types of parameters 'e' and 'evt' are incompatible.
      Type 'Event' is not assignable to type 'MouseEvent'.

� Expected behavior

No error, the parameter type MouseEvent should be inferred as congruent with the parameter type of EventListener

@RyanCavanaugh
Copy link
Member

Non-DOM version for clarity

type A = {
    listen(callback: (this: A, arg: string) => void): void;
    listen(callback: (arg: unknown) => void): void;
    isA: true;
}
type B = {
    listen(callback: (this: B, arg: string) => void): void;
    listen(callback: (arg: unknown) => void): void;
    isB: true;
}
declare const a: A;
declare const b: B;
declare const ab: A | B;
a.listen(fn);
b.listen(fn);
ab.listen(fn);

function fn(arg: string) {}

This happens because the signature unification step occurs first and collapses to the second overload.

I can't think of a way to handle this in the general case that isn't combinatorially explosive; it's basically the same problem as #7294 in a different form.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Nov 16, 2021
@wbt
Copy link

wbt commented Mar 29, 2022

I also encountered this issue and after spending a nontrivial number of hours in the TypeScript code, found a solution to the immediate problem, as well as a more general case.

From the original code posted above, without the error-burying comment, I tweaked it to put the listener definition first, and add a more explicit null check. The error remains. However, explicitly casting as an HTMLElement solves it.

The event map used to give more specific events is defined here (with the click event listed here). That map is merged with others here and used on this line to add the more specific event listeners. The definitions of HTMLAnchorElement and HTMLButtonElement show that both directly extend HTMLElement, but both also have a definition for addEventListener<K extends keyof HTMLElementEventMap>... where the ‘this’ type is specified more narrowly than in the parent.

This is not an obvious solution because one would expect TypeScript to be able to figure out that clicker is an HTMLAnchorElement|HTMLButtonElement which are both HTMLElements so clicker should be treatable as an HTMLElement without requiring the cast. However, because both subtypes override the parent's addEventListener method, each subtype appears to be treated as if that method were not defined on the parent, and TypeScript won't look at that parent definition when trying to match.

In the much more general case, I think it would be reasonably efficient that if there’s an error encountered in matching a function on a union type, search the nearest common ancestor of the unioned types. Computing the nearest common ancestor type of a union type seems like it could be relatively efficient especially with the relatively short inheritance chains that are common in practice. In this case, it would have been easy to identify HTMLElement as a common ancestor, along with all of its ancestors, and search there for a matching signature. As shown by the success of the cast, this would’ve worked.

In the moderately general case, it seems like it should be possible to address this in the lib.dom.d.ts file that ships with TypeScript, using a generic such as

interface HTMLEventListener<E extends HTMLElement> extends DocumentAndElementEventHandlers, GlobalEventHandlers {
    addEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: E, ev: HTMLElementEventMap[K]) => any, options?: boolean | AddEventListenerOptions): void;
    addEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions): void;
    removeEventListener<K extends keyof HTMLElementEventMap>(type: K, listener: (this: E, ev: HTMLElementEventMap[K]) => any, options?: boolean | EventListenerOptions): void;
    removeEventListener(type: string, listener: EventListenerOrEventListenerObject, options?: boolean | EventListenerOptions): void;
}

and then the individual elements could extend that type, either directly or through another layer with generic, such as

interface HTMLButtonElement extends HTMLElement, HTMLEventListener<HTMLButtonElement> {

or

interface HTMLElementWithListeners<E extends HTMLElement> extends HTMLElement, HTMLEventListener<E> { 
//...
interface HTMLButtonElement extends HTMLElementWithListeners<HTMLButtonElement> {

In such a PR, the two interfaces extended by HTMLEventListener would be removed from HTMLElement. However, it would also appear to be necessary to similarly genericize Element, EventTarget, EventListenerOrEventListenerObject, and EventListener to avoid conflicts, and that sounds like much more major surgery than my time budget allows. There may be a much simpler way to do this with other features of generics that are beyond my current time budget to explore but which may be already known to someone more expert than I. This might not be the only issue fixed by such a use of generics.

Summary: It's definitely possible to overcome this in a very specific case by casting, likely possible to overcome this in a moderately general case by changes to the DOM type library, and maybe possible to overcome this in a much more general (though still not universal) case by examining common ancestors of unioned types for signature matches where the existing searches fail.

@bitbof
Copy link

bitbof commented Jul 15, 2023

In the case of HTMLAnchorElement|HTMLButtonElement you could go with HTMLElement as a workaround.

That would not work for me unfortunately. HTMLElement | SVGElement individually accept PointerEvent for "pointerdown" etc. But when using their shared ancestor Element, neither the type nor listener match.

const listener = (e: PointerEvent) => {
    console.log(e);
};

({} as SVGElement).addEventListener('pointerdown', listener); // ok
({} as HTMLElement).addEventListener('pointerdown', listener); // ok
({} as SVGElement | HTMLElement).addEventListener('pointerdown', listener); // error
({} as Element).addEventListener('pointerdown', listener); // error

@wbt
Copy link

wbt commented Feb 26, 2024

Closed as completed - was this fixed?

@RyanCavanaugh
Copy link
Member

The bulk "Close" action I applied to Design Limitation issues doesn't let you pick what kind of "Close" you're doing, so don't read too much into any issue's particular bit on that. I wish GitHub didn't make this distinction without a way to specify it in many UI actions!

The correct state for Design Limitation issues is closed since we don't have any plausible way of fixing them, and "Open" issues are for representing "work left to be done".

@RyanCavanaugh RyanCavanaugh closed this as not planned Won't fix, can't repro, duplicate, stale Feb 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

4 participants