[feat] action types

[ignatiusmb]

This introduces a new svelte/action module, you can only import types for now

import type { Action } from 'svelte/action';

// example usage
export const fooBar: Action<HTMLAnchorElement, { name: string }> = (node, param) => {
  //                        |                                       |     \_ is type `undefined | { name: string }`
  //                        |                                       \_ is type `HTMLAnchorElement`
  //                        \_ defaults to `HTMLElement` if not specified
}

Resolves #6538

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with [feat], [fix], [chore], or [docs].
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with npm test and lint the project with npm run lint

[dummdidumm]

Oh that's great! I think we need to enhance the exports of the package.json, too, although only with a "types" section.

[ignatiusmb]

I knew I felt like something was missing as soon as I pressed the button. Docs seems to be in an odd state as well with no examples for TS usage.

[ignatiusmb]

I've played around and tried to create several actions, in action, and found that we need to have the optional ? mark in Action (but not in ActionReturn) because that parameters is the only side that is exposed to the final user/dev that would use them.

That means, (1) authors that have their action params optionally receive any use:action={{ x: 'y' }} or none at all use:action can't have their users use the latter since TS will complain that it Expected 2 arguments, but got 1., even though it's totally valid because you need to either have it typed { ... } or none at all void, and (2) gives a false sense of security when writing their code since not having ? essentially means there will always be an argument passed, but this is the domain where authors aren't in control, and this can make them careless and not handle situation where a JS-only user uses their action, leaving them with an Uncaught TypeError because the author forgot to handle when nothing is passed, and immediately destructures the parameters.

So, I believe optionally marking the parameters is the way, and this should be good to go, unless there's any other suggestions for the names.

[bluwy]

Looks like you're right. I did some quick tests locally, and ? can't be removed without causing an error somewhere else. Seems like the way around this is with a ternary, but I don't think that's a great idea. This looks good to me then!

[benbender]

Don't want to be pushy, but would love to see this land as it would make my live easier :) Any blockers outstanding or just no time yet?

PS: Thanks for your hard work!

[seanlail]

I actually needed this today and came across a solution for the optional.
Apparently setting it to default to void is how you make the generic optional.

I ended up with this:

export interface ActionReturn<Parameters = void> {
  update?: (parameters?: Parameters) => void;
  destroy?: () => void;
}

export interface Action<Element extends HTMLElement, Parameters = void> {
  (node: Element, parameters?: Parameters): ActionReturn<Parameters> | void;
}

[dummdidumm]

After going back and forth on this, I think it's better to have Element as the first generic, to have the same order as the parameters. It just feels very unintuitive in my head the other way around.

[benbender]

Tbh, I like the way it is with the option to narrow down the element as an optional. Imho this reflects the common usage-scenario of actions quite well. But it is a matter of taste and explicit vs implicit logic.

[ignatiusmb]

Updated so the generic parameters matches the function parameters, downside would be that authors would need to explicitly pass in the base element every time they wanted to type Parameters, but it would be more predictable this way.

On an unrelated note, would this be better if placed under svelte instead of svelte/action? or perhaps if we were to add more of these utility types, maybe rework and repurpose svelte/types for future enhancements? or maybe this would be a question for Svelte 4 👀

[dummdidumm]

Thought I'll try this out myself once more before merging and ran into the same "what to do with parameters" situation. As @seanlail mentions, it seems possible to mark the parameter as optional by making it void by default. So the typings would look like this:

export interface Action<Element = HTMLElement, Parameter = void> {
    <Node extends Element>(node: Node, parameter: Parameter): void | ActionReturn<Parameter>;
}
// ActionReturn stays untouched/unchanged

This allows people to write actions without a parameter simply by not defining it

const actionWithoutParams: Action<HTMLElement> = (node, param) => {
   // using the param in the function body will error
   param.asd; // error: Property 'asd' does not exist on type 'void'
};

If people want a parameter, they are required to think about what the parameter looks like, and users have to use them

const action: Action<HTMLElement, { a: true }> = (node, param) => {
   param.a; // works
};

Buuuut, if people want optional params, they have to use void | TheirType, which almost works: using a default param doesn't make TS know that it's always defined

const actionWithOptionalParam: Action<HTMLElement, void | { a: true }> = (node, param = {a: true}) => {
   param.a; // error: Property 'a' does not exist on type 'void | { a: true; }'
};

This isn't good, and I agree with @ignatiusmb that authors should be nugded to use default/fallback params, so ultimatly I conclude that the action types are good as-is.

[mrkishi]

By marking it ?, doesn't this make it impossible to have a required param type-check?

I thought undefined would work well instead of void, see playground:

export interface ActionReturn<Parameters = void> {
	update?: (parameters: Parameters) => void;
	destroy?: () => void;
}

export interface Action<Parameters = any, Element extends HTMLElement = HTMLElement> {
	(node: Element, parameters: Parameters): ActionReturn<Parameters> | void;
}

const untyped: Action = (node, param) => {
	const a = param.anything;
	const b: typeof param = { some: 'param' };
};

const param_required: Action<string> =  (node, param) => {
	const a = param.length;
	const b: typeof param = 'string';
};

const param_optional_missing: Action<string | undefined> = (node, param) => {
	const a = param.length;
	const b: typeof param = 'string';
};

const param_optional_default: Action<string | undefined> = (node, param = 'abc') => {
	const a = param.length;
	const b: typeof param = 'string';
};

const param_rejected: Action<void> = function (node, param) {
	const a = param.length;
	const b: typeof param = 'string';
};

[ignatiusmb]

Hey @benbender, thank you for your review and input here, we've discussed this in the latest meeting and decided it would be more consistent to keep the order of generics the same with the order of function parameters.

There's also some supporting arguments that actions are rarely made to generically work with all HTML elements, it's usually a specific action made for a/some specific elements, most also don't receive a parameter, and that would make typing the element harder if we were to put the generic order in reverse.

When it is a generic purpose action, needing to (re)write the <HTMLElement, ... wouldn't be the end of the world, and decided that it's a sacrifice we're willing to make. I also think it's always better to be explicit about these kinds of stuff as it minimizes the chances of error and improves maintainability, I mean we're already being verbose by using TS so what's the harm in typing a couple more characters 😉

[benbender]

@ignatiusmb thanks for the feedback and totally fine to me. Glad this pr landed. Thanks to all of you for your work! Very much appreciated.

[ignatiusmb]

Awesome! Leaving this finding from our meeting as well in case someone still wanted to only type the parameter when defining their action. Users can always make their own interface to avoid needing to type in the Element generic over and over again, here's an example for an action interface that takes in any HTML element

export interface HTMLAction<T> extends Action<HTMLElement, T> {}

[Antonio-Bennett]

Hey I saw this has just merged. Would be able to now import these types for use? If so how do we do so?

[dummdidumm]

import type { Action } from "svelte/action";

Usage example: see docs of the interface. It's not released yet so you can't use it right now.

[dummdidumm]

@Antonio-Bennett this is unrelated, this is a type error, TypeScript doesn't know this event exist, see here for how to fix it: https://github.com/sveltejs/language-tools/blob/master/docs/preprocessors/typescript.md#im-using-an-attributeevent-on-a-dom-element-and-it-throws-a-type-error

[Antonio-Bennett]

@dummdidumm Thanks a lot worked like a charm!

[ak5]

Maybe I am overlooking something but I want a second parameter to be a () => void

export const clickOutside: Action<HTMLElement, () => void> = (node, callback) => {
	function onClick(event: MouseEvent) {
		if (!node.contains(event.target as Node)) {
			// @ts-expect-error can't seem to type this
			callback();
		}
	}
	tick().then(() => document.addEventListener('click', onClick));

	return {
		destroy() {
			document.removeEventListener('click', onClick);
		}
	};
};

but I need the //@ts-expect-error for some reason?