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

Change event handler dynamically #3040

Closed
astanet opened this issue Jun 18, 2019 · 14 comments · Fixed by #3836
Closed

Change event handler dynamically #3040

astanet opened this issue Jun 18, 2019 · 14 comments · Fixed by #3836

Comments

@astanet
Copy link

astanet commented Jun 18, 2019

Svelte 3 is really great work.

I want to change event handler dynamically.
So, I assigned anonymous function to a variable and set this variable to event handler.
But it doesn't works as I expect.
Please see this REPL.
https://svelte.dev/repl/e9cc2a6c4ad64cb997bb557e066a7696?version=3.5.1

I found some workaround, but it's annoying a bit.
Is this behavior correct?
Is there something I missed?

with many thanks.

@danburzo
Copy link

Hi everyone,

Just wanted to add my thoughts on why conditional events are useful. I'm playing around with building a Slider component. When you mousedown on an element, I'd like to hook up mousemove / mouseup events on the document.

This was my first (ever) try:

<div class='slider'>
	<div class='slider__handle' on:mousedown={() => interacting = true } />
</div>
<svelte:body on:mouseup={interacting ? () => { interacting = false; } : undefined } />

It would be brilliant if event listeners were reactive, with undefined (or falsy values in general) detaching the listener.

(Full code: https://svelte.dev/repl/f91cb7cc81c44771b3260bde7f12013b?version=3.6.9)

@danburzo
Copy link

danburzo commented Jul 29, 2019

P.S.: As a workaround, I can use:

	import { onDestroy } from 'svelte';

	$: interacting ?
		document.addEventListener('mouseup', end) :
		document.removeEventListener('mouseup', end);

	onDestroy(() => {
		document.removeEventListener('mouseup', mouseup);
	});

@danburzo
Copy link

If I understand correctly, event handlers are currently evaluated once. And if they were to become reactive, does that mean it negates the you can safely use inline handlers in Svelte part?

If that's the case, maybe reactivity should be opt-in, via an event modifier? (e.g. |reactive). Opting in would come with the disclaimer that you need to use named functions for best performance, while existing event bindings would still retain the benefit of zero-cost inline functions.

@Conduitry
Copy link
Member

Comment from @pngwn on Discord:

{#each list as x, i}
  <a on:click={fn(x, i)}> { x } </a>
{/each}

Anyway, I was doing this today and I instinctively keyed the list because I expected the function to be 'bound' to that element. What I'm trying to say is that the way it works now is exactly how I think it should work and it shouldn't be considered a bug and i think 'fixing' this as an enhancement would be confusing.

@danburzo
Copy link

Thanks! It would be interesting to see more of that example, if @pngwn can share, to understand what they mean with the snippet. If fn(x, i) is a partial application of an event handler, would that not bind the function to the initial value of x forever?

@pngwn
Copy link
Member

pngwn commented Aug 28, 2019

Yeah, the handler in question was:

const fn = (x, i) => evt => // whatever

And yes, this binds the handler to x and i permanently, this was expected and what I wanted. If svelte started swapping this around after I expressly told it to permanently bind these values, I think it would be confusing.

@danburzo
Copy link

Thanks for the additional details.

I wrote an example (based on the discussion in #3132) which was, indeed, a bit surprising. I guess I was lacking the correct mental model for how/when event handlers are evaluated in an each loop, but the fact that in this snippet:

<ul>
	{#each fruits as fruit }
		<li on:click={eat(fruit)}>{ fruit }</li>
	{/each }	
</ul>

the event handler got de-synchronized from the item it was initially attached to is unexpected IMHO. (I do understand, however, how keying the each loop causes the event handlers to stay in sync.)

@milkbump
Copy link

On wanting a dynamic handler, I found myself wanting to do this:

<script>
     let number = 1;
     let doer = () => number = 2;
     function doer_change(){
          doer = () => number = 3;
     }
</script>

<p>{number}</p>
<button on:click={doer}>Do</button>
<button on:click={doer_change}>Change do</button>

It seems intuitive that by clicking the Change do button and clicking Do after, number should become 3 and not 2.

@tanhauhau
Copy link
Member

tanhauhau commented Oct 29, 2019

@kwangure i guess a workaround for now would be
https://svelte.dev/repl/4de1ed6d15c44bfaa3a96bec711e8aae?version=3.12.1

<script>
     let number = 1;
     let doer = () => number = 2;
     function doer_change(){
          doer = () => number = 3;
     }
     let onClick = () => doer();
</script>

<p>{number}</p>
<button on:click={onClick}>Do</button>
<button on:click={doer_change}>Change do</button>

@astanet
Copy link
Author

astanet commented Nov 22, 2019

@tanhauhau Thank you for your wonderful work.
The same test code I wrote works as expected in v3.15.0.
https://svelte.dev/repl/e9cc2a6c4ad64cb997bb557e066a7696?version=3.15.0
This improvement increases the clarity of Svelte.

@fregante
Copy link

fregante commented Apr 18, 2020

This is good, but it'd be great to see:

  • actually removing the listener
  • allowing undefined to unset it, rather than requiring the handler to be a function (i.e. @astanet’s example should never error)

@aradalvand

This comment has been minimized.

@milkbump
Copy link

See #3040 (comment) above.

@aradalvand
Copy link

@kwangure Oh okay, sorry.

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

Successfully merging a pull request may close this issue.

8 participants