(feat) enable actions to enhance typings on applied element

[dummdidumm]

#1243
#431

This assumes we also enhance the typing of the action type in Svelte core so they match. Assuming we have this:

export interface ActionReturn<Parameter = any, Attributes extends Record<string, any> = Record<never, any>> {
	update?: (parameter: Parameter) => void;
	destroy?: () => void;
	$$attributes?: Attributes;
}

You could define an action like this:

export function action(node: HTMLElement): ActionReturn<never, { foo: boolean; 'on:bar': EventHandler<Event> }> {
   // ..
}

and it would enhance the DOM element with the new required attribute foo and the optional on:bar event.

This requires newer TS types so we can only merge that right before the new transformation (also see #1552)

Question (cc @jasonlyu123 @ignatiusmb @ivanhofer ):
Is the assumed ActionReturn good like this? Should we put both event handler and attributes into one parameter (people would have to type onbar or on:bar then (undecided yet what we do here, see other issues)? If not, are the typings for Events good? Should it be bar: Event instead and we add the EventHandler typing ourselves (but how to get currentTarget of the correct DOM element into that then?)

[ivanhofer]

The idea behind this PR is great!

I didn't dug deep into how the language-tools work so I can't give the best review. I tried to just look at the new additions and the big picture make sense to me.

You mentioned you want to enhance the ActionReturn type from 'svelte/action'. Please also consider enhancing the Action type with the same additions.

Because I don't fully know how the language tools work, I'm wondering why a lot of (or all) types get a __svelte... name. When I set something a wrong param on an element I sometimes get an error inside VS Code like "x is not assignable to __svelte_xyz" where it actually could say "x is not assignable to Xyz". Is the prefix really needed?

[ivanhofer]

not sure if this comment is still valid?

[ivanhofer]

I can only see tests for $$action_0. I think a test with multiple actions applied to a dom element would be good.

What happens if two actions expect the same attribute with a different type?

[dummdidumm]

A test with multiple actions is a good idea, yes. If actions conflict then, well, nothing we can do about that 😄 If they types conflict that likely means these actions aren't compatible at runtime, too.

[dummdidumm]

Thanks for the review! I wasn't expecting a review of the code (although I greatly appreciate it), more of the general idea and thoughts on the questions I mentioned - if you prefer attributes and events to be split or not etc.

About the prefixes: They exist so it's easier to filter them out of completions, and because this minimizes the probability that they clash with other definitions - they are global/ambient so everyone could use the types by accident.

[ivanhofer]

Thanks for the review! I wasn't expecting a review of the code (although I greatly appreciate it), more of the general idea and thoughts on the questions I mentioned - if you prefer attributes and events to be split or not etc.

Sorry, I completely forgot to answer the questions from the description 😅

I prefer to have Attributes and Events split. If not possible/wanted, I would prefer on:event instead of the jsx-like onEvent, since it is 1:1 what you write on the element when listening to the event. And onEvent could also confilct with a prop that is called the same.

The ActionReturn interface should probably restrict Attributes and Events to be Records:

export interface ActionReturn<Parameter = any, Attributes extends Record<string, any> = {}, Events extends Record<string, any> = {}> {
	...
}

Defining Events should be handled the same way like in createEventDispatcher.

About the prefixes: They exist so it's easier to filter them out of completions, and because this minimizes the probability that they clash with other definitions - they are global/ambient so everyone could use the types by accident.

By accident or as a workaround. And the naming can change. I today realized that this code does not work anymore because the types have changed in a recent version. (But I can soon switch to the new types from sveltejs/svelte#6770 to get rid of those types)

[dummdidumm]

By accident or as a workaround. And the naming can change. I today realized that this code does not work anymore because the types have changed in a recent version. (But I can soon switch to the new types from sveltejs/svelte#6770 to get rid of those types)

Yeah that's an unfortunate outcome of these types being global. People come to the idea that they can use that but that wasn't the intention, and then they get broken code after bumps. Maybe as part of the new transformation this all should live in a namespace which we hide from autocompletion.

[ignatiusmb]

Is the assumed ActionReturn good like this? Should we put both event handler and attributes into one parameter (people would have to type onbar or on:bar then (undecided yet what we do here, see other issues)?

Ideally they're specified in one place, I would like to avoid the need to skip a parameter as much as possible. I think we should be able to make it so that properties that starts with on: would be treated as events, otherwise an attribute by default.

are the typings for Events good? Should it be bar: Event instead and we add the EventHandler typing ourselves (but how to get currentTarget of the correct DOM element into that then?)

I'd say it's probably better to be as explicit as possible, especially when working with TS. More so if it makes the implementation on the language-tools side more complicated.

[dummdidumm]

Since the event HTML typings in sveltejs/svelte#7649 are now of the form on:X I agree with @ignatiusmb that we should make this one generic where you type both names and events. Will open a PR over at Svelte core shortly.