Skip to content

Commit

Permalink
fix: disallow mixing event-handling syntaxes (#11295)
Browse files Browse the repository at this point in the history
Closes #11262

---------

Co-authored-by: Simon Holthausen <simon.holthausen@vercel.com>
  • Loading branch information
caiquetorres and dummdidumm committed Apr 28, 2024
1 parent bda32ed commit 68071f7
Show file tree
Hide file tree
Showing 20 changed files with 132 additions and 27 deletions.
5 changes: 5 additions & 0 deletions .changeset/cool-poems-watch.md
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: disallow mixing on:click and onclick syntax
4 changes: 4 additions & 0 deletions packages/svelte/messages/compile-errors/template.md
Expand Up @@ -172,6 +172,10 @@

> `let:` directive at invalid position
## mixed_event_handler_syntaxes

> Mixing old (on:%name%) and new syntaxes for event handling is not allowed. Use only the on%name% syntax.
## node_invalid_placement

> %thing% is invalid inside <%parent%>
Expand Down
10 changes: 10 additions & 0 deletions packages/svelte/src/compiler/errors.js
Expand Up @@ -918,6 +918,16 @@ export function let_directive_invalid_placement(node) {
e(node, "let_directive_invalid_placement", "`let:` directive at invalid position");
}

/**
* Mixing old (on:%name%) and new syntaxes for event handling is not allowed. Use only the on%name% syntax.
* @param {null | number | NodeLike} node
* @param {string} name
* @returns {never}
*/
export function mixed_event_handler_syntaxes(node, name) {
e(node, "mixed_event_handler_syntaxes", `Mixing old (on:${name}) and new syntaxes for event handling is not allowed. Use only the on${name} syntax.`);
}

/**
* %thing% is invalid inside <%parent%>
* @param {null | number | NodeLike} node
Expand Down
21 changes: 21 additions & 0 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Expand Up @@ -372,6 +372,8 @@ export function analyze_component(root, source, options) {
uses_render_tags: false,
needs_context: false,
needs_props: false,
event_directive_node: null,
uses_event_attributes: false,
custom_element: options.customElementOptions ?? options.customElement,
inject_styles: options.css === 'injected' || options.customElement,
accessors: options.customElement
Expand Down Expand Up @@ -494,6 +496,13 @@ export function analyze_component(root, source, options) {
analysis.reactive_statements = order_reactive_statements(analysis.reactive_statements);
}

if (analysis.event_directive_node && analysis.uses_event_attributes) {
e.mixed_event_handler_syntaxes(
analysis.event_directive_node,
analysis.event_directive_node.name
);
}

if (analysis.uses_render_tags && (analysis.uses_slots || analysis.slot_names.size > 0)) {
e.slot_snippet_conflict(analysis.slot_names.values().next().value);
}
Expand Down Expand Up @@ -1153,6 +1162,11 @@ const common_visitors = {
});

if (is_event_attribute(node)) {
const parent = context.path.at(-1);
if (parent?.type === 'RegularElement' || parent?.type === 'SvelteElement') {
context.state.analysis.uses_event_attributes = true;
}

const expression = node.value[0].expression;

const delegated_event = get_delegated_event(node.name.slice(2), expression, context);
Expand Down Expand Up @@ -1286,6 +1300,13 @@ const common_visitors = {

context.next();
},
OnDirective(node, { state, path, next }) {
const parent = path.at(-1);
if (parent?.type === 'SvelteElement' || parent?.type === 'RegularElement') {
state.analysis.event_directive_node ??= node;
}
next();
},
BindDirective(node, context) {
let i = context.path.length;
while (i--) {
Expand Down
Expand Up @@ -8,6 +8,7 @@ import * as e from '../../errors.js';
import {
extract_identifiers,
get_parent,
is_event_attribute,
is_expression_attribute,
is_text_attribute,
object,
Expand Down
5 changes: 5 additions & 0 deletions packages/svelte/src/compiler/phases/types.d.ts
Expand Up @@ -2,6 +2,7 @@ import type {
Binding,
Css,
Fragment,
OnDirective,
RegularElement,
SlotElement,
SvelteElement,
Expand Down Expand Up @@ -59,6 +60,10 @@ export interface ComponentAnalysis extends Analysis {
uses_render_tags: boolean;
needs_context: boolean;
needs_props: boolean;
/** Set to the first event directive (on:x) found on a DOM element in the code */
event_directive_node: OnDirective | null;
/** true if uses event attributes (onclick) on a DOM element */
uses_event_attributes: boolean;
custom_element: boolean | SvelteOptions['customElement'];
/** If `true`, should append styles through JavaScript */
inject_styles: boolean;
Expand Down
@@ -1,4 +1,4 @@
<div on:click={(e) => { console.log('clicked div') }}>
<div onclick={(e) => { console.log('clicked div') }}>
<button onclick={(e) => { console.log('clicked button'); e.stopPropagation() }}>
Button
</button>
Expand Down
@@ -0,0 +1,7 @@
<script>
const { children, ...props } = $props();
</script>

<div {...props} on:click>
{@render children()}
</div>
@@ -1,12 +1,13 @@
<script>
import Component from "./Component.svelte";
import Sub from "./sub.svelte";
</script>

<svelte:window onclick="{() => console.log('window main')}" />
<svelte:document onclick="{() => console.log('document main')}" />

<div on:click={() => console.log('div main 1')} on:click={() => console.log('div main 2')}>
<Component on:click={() => console.log('div main 1')} on:click={() => console.log('div main 2')}>
<button onclick={() => console.log('button main')}>main</button>
</div>
</Component>

<Sub />
@@ -0,0 +1,7 @@
<script>
const { children, ...props } = $props();
</script>

<button {...props} on:click>
{@render children()}
</button>
@@ -0,0 +1,7 @@
<script>
const { children, ...props } = $props();
</script>

<div {...props} on:click>
{@render children()}
</div>
@@ -1,5 +1,10 @@
<div onclick={() => console.log('outer div onclick')}>
<div on:click={() => console.log('inner div on:click')}>
<button onclick={() => console.log('button onclick')} on:click={() => console.log('button on:click')}>main</button>
</div>
</div>
<script>
import Component from "./Component.svelte";
import Button from "./Button.svelte";
</script>

<Component onclick={() => console.log('outer div onclick')}>
<Component on:click={() => console.log('inner div on:click')}>
<Button onclick={() => console.log('button onclick')} on:click={() => console.log('button on:click')}>main</Button>
</Component>
</Component>
@@ -0,0 +1,7 @@
<script>
const { children, ...props } = $props();
</script>

<button {...props} on:click>
{@render children()}
</button>
@@ -1,21 +1,22 @@
<script>
import Button from './Button.svelte';
let text = $state('click me');
let text2 = $state('');
let spread = { onclick: () => text = 'click spread' };
</script>

<button onclick={() => text = 'click onclick'} {...spread}>
<Button onclick={() => text = 'click onclick'} {...spread}>
{text}
</button>
</Button>

<button {...spread} onclick={() => text = 'click onclick'}>
<Button {...spread} onclick={() => text = 'click onclick'}>
{text}
</button>
</Button>

<button onclick={() => text = 'click onclick'} {...spread} on:click={() => text2 = '!'}>
<Button onclick={() => text = 'click onclick'} {...spread} on:click={() => text2 = '!'}>
{text}{text2}
</button>
</Button>

<button on:click={() => text2 = '?'} {...spread} onclick={() => text = 'click onclick'}>
<Button on:click={() => text2 = '?'} {...spread} onclick={() => text = 'click onclick'}>
{text}{text2}
</button>
</Button>
Expand Up @@ -25,7 +25,7 @@
<!-- svelte-ignore a11y_no_noninteractive_element_interactions -->
<footer on:click={noop}></footer>
<!-- svelte-ignore a11y_no_noninteractive_element_interactions -->
<footer onclick={noop}></footer>
<footer on:click={noop}></footer>

<!-- should not warn -->
<div class="foo"></div>
Expand Down Expand Up @@ -68,7 +68,7 @@
<div on:click={noop} role="presentation"></div>
<div on:click={noop} role="none"></div>
<div on:click={noop} role={dynamicRole}></div>
<div onclick={noop} role={dynamicRole}></div>
<div on:click={noop} role={dynamicRole}></div>

<!-- svelte-ignore a11y_no_static_element_interactions -->
<svelte:element this={Math.random() ? 'button' : 'div'} on:click={noop} />
Expand Up @@ -92,7 +92,7 @@
},
"end": {
"line": 28,
"column": 32
"column": 33
}
}
]
@@ -0,0 +1,14 @@
[
{
"code": "mixed_event_handler_syntaxes",
"message": "Mixing old (on:click) and new syntaxes for event handling is not allowed. Use only the onclick syntax.",
"start": {
"line": 11,
"column": 8
},
"end": {
"line": 11,
"column": 22
}
}
]
@@ -0,0 +1,11 @@
<script>
let { foo } = $props();
</script>

<!-- ok -->
<button onclick={foo}>click me</button>
<Button on:click={foo}>click me</Button>
<Button on:click={foo}>click me</Button>

<!-- error -->
<button on:click={foo}>click me</button>
Expand Up @@ -3,8 +3,7 @@
</script>

<!-- ok -->
<button onclick={foo}>click me</button>
<Button onclick={foo}>click me</Button>
<Button on:click={foo}>click me</Button>
<Button on:click={foo}>click me</Button>

<!-- warn -->
Expand Down
Expand Up @@ -3,36 +3,36 @@
"code": "slot_element_deprecated",
"end": {
"column": 13,
"line": 11
"line": 10
},
"message": "Using `<slot>` to render parent content is deprecated. Use `{@render ...}` tags instead.",
"start": {
"column": 0,
"line": 11
"line": 10
}
},
{
"code": "slot_element_deprecated",
"end": {
"column": 24,
"line": 12
"line": 11
},
"message": "Using `<slot>` to render parent content is deprecated. Use `{@render ...}` tags instead.",
"start": {
"column": 0,
"line": 12
"line": 11
}
},
{
"code": "event_directive_deprecated",
"end": {
"column": 22,
"line": 13
"line": 12
},
"message": "Using `on:click` to listen to the click event is deprecated. Use the event attribute `onclick` instead.",
"start": {
"column": 8,
"line": 13
"line": 12
}
}
]

0 comments on commit 68071f7

Please sign in to comment.