fix: migrate events to be more inline with svelte 4 #13362

paoloricciuti · 2024-09-21T17:15:10Z

Svelte 5 rewrite

This is super draft and it's just to play around with the output and discuss.

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

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 and linting

Run the tests with pnpm test and lint the project with pnpm lint

changeset-bot · 2024-09-21T17:15:14Z

⚠️ No Changeset found

Latest commit: a5184e1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

paoloricciuti · 2024-09-21T17:25:07Z

As @adiguba pointed out here #13273 (comment) we can't use an action on components so using a function is probably necessary

paoloricciuti · 2024-09-22T17:03:14Z

Ok this is almost ready...a few things missing but I would love some feedback on how to solve those.

passive and nonpassive modifiers are not really migratable...we could use actions but this means that if we have even a single passive or nonpassive in the list of modifiers of a series of the same event the whole thing should move to an action and we already determined that that's a nono. I guess we can just avoid migrating them but then the question become how do we signal to the user that this was not migratable? @dummdidumm surrounded the function with an arrow function and added a comment like // @migration-step but creating an arrow for the sole purpose of adding a comment seems a bit bad but it could work. We could also import a function like migrationNeeded() and use that to wrap. This will both throw a warn in dev and provide a useful searchpoint to actually fix things....also now that i think about it there's no way to create a passive event in svelte 5 right?
The typescript story is not the most beutiful since it's an EventListener...before it was able to infer that it was an MouseEvent inside an on:click so if the user is using something like event.key it will throw a TS error (we might not need to worry about this because it should be easily fixable and it's not a runtime error).
Would love to have an insight on the exported HOF...especially once is miimic the browser behavior but it's not really removing the listener. Is that ok?

I would mark this as ready to review and add this stuff as a fix later.

adiguba · 2024-09-22T18:42:24Z

Some remarks :

Isn't it better to use window.dispatchEvent() to dispatch event instead of queueMicrotask() ?
I think handlers() should handle null/undefined...
trusted(), self(), stopPropagation(), once(), stopImmediatePropagation() and preventDefault() are already present in client/dom/legacy/event-modifiers.js
Additionally these functions should also handle null/undefined, or the generated code should be modified.
Currently on:click|preventDefault={fn} is migrated into onclick={preventDefault(fn)}, but the latter throw an error when fn is null/undefined
On the opposite, on:click is now migrated into onclick={(event)=>{onclick?.(event);}}, instead of {onclick} previously.
And I just see a problematic use-case with this code :

<script>
	export let onclick;
</script>
<button on:click={onclick} on:click>
	click
</button>

=> How to migrate this ?

adiguba · 2024-09-22T19:01:36Z

For the (2), I think this statement should fix the type issues :

/** @import { EventHandler } from "svelte/elements"; */

/**
 * Function to mimic the multiple listeners available in svelte 4
 * @template {Event} [E=Event]
 * @template {EventTarget} [T=Element]
 * @param {...(EventHandler<E, T> | null | undefined)} handlers
 * @returns {EventHandler<E, T> | null | undefined}
 */
export function handlers(...handlers) {

paoloricciuti · 2024-09-22T19:10:48Z

Some remarks :

Isn't it better to use window.dispatchEvent() to dispatch event instead of queueMicrotask() ?

I don't know if it makes a difference but I think it does in terms of stacktrace.

I think handlers() should handle null/undefined...

It definitely should, will fix

trusted(), self(), stopPropagation(), once(), stopImmediatePropagation() and preventDefault() are already present in client/dom/legacy/event-modifiers.js

Wtf how did I miss this, maybe I will re-export them from from /legacy tho to not complicate the import logic too much.

Additionally these functions should also handle null/undefined, or the generated code should be modified.

Yeah I agree

On the opposite, on:click is now migrated into onclick={(event)=>{onclick?.(event);}}, instead of {onclick} previously.

Uh thanks, will fix this.

And I just see a problematic use-case with this code :
<script>
	export let onclick;
</script>
<button on:click={onclick} on:click>
	click
</button>
=> How to migrate this ?

Yeah I mean we can only go far enough with static analysis. Currently we are not migrating components either because those might be outside of your control so the migration will still kinda break. I don't think we can migrate complex situations like this in any way.

paoloricciuti · 2024-09-22T20:43:02Z

For the (2), I think this statement should fix the type issues :

/** @import { EventHandler } from "svelte/elements"; */

/**
 * Function to mimic the multiple listeners available in svelte 4
 * @template {Event} [E=Event]
 * @template {EventTarget} [T=Element]
 * @param {...(EventHandler<E, T> | null | undefined)} handlers
 * @returns {EventHandler<E, T> | null | undefined}
 */
export function handlers(...handlers) {

Uh EventHandler from svelte could be the answer...I think this might even be helpful to fix the current typing problem that affect on

Rich-Harris · 2024-09-22T23:38:41Z

Currently we are not migrating components either because those might be outside of your control so the migration will still kinda break

Indeed, this is an argument for preserving the current semantics of on:click without an argument:

<script>
	export let onclick;
</script>
<button on:click={onclick} on:click>
	click
</button>

<script>
	import { createBubbler } from 'svelte/legacy';

	const bubble = createBubbler();
	let { onclick } = $props();
</script>
<button onclick={handler(onclick, bubble('click'))}>
	click
</button>

packages/svelte/src/compiler/migrate/index.js

Rich-Harris · 2024-09-22T23:57:53Z

maybe I will re-export them from from /legacy tho to not complicate the import logic too much

I took the liberty of doing this because I also noticed that we need to mark them as @deprecated

adiguba · 2024-09-23T05:10:47Z

Currently we are not migrating components either because those might be outside of your control so the migration will still kinda break

Indeed, this is an argument for preserving the current semantics of on:click without an argument:

Compatibility with on:event directives is necessary to facilitate code migration, but creating a bubble for each on:directive could be very verbose and error prone.

Currently a code like that :

<button on:click on:focus on:blur on:mouseenter on:mouseleave on:mousedown on:mouseup {...$$restProps}>
	<slot/>
</button>

can be greatly simplified (and improved) with Svelte 5

<script>
	/** @type {HTMLButtonAttributes} */
	let {
		children,
		...rest
	} = $props();
</script>

<button{...rest}>
	{@render children?.()}
</button>

But if we need to create bubble for compatibility, we will get a code that is even more difficult to read than ths Svelte 4 version :

<script>
	import { createBubbler, handlers } from 'svelte/legacy';

	const bubble = createBubbler();
	
	/** @type {{Record<string, any>}} */
	let {
		children,
		onclick,
		onfocus,
		onblur,
		onmouseenter,
		onmouseleave,
		onmousedown,
		onmouseup,
		children,
		...rest
	} = $props();

</script>

<button onclick={onclick}
	onfocus={handlers(onfocus, 'click')}
	onblur={handlers(onblur, 'blur')}
	onmouseenter={handlers(onmouseenter, 'mouseenter')}
	onmouseleave={handlers(onmouseleave, 'mouseleave')}
	onmousedown={handlers(onmousedown, 'mousedown')}
	onmouseup={handlers(onmouseup, 'mouseup')}
	{...rest}>
	{@render children?.()}
</button>

So instead... why not using a way to do an automatic conversion from on:directive to props ?
For example the compiled code could become:

export default function Button($$anchor, $$props) {
+	$.convert_on_directives($$props);
	$.push($$props, true);
// ...

With convert_on_directives() which would look something like this :

/**
 * Put the old on:event directive into the Svelte 5's props
 * @param {Record<string, any>} props
 */
export function convert_on_directives(props) {
	if (props?.$$events) {
		for (const event_name of Object.getOwnPropertyNames(props.$$events)) {
			/** @type {Function[]} */
			let funcs = [];

			if (event_name in props) {
				funcs.push(props[event_name]);
			}

			/** @type {Function | Array<Function>} */
			let on_directive = props.$$events[event_name];
			if (typeof on_directive === 'function') {
				funcs.push(on_directive);
			} else {
				funcs.push(...on_directive);
			}

			if (funcs.length === 1) {
				props[event_name] = funcs[0];
			} else {
				props[event_name] = handlers(...funcs);
			}
		}
	}
}

It should perhaps be enabled via an option on the component :

<svelte:options convert_on_directives={true} />

The migration code will be simple, and ready for the next step (when on:event will be removed)

adiguba · 2024-09-23T06:02:58Z

Also, should'nt handlers() be in 'svelte/events' instead of 'svelte/legacy' ?

So the doc multiple-event-handlers can be updated with respect for stopImmediatePropagation and error's handling.

paoloricciuti · 2024-09-23T06:07:29Z

Also, should'nt handlers() be in 'svelte/events' instead of 'svelte/legacy' ?

So the doc multiple-event-handlers can be updated with respect for stopImmediatePropagation and error's handling.

Personally I think it doesn't make sense for fresh svelte 5 code to be written in multiple listeners...we need something like handlers because to have the same behavior there's quite a bit of code but if you are writing it from scratch you can simply write a single one.

paoloricciuti · 2024-09-23T06:15:02Z

Currently we are not migrating components either because those might be outside of your control so the migration will still kinda break

Indeed, this is an argument for preserving the current semantics of on:click without an argument:

Compatibility with on:event directives is necessary to facilitate code migration, but creating a bubble for each on:directive could be very verbose and error prone.

Currently a code like that :

<button on:click on:focus on:blur on:mouseenter on:mouseleave on:mousedown on:mouseup {...$$restProps}>
	<slot/>
</button>

can be greatly simplified (and improved) with Svelte 5

<script>
	/** @type {HTMLButtonAttributes} */
	let {
		children,
		...rest
	} = $props();
</script>

<button{...rest}>
	{@render children?.()}
</button>

But if we need to create bubble for compatibility, we will get a code that is even more difficult to read than ths Svelte 4 version :

<script>
	import { createBubbler, handlers } from 'svelte/legacy';

	const bubble = createBubbler();
	
	/** @type {{Record<string, any>}} */
	let {
		children,
		onclick,
		onfocus,
		onblur,
		onmouseenter,
		onmouseleave,
		onmousedown,
		onmouseup,
		children,
		...rest
	} = $props();

</script>

<button onclick={onclick}
	onfocus={handlers(onfocus, 'click')}
	onblur={handlers(onblur, 'blur')}
	onmouseenter={handlers(onmouseenter, 'mouseenter')}
	onmouseleave={handlers(onmouseleave, 'mouseleave')}
	onmousedown={handlers(onmousedown, 'mousedown')}
	onmouseup={handlers(onmouseup, 'mouseup')}
	{...rest}>
	{@render children?.()}
</button>

So instead... why not using a way to do an automatic conversion from on:directive to props ? For example the compiled code could become:

export default function Button($$anchor, $$props) {
+	$.convert_on_directives($$props);
	$.push($$props, true);
// ...

With convert_on_directives() which would look something like this :

/**
 * Put the old on:event directive into the Svelte 5's props
 * @param {Record<string, any>} props
 */
export function convert_on_directives(props) {
	if (props?.$$events) {
		for (const event_name of Object.getOwnPropertyNames(props.$$events)) {
			/** @type {Function[]} */
			let funcs = [];

			if (event_name in props) {
				funcs.push(props[event_name]);
			}

			/** @type {Function | Array<Function>} */
			let on_directive = props.$$events[event_name];
			if (typeof on_directive === 'function') {
				funcs.push(on_directive);
			} else {
				funcs.push(...on_directive);
			}

			if (funcs.length === 1) {
				props[event_name] = funcs[0];
			} else {
				props[event_name] = handlers(...funcs);
			}
		}
	}
}

It should perhaps be enabled via an option on the component :

<svelte:options convert_on_directives={true} />

The migration code will be simple, and ready for the next step (when on:event will be removed)

The point is that we are not migrating components so

<Component on:click />

Will stay like this. So no, you can't migrate your component to spreading everything by props. And if we follow rich suggestion you don't even need to create new props, you just need to create a bubbler and set the handler of onclick to bubble('click').

Is this the more idiomatic svelte 5 code? No. But you can't expect a programmatic migration tool to write perfect idiomatic code.

adiguba · 2024-09-23T07:20:59Z

Personally I think it doesn't make sense for fresh svelte 5 code to be written in multiple listeners...we need something like handlers because to have the same behavior there's quite a bit of code but if you are writing it from scratch you can simply write a single one.

The doc contains an example and an solution for that...
This can be useful when you want to add a handler while spreading it from the caller.

Using handlers() will fix the antipattern (we immediately see that there are several handlers), while managing properly stopImmediatePropagation() and errors...

adiguba · 2024-09-23T07:56:17Z

The point is that we are not migrating components so
<Component on:click />

<Component on:click={fn} /> should not be migrated, but I think that <Component on:click /> can be migrated to :

<script>
	let { onclick } = $props();
</script>
<Component on:click={click} />

adiguba · 2024-09-23T08:12:16Z

Is this the more idiomatic svelte 5 code? No. But you can't expect a programmatic migration tool to write perfect idiomatic code.

It's not only for programmatic migration, but migration in general.

When migrating a Svelte 4 component to Svelte 5, switching to the handler as props will break all existing code that use this component with on:event directive (and this code can be out-of-control).

In 100% Svelte 5 code, <button on:click on:blur on:focus {...$$restProps}> could be simplified as <button {...props}> (and it's even better since all the handlers are spreaded).

But since I need to keep compatibility, I should add bubbles to forward events again in order to support the old on:event syntax from the caller :

<script>
    let { onclick, onblur, onfocus, ...props } = $props();
    
    const bubble = createBubbler();
</script>
<button onclick={handlers(onclick, bubble('click')}
    onblur={handlers(onblur, bubble('blur')}
    onfocus={handlers(onfocus, bubble('focus')}
    {...props}> ... </button>

=> A lot more verbose, even than the Svelte 4 code, and that's just an one-line component !
Additionally, this would need another rewrite in the futur when on:event directive will be removed.

I think that could prevent devs from migrating their codes...

In fact the Svelte 4 code is even simpler, since it can handle both onclick and on:click.
Example : REPL

=> For "pure" Svelte 5 component, I think that on:event should be automatically mapped into onevent.

fix: migrate events to be more inline with svelte 4

528da1c

paoloricciuti added the needs discussion label Sep 21, 2024

vercel bot deployed to Preview – svelte-dev September 21, 2024 17:17 View deployment

vercel bot deployed to Preview – svelte-5-preview September 21, 2024 17:20 View deployment

fix: use function instead of action

2194d49

vercel bot deployed to Preview – svelte-dev September 22, 2024 16:41 View deployment

vercel bot deployed to Preview – svelte-5-preview September 22, 2024 16:43 View deployment

chore: generate types

5c1285e

vercel bot deployed to Preview – svelte-dev September 22, 2024 16:45 View deployment

vercel bot deployed to Preview – svelte-5-preview September 22, 2024 16:48 View deployment

paoloricciuti marked this pull request as ready for review September 22, 2024 17:03

Rich-Harris reviewed Sep 22, 2024

View reviewed changes

packages/svelte/src/compiler/migrate/index.js Outdated Show resolved Hide resolved

re-export event modifier substitutes

06d28f9

vercel bot deployed to Preview – svelte-dev September 22, 2024 23:57 View deployment

vercel bot deployed to Preview – svelte-5-preview September 23, 2024 00:00 View deployment

chore: replace map with object

ce6aedd

vercel bot deployed to Preview – svelte-5-preview September 23, 2024 07:07 View deployment

vercel bot deployed to Preview – svelte-dev September 23, 2024 07:07 View deployment

fix: handle nullish values

a5184e1

vercel bot deployed to Preview – svelte-dev September 23, 2024 07:09 View deployment

vercel bot deployed to Preview – svelte-5-preview September 23, 2024 07:12 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: migrate events to be more inline with svelte 4 #13362

fix: migrate events to be more inline with svelte 4 #13362

paoloricciuti commented Sep 21, 2024

changeset-bot bot commented Sep 21, 2024 •

edited

Loading

paoloricciuti commented Sep 21, 2024

paoloricciuti commented Sep 22, 2024

adiguba commented Sep 22, 2024

adiguba commented Sep 22, 2024

paoloricciuti commented Sep 22, 2024

paoloricciuti commented Sep 22, 2024

Rich-Harris commented Sep 22, 2024

Rich-Harris commented Sep 22, 2024

adiguba commented Sep 23, 2024

adiguba commented Sep 23, 2024

paoloricciuti commented Sep 23, 2024

paoloricciuti commented Sep 23, 2024

adiguba commented Sep 23, 2024

adiguba commented Sep 23, 2024

adiguba commented Sep 23, 2024

fix: migrate events to be more inline with svelte 4 #13362

Are you sure you want to change the base?

fix: migrate events to be more inline with svelte 4 #13362

Conversation

paoloricciuti commented Sep 21, 2024

Svelte 5 rewrite

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

Tests and linting

changeset-bot bot commented Sep 21, 2024 • edited Loading

⚠️ No Changeset found

paoloricciuti commented Sep 21, 2024

paoloricciuti commented Sep 22, 2024

adiguba commented Sep 22, 2024

adiguba commented Sep 22, 2024

paoloricciuti commented Sep 22, 2024

paoloricciuti commented Sep 22, 2024

Rich-Harris commented Sep 22, 2024

Rich-Harris commented Sep 22, 2024

adiguba commented Sep 23, 2024

adiguba commented Sep 23, 2024

paoloricciuti commented Sep 23, 2024

paoloricciuti commented Sep 23, 2024

adiguba commented Sep 23, 2024

adiguba commented Sep 23, 2024

adiguba commented Sep 23, 2024

changeset-bot bot commented Sep 21, 2024 •

edited

Loading