sveltejs / language-tools

The Svelte Language Server, and official extensions which use it
MIT License
1.26k stars 202 forks source link

Support custom events on native DOM elements #431

Open johanbissemattsson opened 4 years ago

johanbissemattsson commented 4 years ago

Is your feature request related to a problem? Please describe. I get the following error when I'm trying to add custom events on native DOM elements:

Type '{ onswipestart: (event: CustomEvent<any>) => void; onswipemove: (event: CustomEvent<any>) => void; onswipeend: (event: CustomEvent<any>) => void; style: string; class: string; }' is not assignable to type 'HTMLProps<HTMLDivElement>'.
  Property 'onswipestart' does not exist on type 'HTMLProps<HTMLDivElement>'.ts(2322)

The custom events are dispatched to a div element using Sveltes actions/use directive https://svelte.dev/examples#actions).

Describe the solution you'd like Be able to type check for custom events dispatched using Svelte actions on native DOM elements.

Describe alternatives you've considered I could try to convert the div to an individual Svelte component but preferably it should work on native DOM elements as well.

Additional context

The error message

This is how I listen to the custom events:

<div
  class="bottomSheet"
  class:draggable
  bind:this={bottomSheet}
  use:swipeable
  on:swipestart={handleSwipeStart}
  on:swipemove={handleSwipeMove}
  on:swipeend={handleSwipeEnd}
  style="height:{height};bottom:{bottom};transform:translateY({$coords.ty}px);"
>

Related Pull Request for custom events on Svelte components: https://github.com/sveltejs/language-tools/pull/303

jasonlyu123 commented 4 years ago

Don't know if there is a way to make it only applies to elements with the action. but you can make it globally available like this.

declare namespace svelte.JSX {
    interface HTMLAttributes<T> {
        onswipemove?: (event: CustomEvent<number> & { target: EventTarget & T }) => any;
    }
}
dummdidumm commented 4 years ago

We could also enhance our typings with "fall back to CustomEvent<any> but I'd rather not do it because others may rely on the typings to throw a "does not exist" error if they mistype an event. I would be okay with something like that if it's only applied to elements with actions, but I'm not sure how we would implement that. To infer the type from the action is an almost impossible task I think. Maybe we can find a way to let the user explicitly type the action and its possible events.

johanbissemattsson commented 4 years ago

Thanks, letting the user explicitly type the action or explicitly choose when to fall back to CustomEvent<any> would probably be the best case scenario but the solution by @jasonlyu123 is good enough for me now. Thanks for the great work!

dummdidumm commented 4 years ago

I had a look at this again and now I'm split on what's the best way forward.

Option 1: Silence error

We could silence the error if it's a on:XX event on a DOM element with a use:YY directive. The drawback is that the event is of type any implicitly. It also may not catch all cases, because Svelte is so dynamic in its nature, someone could do bubbling CustomEvents.

Option 2: Add CustomEvent<any> fallback

We would essentially add & {[key: string]: CustomEvent<any>} to all instrinsic element typings. This means any attribute that is not listed above will fall back to CustomEvent<any>. This would fix the "dynamic nature" problem and the "implicit any" problem of option 1. But it is also wrong in cases someone uses a custom attribute like myownAttribute.

Option 3: Add any fallback

Like option 2, only that we add a & {[key: string]: any} fallback. This means anything that does not fit falls back to any. This fits the dynamic nature of the DOM the best at the cost of not catching typos anymore.

dummdidumm commented 3 years ago

My latest idea for this is to

Edit: I just realized this would break people who enhanced the definition with their own, because we don't know of those and would still transform to the CustomEvent<any> fallback. I don't think this is desireable.

aradalvand commented 3 years ago

Hey there, I was wondering if the solution suggested by @jasonlyu123 is still the only fix for this problem? I, too, have actions that dispatch custom events and I would like to get rid of the type error.

dummdidumm commented 3 years ago

It's still the solution, yes. I would appreciate some feedback on the three options listed in my comment above. I lean more and more towards option 3 since it matches the dynamic nature of Svelte the best, and typos are very rare I guess.

aradalvand commented 3 years ago

@dummdidumm Personally, I'm an everything-should-be-as-type-safe-as-possible kinda guy, so I think rather than falling back to any or even CustomEvent<any>, it'd be nice if we could have a way to explicitly declare what custom events a given action will potentially dispatch, and also whether or not each event bubbles. If an event bubbles, then it should be available on the element itself (the element that has the action applied to it) AND all of its ancestors. Everywhere else the event is used we should have a type error.

Now, I have no idea how difficult or tricky this would be to implement. It might actually be a terrible idea. Let me know what you think.

If this isn't feasible, I agree that the 3rd approach among the ones you proposed in this comment would be the sanest.

dummdidumm commented 3 years ago

Yes what you describe would be ideal, but from a typing perspective I don't see a way to accomplish it, because one can only retrieve the return type of the function. One possibility would be to check if there's a property on the function which is only used for type checking, similar to how Angular does it. But even if we do that, then there's the bubble thing, which is just impossible in my mind to do, because we more or less have to statically analyze everything as a whole, and even that might not work for all cases. So yeah, solution 3 is probably the best option, and people who want strict types for specific events can still type them given the instructions above. And we could look into that property-on-function-thing.

dummdidumm commented 3 years ago

We could use the enhanced type signatures to tell svelte2tsx that every property starting with on and not part of the other defined ones is a Custom Event. Question is if that's also too narrow, since theoretically it could be something else. It also requires users of svelte-check to at least use TS 4.4

TheFanatr commented 3 years ago

what about returning some metadata from the action itself i.e.:

return {
    ... // destroy, update, etc.
    events: {
        allowAll: false,
        definitions: [
            { name: "panstart", maxRate: 3 },
            ...
        ]
    }
}

another option would be to include them as parameters:

type ClickEventDetails = HeldClickedEventDetails & { secondsSinceLast: number }
type ClickHeldEventDetails = { count: number }
type ClickHoldReleasedEventDetails = ClickEventDetails & { totalHoldSeconds: number }

function extraClickEvents(node: Node, countableClick: CustomEvent<ClickEventDetais>, clickHeld: CustomEvent<ClickHeldEventDetails>, clickHoldReleased: CustomEvent<ClickHoldReleasedDetails>) {
    ...
    node.dispatchEvent(nameof(countableClick).toLower(), countableClick.setDetails({ count: ..., secondsSinceLast: ... })) // toLower just represents the concept of lowercasing.
    ...
}

then:

<div use:extraclickevents on:countableclick={...} ...>
dummdidumm commented 3 years ago

I like the "use return type to hint at this"-idea. How this could be implemented:

  1. assemble list of all known events inside the svelte-jsx-typings and make them available at runtime
  2. when transforming an event on a dom element, check the list of known events. If the event is not part of it and there's an action on the dom node, do a different transformation which also makes the extra events available

The limitation of this is that you can create actions which coordinate across multiple/parent nodes and therefore only works if the event is listened to on the same node the action was applied to.

taylorhadden commented 2 years ago

We could use the enhanced type signatures to tell svelte2tsx that every property starting with on and not part of the other defined ones is a Custom Event. Question is if that's also too narrow, since theoretically it could be something else. It also requires users of svelte-check to at least use TS 4.4

I do think it would be too narrow. What if I'm invoking some other event through the normal dom methods? What if the event originates from a child element? To handle this, I've resorted to manually adding & removing event listeners to element references. Not ideal.

dummdidumm commented 2 years ago

Dumping this here for myself for later on: A way when using the new transformation on how to get the types of an action onto the element where the action is applied:

type SvelteAction<U extends any, El extends any> = (node: El, args:U) => {
    update?: (args:U) => void,
    destroy?: () => void,
    attributes?: Record<string, any>,
    events?: Record<string, any>
} | void

export type ValuesOf<T> = T[keyof T];
export type ObjectValuesOf<T extends Object> = Exclude<
  Exclude<Extract<ValuesOf<T>, object>, never>,
  Array<any>
>;
declare function createElement<Elements extends IntrinsicElements, Key extends keyof Elements, Actions extends Record<string, SvelteActionReturn>>(
    element: Key,
    actions: Actions,
    attrs: Elements[Key] & ObjectValuesOf<
        {[ActionKey in keyof Actions]:
            {[AttrKey in keyof Actions[ActionKey]['attributes']]: Actions[ActionKey]['attributes'][AttrKey] } &
            {[EvtKey in keyof Actions[ActionKey]['events'] as `on:${EvtKey & string}`]: Actions[ActionKey]['events'][EvtKey]
        }
    }>
): Key extends keyof ElementTagNameMap ? ElementTagNameMap[Key] : Key extends keyof SVGElementTagNameMap ? SVGElementTagNameMap[Key] : HTMLElement;

This monstrosity will allow events and attributes returned by the action type to be added as attributes in addition to the intrinsic attributes.

jacwright commented 2 years ago

Another place where this falls down is bubbled native events. In my app I am listening to the input event being bubbled up from form elements inside a form. Typescript doesn't appreciate <form on:input={...}> though it is perfectly valid. While I appreciate all the benefits of TypeScript, the nature of events and bubbling means that you can't account for every event that may come past an element by only looking at that element's events.

And in this case, the JSX method only works with Event and throws errors with InputEvent.

Screenshot 2022-11-17 at 11 52 34 AM
jasonlyu123 commented 2 years ago

The error is correct. The type of Input event is only InputEvent on some occasions as mentioned in MDN https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/input_event