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

typings: oncancel? should be EventHandler<Event,T> instead of EventHandler<T> #672

Closed
pschyska opened this issue Nov 13, 2020 · 4 comments
Closed
Labels
bug Something isn't working Fixed Fixed in master branch. Pending production release.

Comments

@pschyska
Copy link

Describe the bug
I think the type of oncancel is incorrect. For example on a HTMLDialogElement this resolves to EventHandler<HTMLElement,HTMLElement>. I think oncancel (and probably every other handler referencing EventHandler<T>) should be typed:
oncancel?: EventHandler<Event,T>;

To Reproduce

<script lang="ts">
function cancel(ev: Event)  {
    ev.stopImmediatePropagation();
    ev.preventDefault()
}
</script>

<main>
    <dialog on:cancel="{cancel}">
    </dialog>
</main>

This type errors like this:

Type '(ev: Event) => void' is not assignable to type 'EventHandler<HTMLElement, HTMLElement>'.
  Types of parameters 'ev' and 'event' are incompatible.
    Type 'HTMLElement & { currentTarget: EventTarget & HTMLElement; }' is missing the following properties from type 'Event': bubbles, cancelBubble, cancelable, composed, and 17 more.ts(2322)

According to MDN, this should be a legal handler.

Expected behavior
In this concrete example, I think the type should be EventHandler<Event, HTMLDialogElement>. Or EventHandler<Event, HTMLElement> if there is a reason to not type it as HTMLDialogElement that escapes me right now. (iow: why is dialog not a HTMLProps<HTMLDialogElement>)

@pschyska pschyska added the bug Something isn't working label Nov 13, 2020
@pschyska
Copy link
Author

@jasonlyu123 What do you think about dialog being typed as HTMLProps<HTMLDialogElement>, while we are at it :-)

dummdidumm pushed a commit that referenced this issue Nov 17, 2020
@dummdidumm dummdidumm added the Fixed Fixed in master branch. Pending production release. label Nov 17, 2020
@jasonlyu123
Copy link
Member

It looks like there's plenty of them is typed as HTMLProps<HTMLElement>. I think we can change it to

type HTMLElementPropMap = {
    [k in keyof HTMLElementTagNameMap]: HTMLProps<HTMLElementTagNameMap[k]>
}
interface IntrinsicElements extends HTMLElementPropMap { }

and remove the overlaps.

@dummdidumm
Copy link
Member

Sounds good, we just need to look very closely to not throw out tags that are present now by accident.

@dummdidumm
Copy link
Member

Closing as the original issue has been resolved. For the other things, feel free to open a separate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Fixed Fixed in master branch. Pending production release.
Projects
None yet
Development

No branches or pull requests

3 participants