-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: migrate events to be more inline with svelte 4 #13362
Conversation
🦋 Changeset detectedLatest commit: 57b4ab7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
As @adiguba pointed out here #13273 (comment) we can't use an action on components so using a function is probably necessary |
Ok this is almost ready...a few things missing but I would love some feedback on how to solve those.
I would mark this as ready to review and add this stuff as a fix later. |
Some remarks :
<script>
export let onclick;
</script>
<button on:click={onclick} on:click>
click
</button> => How to migrate this ? |
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) { |
I don't know if it makes a difference but I think it does in terms of stacktrace.
It definitely should, will fix
Wtf how did I miss this, maybe I will re-export them from from /legacy tho to not complicate the import logic too much.
Yeah I agree
Uh thanks, will fix 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. |
Uh EventHandler from svelte could be the answer...I think this might even be helpful to fix the current typing problem that affect |
Indeed, this is an argument for preserving the current semantics of <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> |
I took the liberty of doing this because I also noticed that we need to mark them as |
Compatibility with 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 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) |
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. |
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 Is this the more idiomatic svelte 5 code? No. But you can't expect a programmatic migration tool to write perfect idiomatic code. |
I don't really know why attribute modifiers were ever removed... |
Svelte 5 rewrite
Closes #13273
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 notmain
.Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint