Skip to content
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

Svelte 5: spreading attributes on a input changes event handlers behaviour #11138

Closed
hawk93 opened this issue Apr 11, 2024 · 3 comments · Fixed by #11230
Closed

Svelte 5: spreading attributes on a input changes event handlers behaviour #11138

hawk93 opened this issue Apr 11, 2024 · 3 comments · Fixed by #11230
Assignees
Milestone

Comments

@hawk93
Copy link

hawk93 commented Apr 11, 2024

Describe the bug

Hi, I made a simple component to wrap a checkbox and his style. I realized that if you use rest props while destructing $props rune, the onchange event is triggered with the wrong value.

When calling onchange checked value is wrong

<script>
	let { checked = $bindable(), children, onchange, ...rest } = $props();
</script>

<p>
	<label><input type="checkbox" {onchange} {...rest} bind:checked /> {@render children()}</label>
</p>

When calling onchange checked value is right

<script>
	let { checked = $bindable(), children, onchange, disabled } = $props();
</script>

<p>
	<label><input type="checkbox" {onchange} {disabled} bind:checked /> {@render children()}</label>
</p>

Reproduction

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAACrWSTUvEMBCG_8oQFraF2t7XNCgeRIRdUMGD9ZA2s7vBNAlJVpTS_y5xu-t-KR70kENmMu888046MpcKPZk8dUTzFsmEXFpLMhLebbz4V1QBSUa8WbkmRqhvnLSBVboKsrXGBbhaYvNSm7c79AHmzrQwzovdYL6WGZ-omZqTVevwbl08CgNwKGHkAw-YzLnymJ5_5erjnKbFF7Cmll2Z1hqNOkBY8gArjx7yPHeRYmSdsT5J4Qymswd4nN3d3kyvaWFZpenelEY3S64XWHZJCiWDxmhvFObKLBKe9lBLLSZNrEBRdrxnnO458g2NMOj1-JPqFFQEuj_AGQz8Cag-Aqp7VtMDs1mlSUZaI-RcoiCT4FbYZ9tvsUv_2_8Rd9LB0DXuJlLwWmGSZtAspRIOdbZlz7Yz9_HxMPeJHUZtqniNilGp7SpAxCkr0gyQFYFuo9pDN8juuwAFg-7CoRbotjBJ2tNirRzb2l-asnbw32wR0se0-GtfNrp_Zcxz_wEjEgCuUAQAAA==

Logs

No response

System Info

Repl

Severity

blocking all usage of svelte

@hawk93
Copy link
Author

hawk93 commented Apr 15, 2024

I made a slightly different reproduction of the problem repl because I found that the problem is not about destructuring $props with a rest variable, but spreading attributes on the input.
Whenever you do it the event handlers attached to it are fired in a different moment, giving different results

@hawk93 hawk93 changed the title Svelte 5: using rest props in a component triggers handler in the wrong order Svelte 5: spreading attributes on a input changes event handlers behaviour Apr 15, 2024
@trueadm trueadm added this to the 5.0 milestone Apr 16, 2024
@dummdidumm dummdidumm self-assigned this Apr 17, 2024
@dummdidumm
Copy link
Member

The behavior for this is non-deterministic, because it depends on which events fire in what order. Generally bindings need to happen after attributes have updated, and the spread puts the change event into the same bucket as the spread into one render effect.
I'm tempted to mark this as "won't fix" and instead add a note in the documentation about this.

@hawk93
Copy link
Author

hawk93 commented Apr 18, 2024

If you just add it to the documentation a simple input wrapper like this becomes a lot more verbose

dummdidumm added a commit that referenced this issue Apr 18, 2024
This is attempt 1 to fix #11138. The idea is that actions, bindings and event directives are added to the "spread attributes" function. That function then first sets all attributes, collects event attributes, then runs actions/bindings/event handlers, and then runs event attributes, to guarantee that event attributes run last.

The sad thing is that it doesn't work. The problem is that bindings or actions could have effects inside them, and those effects would then be tied to the render effect surrounding the "spread attributes" function, and not to the block effect above it, leading to wrong rerun/destroy timings.
dummdidumm added a commit that referenced this issue Apr 18, 2024
By running the event listener logic inside an effect on the first run we guarantee that they're attached after binding listeners. Fixes #11138.
dummdidumm added a commit that referenced this issue Apr 18, 2024
By running the event listener logic inside an effect on the first run we guarantee that they're attached after binding listeners. Fixes #11138.
Rich-Harris added a commit that referenced this issue Apr 19, 2024
* fix: run event attributes after binding event listeners

By running the event listener logic inside an effect on the first run we guarantee that they're attached after binding listeners. Fixes #11138.

* give arrow functions stable id, better code gen

* optimise normal function expressions too (rare but valid!)

---------

Co-authored-by: Rich Harris <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants