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: Higher order dynamic event handlers are not reactive #12520

Closed
brunnerh opened this issue Jul 21, 2024 · 11 comments · Fixed by #12549, #12563, #12570 or #12582
Closed

Svelte 5: Higher order dynamic event handlers are not reactive #12520

brunnerh opened this issue Jul 21, 2024 · 11 comments · Fixed by #12549, #12563, #12570 or #12582

Comments

@brunnerh
Copy link
Member

brunnerh commented Jul 21, 2024

Describe the bug

Changing an event handler function does not always work as expected when the handler is provided by a higher order function (in terms of the function returning another function).

Noticed this when looking into an issue around dynamic handlers with Svelte 3/4 that does not happen in Svelte 5.

Reproduction

<script>
	let saySomething = $state(name => () => alert('Hello ' + name));

	function change() {
		saySomething = name => () => alert('Bye ' + name);
	}
</script>

<button onclick={saySomething('Tama')}>Tama</button>
<button onclick={saySomething('Pochi')}>Pochi</button>

<br>
<button onclick={change}>Change Function</button>
{saySomething.toString()}

REPL

Dynamic handlers that are not higher order work.
Wrapping the button makes this work (with spread and without).


Longer reproduction to help guard against reintroducing #12519 - the log statements should only happen on handler change:

<script>
	let saySomething = $state(name => {
		console.log('creating "Hello" handler for ' + name);
		return () => alert('Hello ' + name)
	});

	function change() {
		saySomething = name => {
			console.log('creating "Bye" handler for ' + name);
			return () => alert('Bye ' + name);
		}
	}
</script>

<button onclick={saySomething('Tama')}>Tama</button>
<button onclick={saySomething('Pochi')}>Pochi</button>

<br>
<button onclick={change}>Change Function</button>
<pre>{saySomething.toString()}</pre>

REPL

Logs

No response

System Info

REPL

Severity

annoyance

@brunnerh
Copy link
Member Author

@trueadm Now handlers are created on event execution rather than before, introducing #12519 to Svelte 5.
See the longer reproduction I provided above specifically regarding that.

@trueadm
Copy link
Contributor

trueadm commented Jul 22, 2024

@trueadm Now handlers are created on event execution rather than before, introducing #12519 to Svelte 5. See the longer reproduction I provided above specifically regarding that.

Maybe we need to wrap them in a derived then like we do for others. I can create a follow up

@brunnerh
Copy link
Member Author

Note that Rich is also working on a related PR (#12553).

@brunnerh
Copy link
Member Author

Sorry to be a pain, but this breaks:

<script>
	let saySomething = $state(name => {
		console.log('creating "Hello" handler for ' + name);
		return { handler: () => alert('Hello ' + name) };
	});

	function change() {
		saySomething = name => {
			console.log('creating "Bye" handler for ' + name);
			return { handler: () => alert('Bye ' + name) };
		}
	}
</script> 

<button onclick={saySomething('Tama').handler}>Tama</button>
<button onclick={saySomething('Pochi').handler}>Pochi</button>

<br>
<button onclick={change}>Change Function</button>
<pre>{saySomething.toString()}</pre>

REPL (PR #12563)

I suspect there needs to be a more general expression resolution that can handle any combination of function calls and property accesses.

@trueadm
Copy link
Contributor

trueadm commented Jul 23, 2024

Sorry to be a pain, but this breaks:

<script>
	let saySomething = $state(name => {
		console.log('creating "Hello" handler for ' + name);
		return { handler: () => alert('Hello ' + name) };
	});

	function change() {
		saySomething = name => {
			console.log('creating "Bye" handler for ' + name);
			return { handler: () => alert('Bye ' + name) };
		}
	}
</script> 

<button onclick={saySomething('Tama').handler}>Tama</button>
<button onclick={saySomething('Pochi').handler}>Pochi</button>

<br>
<button onclick={change}>Change Function</button>
<pre>{saySomething.toString()}</pre>

REPL (PR #12563)

I suspect there needs to be a more general expression resolution that can handle any combination of function calls and property accesses.

Was this in relation to my PR or this PR?

@brunnerh
Copy link
Member Author

It apparently has been broken before the last PR, just did not test this particular scenario until the other issue was resolved.

@trueadm
Copy link
Contributor

trueadm commented Jul 23, 2024

It apparently has been broken before the last PR, just did not test this particular scenario until the other issue was resolved.

So this PR broke this use-case?

@brunnerh
Copy link
Member Author

There are multiple PRs associated with this issue (at least #12549 and #12563), the repro that combines call and property appears to have never worked, tried in the REPLs of the PRs and with version 5.0.0-next.192, which should have been the latest at the time of reporting.

@trueadm
Copy link
Contributor

trueadm commented Jul 23, 2024

There are multiple PRs associated with this issue (at least #12549 and #12563), the repro that combines call and property appears to have never worked, tried in the REPLs of the PRs and with version 5.0.0-next.192, which should have been the latest at the time of reporting.

Thanks. That makes sense now.

@trueadm
Copy link
Contributor

trueadm commented Jul 23, 2024

@brunnerh Addressed with #12570. Just needed to move the logic to be sooner.

@brunnerh
Copy link
Member Author

brunnerh commented Jul 24, 2024

@trueadm Just one more thing...
This does not seem to work for the old event handlers (on:click), should that be addressed?

REPL - on:click
REPL - onclick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment