sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
76.8k stars 3.98k forks source link

EventHandler type is missing important properties #11436

Closed AleksandrHovhannisyan closed 2 weeks ago

AleksandrHovhannisyan commented 2 weeks ago

Describe the problem

Note: I couldn't find anything in the docs or on this repo about this issue.

EventHandler is typed as follows:

https://github.com/sveltejs/svelte/blob/1f9ad03287a01e46fbbca5423195e1b7ad6bc28c/packages/svelte/elements.d.ts#L42-L44

If I want to handle an input event with type safety, I have two options, neither of which is satisfactory:

  1. Type the handler as EventHandler<InputEvent, HTMLInputElement>:
<script lang="ts">
  const handleInput: EventHandler<InputEvent, HTMLInputElement> = (e) => {}
</script>
<input on:input={handleInput} />
  1. Type the handler as FormEventHandler:

https://github.com/sveltejs/svelte/blob/1f9ad03287a01e46fbbca5423195e1b7ad6bc28c/packages/svelte/elements.d.ts#L50

<script lang="ts">
  const handleInput: FormEventHandler<HTMLInputElement> = (e) => {}
</script>
<input on:input={handleInput} />

Either way, the typing for e.target is incomplete:

Trying to access a property on e.target in an input event handler. Only three properties are accessible: addEventListener, removeEventListener, and dispatchEvent.

For some reason, only those three properties are available on the event target itself. Meaning I have to instead use e.currentTarget. But this means that unless I add the typings myself (which I shouldn't have to do), I can't implement event delegation or take advantage of event bubbling since I would always be reading values/properties off of currentTarget, which may not necessarily be the element that fired the event.

Moreover, there is no relatedTarget available on any of these handlers. But it should be part of the types because it's a web standard.

Describe the proposed solution

Better typing for EventHandler's properties. Most other frameworks like React/Solid/etc. have built-in types for these and allow you to access all relevant event target properties.

Importance

would make my life easier

rChaoz commented 2 weeks ago

I believe this is intended? Event.target is the element that received the event, while .currentTarget is the one you attach the listener to (and you know for sure what type it is - like HTMLElement). For example, if you have a div with a click listener, which contains an SVG, part of which is clicked, it's this part of the SVG which is clicked, and the event bubbles eventually to the grandparent div. As such, event.target will be set to the SVG element that was clicked (not an HTMLElement as you might expect) - basically you can't be sure what type it might have. According to the spec, it is typed as an EventTarget object, only having those 3 methods you observed.

As for the relatedTarget property, I think it only exists for mouse/focus events, although I might be wrong.

TLDR: You most likely need to use currentTarget instead of target.

AleksandrHovhannisyan commented 2 weeks ago

I understand the difference between Event.target and Event.currentTarget, but that's beside the point. It doesn't really matter if the event target and current target are different HTML element types—the event type is the same (input, click, whatever), meaning I should be able to access event-related properties other than just the current target. I should also be able to take advantage of event propagation and delegate events to parents rather than attaching a listener to each individual child; this is a standard practice on the web and many other frameworks support it, with proper type narrowing out of the box. It's not an issue in React, Solid, or other frameworks, so I'm wondering why the typing should be different in Svelte.

brunnerh commented 2 weeks ago

This is how target is typed in the spec.

interface Event {
  constructor(DOMString type, optional EventInit eventInitDict = {});

  readonly attribute DOMString type;
  readonly attribute EventTarget? target;
  ...

And if you define a handler function that is not inline, you will always have to type the arguments anyway.

rChaoz commented 2 weeks ago

I don't quite understand what the issue is, exactly. However:

I should be able to access event-related properties other than just the current target.

I believe you can. When using inline listeners, the types are set automatically. Otherwise, indeed, you need to set the types manually:

// Either like this
const handler: EventHandler<InputEvent, HTMLInputElement> = (e) => { /* ... */ }
// Or like this (which I personally prefer because I like my functions to use the function keyword)
function handler(e: InputEvent /* & { currentTarget: HTMLInputElement } // only if I use currentTarget */) {
    // ....
}

If you do, all the types will be correct. You have access to all required values through e, not just the currentTarget. Just don't use the target property - the only real use might be something like this, to check if a specific child was clicked:

if (e.target instaceof HTMLElement && e.target.dataset.something === "some-value") {
    // ...
}

As for:

I should also be able to take advantage of event propagation and delegate events to parents

I believe you can:

// Component.svelte
<script>
    let {
        myProp = 3,
        ...restProps,
    } = $props()
</script>

<div {...restProps}> ... {myProp} ... </div>

And then:

<Component onclick={listener} />
AleksandrHovhannisyan commented 2 weeks ago

@brunnerh If we're following the spec, then shouldn't event objects also have a relatedTarget? Per the spec linked to:

An event has an associated relatedTarget (a potential event target). Unless stated otherwise it is null.

@rChaoz

As for:

I should also be able to take advantage of event propagation and delegate events to parents

I believe you can:

// Component.svelte
<script>
    let {
        myProp = 3,
        ...restProps,
    } = $props()
</script>

<div {...restProps}> ... {myProp} ... </div>

And then:

<Component onclick={listener} />

Maybe we're misunderstanding each other, but this is not event delegation. What you've demonstrated is passing an event handler as a prop. Event delegation is when you set a handler on a parent element to capture/bubble the event if it originated from a child target. For example, if you're building a theme picker with <input type="radio"> radio buttons or a <select>, you shouldn't have to add an on:input listener to each individual radio <input> or <option>—you can instead set a single listener on the parent element and get the value from Event.target.value.

I suppose in the meantime I could just do instanceof checks on the event target so that TypeScript narrows the type correctly, but I've never had to do this in any other JavaScript framework I've worked with.

rChaoz commented 2 weeks ago

If we're following the spec, then shouldn't event objects also have a relatedTarget?

According to MDN, relatedTarget is only defined for MouseEvents. Additionally, the typings come from TypeScript's DOM type definitions, not from Svelte. Finally, if you scroll down just a bit in the link you shared, you get a list of all properties defined for an Event, and relatedTarget is not there.

I suppose in the meantime I could just do instanceof checks on the event target so that TypeScript narrows the type correctly, but I've never had to do this in any other JavaScript framework I've worked with.

Indeed, in React Event.target is typed by default as Element I believe, but React uses synthetic Event types - AKA it redefines all types instead of using the existing "official" types. Svelte uses DOM Event types, which follow the spec, so Event.target is typed as EventTarget.

I suppose in the meantime I could just do instanceof checks on the event target so that TypeScript narrows the type correctly

Not even, you can use this, shorter variant:

const value = (e.target as HTMLInputElement).value

I also believe this code is more explicit - in TypeScript, as basically says "this code will error out if the target is not an Input element, but I am sure it is" - which is exactly the case.

instanceof is to be used when you are not sure if the child necessarily is an Input element or not.

dummdidumm commented 2 weeks ago

This is by design and a limitation of the type system which can't know what the target really was, and therefore TypeScript plays it safe. Therefore closing. You can read more about it here: https://stackoverflow.com/questions/28900077/why-is-event-target-not-element-in-typescript

AleksandrHovhannisyan commented 2 weeks ago

@rChaoz Oh, right, I don't know why I was trying to access relatedTarget off of an input event handler 🤦

My bad y'all.