migrate: Merging event handlers adds an unnecessary parameter

[ottomated]

Describe the bug

When this code is migrated:

<button
  on:click={() => console.log('a')}
  on:click={() => console.log('b')}
></button>

it turns into this

<button

  onclick={(event) => {
    console.log('a')
    console.log('b')
  }}
></button>

which both contains extra whitespace and an unnecessary event argument.

Reproduction

REPL

Logs

No response

System Info

svelte@5.0.0-next.247 in repl

Severity

annoyance

[brunnerh]

A correct migration would look closer to this, though:

<button
  onclick={() => {
    try { console.log('a') } catch (e) { console.error(e) }
    try { console.log('b') } catch (e) { console.error(e) }
  }}>

🤔🤔🤔

(And that is also inaccurate since it doesn't throw, so it probably should be more like increasingly nested try/finally. Or maybe throw from the microtask queue.)

[ottomated]

Or a temporary utility function similar to run

[brunnerh]

Yes, been thinking that as well; since there might be quite a bit of logic, maybe something like this:

<button
  onclick={handlers(
    () => console.log('a'),
    e => console.log(e.type),
  )}>

[paoloricciuti]

So i started working a bit on this and while technically the fix to remove the even listener is easy i found some other issue in the events migration. For example something like this

<button
  on:click={(e) => console.log(e)}
  on:click={(event) => console.log(event)}
></button>

is migrated to this

<button

  onclick={(event) => {
    console.log(e)
    console.log(event)
  }}
></button>

which (if you access some property on e instead of just logging it is actually a runtime error. Also as pointed out by @brunnerh putting every handler in one is not the same as separate listeners...running the listeners in a microTask get us pretty close to browser behaviour but can also very likely interfere with immediate propagation (so we should probably manually reimplement that).

There's also to consider that handlers could have modifiers so we need to send those to the handlers function too (or modify the function but as we've seen that adds a complexity).

Long story short for the moment i started exploring this which looks super ugly

<script>
    function stuff(){

    }
</script>
<button
    on:click={(eve) => console.log(eve)}
    on:click={(eve) => console.log(eve)}
    on:blur
    on:click|preventDefault={stuff()}
    on:click
>a</button>

is converted to

<script>
    /** @type {{onclick?: (event: any) => void, onblur?: (event: any) => void}} */
    let { onclick, onblur } = $props();
    function stuff(){

    }
</script>
<button

    onblur={(event)=>{onblur?.(event);}}

    onclick={handlers(
        [(eve) => console.log(eve), ],
        [(eve) => console.log(eve), ],
        [stuff(), "preventDefault"],
        [(event)=>{onclick?.(event);}, ])}
>a</button>

with a handler function that looks like this (stopImmediatePropagation yet to be developed).

function handlers(...listeners){
    return (e)=>{        
        for(let [listener, ...modifiers] of listeners){
            queueMicrotask(()=>{
                for(let modifier of modifiers){
                    if(e[modifier]){
                        e[modifier]();
                    }
                }
                listener(e);
            });
        }
    }
}

however i'm now wondering. What if instead of a listener we replace everything with an action that actually register multiple listeners (maybe even with the on function itself?

All of this to say that i went straight into the rabbit hole and it's deeper that i would've liked lol

[adiguba]

I don't think that using an action is a good idea, as this wil introduce a new way to use handlers...

I prefer the handlers() function, but without the modifiers and ignoring null/undefined :

function handlers(...listeners){
  return (e)=>{
    for (let listener of listeners) {
      if (listener) {
        queueMicrotask(()=>{ listener(e); } );
      }
    }
  }
}

So

<script>
    function stuff(){

    }
</script>
<button
    on:click={(eve) => console.log(eve)}
    on:click={(eve) => console.log(eve)}
    on:blur
    on:click|preventDefault={stuff()}
    on:click
>a</button>

Could be migrated into :

<script>
    import { handlers } from 'svelte/events';

    let { onclick, onblur } = $props();

    function stuff(){

    }
</script>
<button {onblur} onclick={handlers(
    (eve) => console.log(eve),
    (eve) => console.log(eve),
    (event) => {
    event.preventDefault();
    stuff?.(event);
    },
    onclick
)}>a</button>

[paoloricciuti]

I don't think that using an action is a good idea, as this wil introduce a new way to use handlers...

What do you mean a new way? Action always existed and the handler function is not that different. The difference is that with an action you can literally register multiple listener just like svelte 4 while with the function you basically have to reimplement the functionality and the bugs that this could introduce are very subtle.

[adiguba]

Yes it's already possible, but not really the recommended way for a simple handler. I wouldn't expect a migration tool to generate on:action in order to handle events

So in the previous example the multiple on:click would generate something like this :

<button 
    on:addEventListener={['click', (eve) => console.log(eve)]}
    on:addEventListener={['click', (eve) => console.log(eve)]}
    on:addEventListener={['click', (event) => {
        event.preventDefault();
        stuff.?();
    }]}
    on:addEventListener={['click', click]}
>a</button>

I would much rather have something like this :

<button 
    onclick={handlers(
        (eve) => console.log(eve),
        (eve) => console.log(eve),
        (event) => {
            event.preventDefault();
            stuff.?();
        },
        click
    )}
>a</button>

And the handlers() function will would take care of managing errors and stopImmediatePropagation..

[adiguba]

with the handlers function defined like that :

export function handlers(...listeners) {
  // remove null/undefined
  listeners = listeners.filter( i => i );
  // special case :
  switch (listeners.length) {
    case 0: return null;
    case 1: return listeners[0];
  }
  return (e)=> {
    const originalStopImmediatePropagation = e.stopImmediatePropagation;
    let enabled = true;
    e.stopImmediatePropagation = () => {
      originalStopImmediatePropagation.call(e);
      enabled = false;
    }
    for (let listener of listeners) {
      if (listener) {
        // not sure if queueMicrotask() is really needed
        // a try/catch may be a better solution
        queueMicrotask(()=>{ enabled && listener(e); } );
      }
    }
  }
}

[paoloricciuti]

I was actually thinking of a single action that register multiple but to be fair I actually like the multiple action more 🤣

IMHO the point is that this code shouldn't be the most elegant code ever, is obvious that if you want elegant code you should rewrite it yourself. But it should be the most correct. If I'm migrating my code and it introduce a bug in my code because of some quirk of how browsers implements multiple events that would be much worst than a less beautiful code.

[adiguba]

I think that an action with on() is required for the passive/nonpassive modifiers. But for all others case the new onevent syntax is preferable.

[paoloricciuti]

I think that an action with on() is required for the passive/nonpassive modifiers. But for all others case the new onevent syntax is preferable.

Again, that's the most beautiful output but not necessarily the most correct code.

[paoloricciuti]

Just as an example...i've started trying out your function and i already had to change it like this

export function handlers(...listeners) {
  // remove null/undefined
  listeners = listeners.filter( i => i );
  // special case :
  switch (listeners.length) {
    case 0: return null;
    case 1: return listeners[0];
  }
  return (e)=> {
    const originalStopImmediatePropagation = e.stopImmediatePropagation;
    let enabled = true;
    e.stopImmediatePropagation = () => {
      originalStopImmediatePropagation.call(e);
      enabled = false;
    }
    let errors = [];
    for (let listener of listeners) {
      if (listener) {
        try{
          enabled && listener(e);
        }catch(err){
          errors.push(err);
        }
      }
    }
    for(let err of errors){
      queueMicrotask(()=> {throw err});
    }
  }
}

and this still is not matching browser behavior because the errors are thrown after all the listeners have run. Is this kind of edge cases that would be solved by an action imho.

[adiguba]

For the error I think the best way is to dispatch an ErrorEvent :

    } catch (error) {
        window.dispatchEvent(new ErrorEvent('error', {error}));
    }

I also tried some examples with an action, and the two seem to work :

• REPL using on:click for compararing
• REPL using Svelte 5 events

[paoloricciuti]

For the error I think the best way is to dispatch an ErrorEvent :

  } catch (error) {
      window.dispatchEvent(new ErrorEvent('error', {error}));
  }

I also tried some examples with an action, and the two seem to work :

• REPL using on:click for compararing
• REPL using Svelte 5 events

Yeah but what I mean is that this is a catch to the next edge case. With the action with a much simpler code we can get a guarantee of the old behavior.

A downside of the action is that it makes more difficult to actually remove the code later however. Also I have to check but more likely we can't use the action on svelte:body and svelte:window 🤔

[adiguba]

But the action use:listen can introduce new problem when we use an onclick : REPL

=> use:listen has priority on onclick

[paoloricciuti]

But the action use:listen can introduce new problem when we use an onclick : REPL

=> use:listen has priority on onclick

That's how it should be: in svelte 4 there's no event delegation. If you change your code to add another listener you obviously need to modify the code in some way

[adiguba]

Yes but this is because we introduced a new way to add events, which does not mix well with the onevent So it should be documented and explained, and this can be disturbing. That why I talked about a new way to use handlers...

Also the action cannot work on a component, so we need another solution here.

And it would be a clean solution for Multiple event handlers in the doc, which does not handle errors or stopImmediatePropagation :

<button
+   onclick={handlers(one, two)}
-   onclick={(e) => {
-       one(e);
-       two(e);
-   }}
>
    ...
</button>

<button
    {...props}
+   onclick={handlers(doStuff, props.onclick)}
-   onclick={(e) => {
-       doStuff(e);
-       props.onclick?.(e);
-   }}
>
    ...
</button>

[paoloricciuti]

Also the action cannot work on a component, so we need another solution here.

Mmm this is a much better reason to try the handlers solution.

As per the rest i don't think we should incentive people to write multiple handlers in svelte 5...this is just so that if you already have weird code that uses multiple we don't break your code