on:click "Cannot read property 'apply' of undefined"

[ghost]

Describe the bug
Click handler throws an exception if handler function is stored as a variable and that variable is set somewhere in the <script> section.

We found this while attempting to fix #4087 via PR #4088.

To Reproduce
See repl:
https://svelte.dev/repl/7433e605325d4b4b87ea5522f5a5ff61?version=3.16.3

The exceptions started occurring from v3.13.0 onward.

Using same repl with older compiler produces expected results (no exceptions):
https://svelte.dev/repl/7433e605325d4b4b87ea5522f5a5ff61?version=3.12.1

Expected behavior
Click event shouldn't do anything (i.e. no exception).

Severity
Low; though it does violate the expectations for events on normal DOM elements.

Additional context

Offending generated code is:

dispose = [
	listen(button0, "click", /*updateHandler1*/ ctx[3]),
	listen(button1, "click", /*updateHandler2*/ ctx[4]),
	listen(button2, "click", function () {
		/*clickHandler*/ ctx[0].apply(this, arguments); // EXCEPTION HERE
	}),
	listen(button3, "click", /*runTest*/ ctx[5])
];

[Conduitry]

Sort of adjacent to this and also to #4153, I was just thinking about the doc change made in #2944. It feels like in theory we should be able to undo that change, now that event handlers are reactive.

Indeed, this works:

<script>
	import ShoppingCart from './ShoppingCart.svelte';
	let cart;
</script>

<ShoppingCart bind:this={cart}/>

<button on:click={cart.empty}>
	Empty shopping cart
</button>

However, this does not:

<script>
	import ShoppingCart from './ShoppingCart.svelte';
	let cart;
</script>

<button on:click={cart && cart.empty}>
	Empty shopping cart
</button>

<ShoppingCart bind:this={cart}/>

(The cart && cart.empty is necessary because cart will still be undefined the first time we evaluate the event handler.) Clicking on the button does not call the cart.empty() method. I'm not sure why.

cc @tanhauhau

[tanhauhau]

hmm.. interesting example, this is a totally different problem..........

bind:this does not mark the cart as reactive:

<ShoppingCart bind:this={cart} />

See repl that works.

Made a PR for the fix #4155

[Conduitry]

What changed between 3.12.1 and 3.13.0 for the code that's generated here is:

dispose = listen(button, "click", ctx.handler);

became

dispose = listen(button, "click", function () {
	ctx.handler.apply(this, arguments);
});

Previously, we were just attaching undefined as an event listener, which the browser happily treated as a no-op when the event happened. Post-3.13, we are always explicitly trying to call it.

I'm not sure whether this change was intended to fix something else, or whether it was just the result of other structured code generation changes. If it were possible to change this back to ctx.handler (or, rather, ctx[4] or whatever) without breaking anything, I think that would be ideal, because it would be less code, and it would go back to letting the browser decide what was a no-op, like was happening in 3.12 and earlier.

[tanhauhau]

we change from ctx.handler to function () { ctx.handler.apply(this, arguments); } only if ctx.handler is dynamic (reassigned/mutated).

why we wrapped it in function() {..} is discussed in #3836 (comment)

in summary is that instead of detach and reattach event listeners whenever ctx.handler is changed, we attach a listener function that will will always redirect to the latest ctx.handler, so we only need to addEventListener once, even if the handler is dynamic.

[ghost]

Things like this make me impatient for null-coelescing operators. I'll look deeper after the holiday.

[Conduitry]

Fixed in 3.16.7.