`onclick` callback not optimized inside each loop

[hyunbinseo]

Describe the bug

Reference #16414

The 1st onclick callback should be defined once and be reused, but is no longer the case

{#each { length: 2 }, index}
  <button type="button" value={index} onclick={(e) => console.log(e.currentTarget.value)}>
    {index}
  </button>
  <button type="button" onclick={() => console.log(index)}>{index}</button>
{/each}

// was
var on_click = (e) => console.log(e.currentTarget.value);
button.__click = [on_click];

// as-is
$.each(node, 0, () => ({ length: 2 }), $.index, ($$anchor, $$item, index) => {
  var fragment_1 = root_1();
  var button = $.first_child(fragment_1);

  $.set_value(button, index);
  button.__click = (e) => console.log(e.currentTarget.value);

Reproduction

From the issue body

• 5.41.0 - fixed
• 5.42.0 - broken

From #16414

• 5.36.5 - broken
• 5.36.6 - fixed
• 5.42.0 - broken

Severity

blocking an upgrade

[7nik]

#17030 removed hoisting of event listeners as not providing real benefits. Maybe it should be reverted for listeners that don't use variables from parents' scopes.

[hyunbinseo]

Weird how #17030 is not mentioned in the change-log.

I've started writing inline onclick handlers knowing that it will be hoisted. That should change.

If the dev intentionally avoids using non-top-level (e.g. @const, index in each loop) variables, I wish it was applied in the optimized output. Inline handlers are such a charm because you don't have to create functions in the top-level script.

[7nik]

I did a small test and don't see a significant difference in the time initialization between 5.41 and 5.42.

[brunnerh]

I think browser engines do a lot of optimization on their own here.

[dummdidumm]

Yeah we removed this optimization because we found that there's no real benefit to doing it and the maintenance overhead and surface area for bugs outweighs the theoretical (and tiny) performance wins

[hyunbinseo]

I want the callback to stay near the <button>, not inside the <script>.

Is this the best workaround for now? REPL

<script lang="ts">
  import type { HTMLButtonAttributes } from 'svelte/elements';
</script>

<!-- A huge chunk of markup -->

<svelte:boundary>
  {@const onclick: HTMLButtonAttributes['onclick'] = (e) => {
    console.log(e.currentTarget.value)
  }}
  {#each { length: 10_000 }, index}
    <button value={index} {onclick}>Label</button>
  {/each}
</svelte:boundary>

export default function App($$anchor) {
  var fragment = $.comment();
  var node = $.first_child(fragment);

  $.boundary(node, {}, ($$anchor) => {
    const onclick = $.derived_safe_equal(() => (e) => {
      console.log(e.currentTarget.value);
    });

[Conduitry]

I think the best workaround for now is that you just keep using the same inline syntax and not worry about it not getting hoisted. Were you seeing an actual performance difference when upgrading Svelte, or was this a theoretical concern about the compiler output changing?

[hyunbinseo]

I will close the issue because it is an intended change with valid reasons.

Still, why not squeeze performance if I am certain that all I need is the <button>'s value attribute?

• I've always used the <form onsubmit> to create a single callback
• Turns out Svelte optimizes inline onclick so I changed my code style
• Now I learned that it no longer optimizes it. I just want to know if my workaround is valid.

My production code does run on old, low-end devices such as Galaxy Tab A7 Lite.