fix: wrong function hoisting when mixing event-handling syntaxes

[caiquetorres]

Fixes #11262

The bug occurs as follows: When only the old event handling system is used (specified by on: followed by the event name), the compiler designates the delegated event as non-hoistable. However, when the new event handling system is employed, it is marked as hoistable. The issue arises when these two systems are mixed.

Currently, there are a few validations in place, but none address this specific interaction between the old and new systems. Consequently, the compiler defaults to labeling any function or callback as hoistable when the new event-handling system is utilized. This causes a problem with the old system, which fails to handle the arguments correctly, as they are not passed to the function or callback as expected.

Essentially, the PR issues an error to the user about the use of both old and new event-handling syntaxes and advises against using them simultaneously within the same component. This error only triggers when mixed syntaxes are used within the same component and on regular/Svelte elements.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: e372742

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dummdidumm]

Thank you, but as I said on the issue (and it seems Rich agrees) I personally would prefer this to be a compiler error instead ("cannot mix old and new event handling syntax in the same component. Migrate towards event attributes completely" or sth like that).

[caiquetorres]

Thank you, but as I said on the issue (and it seems Rich agrees) I personally would prefer this to be a compiler error instead ("cannot mix old and new event handling syntax in the same component. Migrate towards event attributes completely" or sth like that).

@dummdidumm We can traverse the AST to detect the usage of the old event system. If so, we can flag it within the analysis object. During a second traversal of the tree, if we encounter occurrences of the new event handling, we can raise a compile error.

Something like that:

walk(/** @type {import('#compiler').SvelteNode} */ (ast), state, {
  OnDirective(_, context) {
    context.state.analysis.uses_directives = true;
    context.next();
  }
});

[dummdidumm]

I'm pretty sure we can do it in one go using two variables on the analysis object: event_directive_node which is set to the first event directive we come across, and uses_event_attributes which is set to true when we come across one. Then error pointing to the event directive node if uses_event_attributes is true and event_directive_node is set.

[caiquetorres]

I'm pretty sure we can do it in one go using two variables on the analysis object: event_directive_node which is set to the first event directive we come across, and uses_event_attributes which is set to true when we come across one. Then error pointing to the event directive node if uses_event_attributes is true and event_directive_node is set.

@dummdidumm, I implemented the error for detecting both occurrences of event-handling syntaxes applying your suggestions. However, I encountered an issue: numerous tests utilize both syntaxes. To prevent potential side effects, given the uncertainty of their usage, I opted for a warning instead. It alerts users about the presence of both syntaxes. Additionally, I kept the previous fix allowing their combined use within the same component. Could you please provide any suggestions or feedback on this approach?

[dummdidumm]

Which tests are those? I'm ok with adjusting them. A warning doesn't buy us much because we already warn on all event directives as soon as someone is entering runes mode.

[caiquetorres]

Here are the ones I found:

• event-attribute-delegation-4
• event-attribute-delegation-5
• event-attribute-spread-collision
• runes-legacy-syntax-warnings

[caiquetorres]

@dummdidumm, I fixed the tests as follows: for tests using both old and new event handling syntaxes in regular elements, I replaced these elements with components to account for scenarios where the components may not be under the author's control. Maintaining the original logic of the tests. Additionally, I added more tests to confirm that warnings and the new compiler errors trigger as expected.

[olehmisar]

I personally would prefer this to be a compiler error instead

@dummdidumm what if user wants to use <button onclick={...}> and <VeryOldLibraryComponent on:someevent={...}> in the same component?

[dummdidumm]

what if user wants to use and in the same component?

This case is already handled correctly today and shouldn't be a compiler error

[dummdidumm]

Thank you!

[adiguba]

Hello,

Not sure if I should post this here or create a new issue, but I think there is a major flaw about this.

Imagine the following component using Svelte 4 syntax :

// Button.svelte
<button on:click><slot/></button>

Currently, the migration tool will generate this code for Svelte 5 :

// Button.svelte
<script>
	let { children, onclick } = $props();
</script>

<button {onclick}>{@render children()}</button>

Fine. This is correct... but it implies that all other components that use it will also need to be updated to the new syntax.
For example the following component is now broken because on:click will be ignored :

// App.svelte
<script>
	import Button from './Button.svelte';
</script>

<Button on:click={()=>alert("clicked")} />

Demo here : REPL

This is particularly problematic on large projects where components will be migrated gradually...

And there's worse: if the migration tool changes on:click to onclick on elements, it is not the same for components where on:click is kept as is.

=> I think that this error should not apply to the forward directive, and that <button on:click> should be migrated to <button {onclick} on:click> for compatibility reasons, possibly with a warning.

<button {onclick}>click</button>   // OK
<button {onclick} on:click={onclick}>click</button>  // ERROR
<button {onclick} on:click>click</button>  // WARNING