From 3f5616c141e451cd0736aa6ff339fb71c47b02e9 Mon Sep 17 00:00:00 2001 From: Caique Torres Date: Thu, 25 Apr 2024 11:55:46 -0300 Subject: [PATCH 1/5] fix: raising an error when mixing both old and new event-handling syntaxes --- .../svelte/messages/compile-errors/template.md | 4 ++++ packages/svelte/src/compiler/errors.js | 10 ++++++++++ .../src/compiler/phases/2-analyze/index.js | 11 +++++++++++ .../compiler/phases/2-analyze/validation.js | 18 +++++++++++++++++- packages/svelte/src/compiler/phases/types.d.ts | 3 +++ .../runes-legacy-syntax-warnings-2/errors.json | 14 ++++++++++++++ .../input.svelte | 11 +++++++++++ 7 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/errors.json create mode 100644 packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/input.svelte diff --git a/packages/svelte/messages/compile-errors/template.md b/packages/svelte/messages/compile-errors/template.md index c1b8c5f3c3be..293d4845cd7e 100644 --- a/packages/svelte/messages/compile-errors/template.md +++ b/packages/svelte/messages/compile-errors/template.md @@ -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%> diff --git a/packages/svelte/src/compiler/errors.js b/packages/svelte/src/compiler/errors.js index a00a2cd593b6..d2834d03d4c9 100644 --- a/packages/svelte/src/compiler/errors.js +++ b/packages/svelte/src/compiler/errors.js @@ -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 diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 2ed2288a8e72..a57d4943011e 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -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 @@ -1153,6 +1155,8 @@ const common_visitors = { }); if (is_event_attribute(node)) { + 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); @@ -1286,6 +1290,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--) { diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index 4f68b5f9edbe..2a2c7e5e8f95 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -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, @@ -104,6 +105,18 @@ function validate_element(node, context) { for (const attribute of node.attributes) { if (attribute.type === 'Attribute') { + const parent_type = node.type; + + // Don't warn on component events; these might not be under the author's control so the warning would be unactionable + if ( + (parent_type === 'RegularElement' || parent_type === 'SvelteElement') && + is_event_attribute(attribute) && + context.state.analysis.event_directive_node + ) { + const { event_directive_node } = context.state.analysis; + e.mixed_event_handler_syntaxes(event_directive_node, event_directive_node.name); + } + const is_expression = is_expression_attribute(attribute); if (context.state.analysis.runes && is_expression) { @@ -1204,10 +1217,13 @@ export const validation_runes = merge(validation, a11y_validators, { w.slot_element_deprecated(node); } }, - OnDirective(node, { path }) { + OnDirective(node, { state, path }) { const parent_type = path.at(-1)?.type; // Don't warn on component events; these might not be under the author's control so the warning would be unactionable if (parent_type === 'RegularElement' || parent_type === 'SvelteElement') { + if (state.analysis.uses_event_attributes) { + e.mixed_event_handler_syntaxes(node, node.name); + } w.event_directive_deprecated(node, node.name); } }, diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index 3e892fefef9e..ccfa8cb4cb50 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/packages/svelte/src/compiler/phases/types.d.ts @@ -2,6 +2,7 @@ import type { Binding, Css, Fragment, + OnDirective, RegularElement, SlotElement, SvelteElement, @@ -59,6 +60,8 @@ export interface ComponentAnalysis extends Analysis { uses_render_tags: boolean; needs_context: boolean; needs_props: boolean; + event_directive_node: OnDirective | null; + uses_event_attributes: boolean; custom_element: boolean | SvelteOptions['customElement']; /** If `true`, should append styles through JavaScript */ inject_styles: boolean; diff --git a/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/errors.json b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/errors.json new file mode 100644 index 000000000000..6a60c090031b --- /dev/null +++ b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/errors.json @@ -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 + } + } +] diff --git a/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/input.svelte b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/input.svelte new file mode 100644 index 000000000000..c6df23913743 --- /dev/null +++ b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings-2/input.svelte @@ -0,0 +1,11 @@ + + + + + + + + + From f9acfe62533963d168b6c87ec5bd49e476231478 Mon Sep 17 00:00:00 2001 From: Caique Torres Date: Thu, 25 Apr 2024 11:55:53 -0300 Subject: [PATCH 2/5] fix: fixed some broken tests related with the new way of dealing with old and new event-handling syntaxes --- .../event-attribute-delegation-2/main.svelte | 2 +- .../Component.svelte | 7 +++++++ .../event-attribute-delegation-4/main.svelte | 5 +++-- .../event-attribute-delegation-5/Button.svelte | 7 +++++++ .../Component.svelte | 7 +++++++ .../event-attribute-delegation-5/main.svelte | 15 ++++++++++----- .../Button.svelte | 7 +++++++ .../main.svelte | 17 +++++++++-------- .../input.svelte | 4 ++-- .../warnings.json | 2 +- .../runes-legacy-syntax-warnings/input.svelte | 3 +-- .../runes-legacy-syntax-warnings/warnings.json | 12 ++++++------ 12 files changed, 61 insertions(+), 27 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/Component.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Button.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Component.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/Button.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-2/main.svelte index 2a18005538a1..3e803a628375 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-2/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-2/main.svelte @@ -1,4 +1,4 @@ -
{ console.log('clicked div') }}> +
{ console.log('clicked div') }}> diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/Component.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/Component.svelte new file mode 100644 index 000000000000..d096bacaac8d --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/Component.svelte @@ -0,0 +1,7 @@ + + +
+ {@render children()} +
diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/main.svelte index 9697540e18c6..b7b33001684f 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-4/main.svelte @@ -1,12 +1,13 @@ -
console.log('div main 1')} on:click={() => console.log('div main 2')}> + console.log('div main 1')} on:click={() => console.log('div main 2')}> -
+ diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Button.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Button.svelte new file mode 100644 index 000000000000..3de20765c267 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Button.svelte @@ -0,0 +1,7 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Component.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Component.svelte new file mode 100644 index 000000000000..d096bacaac8d --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/Component.svelte @@ -0,0 +1,7 @@ + + +
+ {@render children()} +
diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/main.svelte index 6df18bff43b5..d7d70c0b0d98 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-delegation-5/main.svelte @@ -1,5 +1,10 @@ -
console.log('outer div onclick')}> -
console.log('inner div on:click')}> - -
-
+ + + console.log('outer div onclick')}> + console.log('inner div on:click')}> + + + diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/Button.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/Button.svelte new file mode 100644 index 000000000000..3de20765c267 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/Button.svelte @@ -0,0 +1,7 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/main.svelte index de589e3e40ee..b80782b28f68 100644 --- a/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/main.svelte @@ -1,21 +1,22 @@ - + - + - + - + diff --git a/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/input.svelte b/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/input.svelte index 4f23811a012d..37df598129f5 100644 --- a/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/input.svelte +++ b/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/input.svelte @@ -25,7 +25,7 @@
-
+
@@ -68,7 +68,7 @@
-
+
diff --git a/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/warnings.json b/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/warnings.json index 00adecd2eab8..ec348e03eac0 100644 --- a/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/warnings.json +++ b/packages/svelte/tests/validator/samples/a11y-click-events-have-key-events/warnings.json @@ -92,7 +92,7 @@ }, "end": { "line": 28, - "column": 32 + "column": 33 } } ] diff --git a/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/input.svelte b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/input.svelte index e1d0afed499c..55763285098a 100644 --- a/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/input.svelte +++ b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/input.svelte @@ -3,8 +3,7 @@ - - + diff --git a/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/warnings.json b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/warnings.json index b667f2e2dd4b..90fc8fe6f716 100644 --- a/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/warnings.json +++ b/packages/svelte/tests/validator/samples/runes-legacy-syntax-warnings/warnings.json @@ -3,36 +3,36 @@ "code": "slot_element_deprecated", "end": { "column": 13, - "line": 11 + "line": 10 }, "message": "Using `` 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 `` 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 } } ] From dcb6e2cb7788a203136dfb3f01e607eb2b88605a Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Sun, 28 Apr 2024 22:04:59 +0200 Subject: [PATCH 3/5] changeset --- .changeset/cool-poems-watch.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/cool-poems-watch.md diff --git a/.changeset/cool-poems-watch.md b/.changeset/cool-poems-watch.md new file mode 100644 index 000000000000..34527ad16f0e --- /dev/null +++ b/.changeset/cool-poems-watch.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: disallow mixing on:click and onclick syntax From a87006ecdb755e07cf9a451871df155f653bf993 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Sun, 28 Apr 2024 22:30:47 +0200 Subject: [PATCH 4/5] simplify --- .../src/compiler/phases/2-analyze/index.js | 7 +++++++ .../src/compiler/phases/2-analyze/validation.js | 17 +---------------- packages/svelte/src/compiler/phases/types.d.ts | 2 ++ 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index a57d4943011e..fa9db82752d7 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -496,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); } diff --git a/packages/svelte/src/compiler/phases/2-analyze/validation.js b/packages/svelte/src/compiler/phases/2-analyze/validation.js index 2a2c7e5e8f95..ab793bf1a7b4 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/validation.js +++ b/packages/svelte/src/compiler/phases/2-analyze/validation.js @@ -105,18 +105,6 @@ function validate_element(node, context) { for (const attribute of node.attributes) { if (attribute.type === 'Attribute') { - const parent_type = node.type; - - // Don't warn on component events; these might not be under the author's control so the warning would be unactionable - if ( - (parent_type === 'RegularElement' || parent_type === 'SvelteElement') && - is_event_attribute(attribute) && - context.state.analysis.event_directive_node - ) { - const { event_directive_node } = context.state.analysis; - e.mixed_event_handler_syntaxes(event_directive_node, event_directive_node.name); - } - const is_expression = is_expression_attribute(attribute); if (context.state.analysis.runes && is_expression) { @@ -1217,13 +1205,10 @@ export const validation_runes = merge(validation, a11y_validators, { w.slot_element_deprecated(node); } }, - OnDirective(node, { state, path }) { + OnDirective(node, { path }) { const parent_type = path.at(-1)?.type; // Don't warn on component events; these might not be under the author's control so the warning would be unactionable if (parent_type === 'RegularElement' || parent_type === 'SvelteElement') { - if (state.analysis.uses_event_attributes) { - e.mixed_event_handler_syntaxes(node, node.name); - } w.event_directive_deprecated(node, node.name); } }, diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index ccfa8cb4cb50..1a538aa6cec5 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/packages/svelte/src/compiler/phases/types.d.ts @@ -60,7 +60,9 @@ 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) */ uses_event_attributes: boolean; custom_element: boolean | SvelteOptions['customElement']; /** If `true`, should append styles through JavaScript */ From e372742f6952bcafc03edc7a683e7cd986df8d20 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Sun, 28 Apr 2024 22:38:07 +0200 Subject: [PATCH 5/5] fix --- packages/svelte/src/compiler/phases/2-analyze/index.js | 5 ++++- packages/svelte/src/compiler/phases/types.d.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index fa9db82752d7..8d3088806d19 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -1162,7 +1162,10 @@ const common_visitors = { }); if (is_event_attribute(node)) { - context.state.analysis.uses_event_attributes = true; + 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; diff --git a/packages/svelte/src/compiler/phases/types.d.ts b/packages/svelte/src/compiler/phases/types.d.ts index 1a538aa6cec5..ae346068c40e 100644 --- a/packages/svelte/src/compiler/phases/types.d.ts +++ b/packages/svelte/src/compiler/phases/types.d.ts @@ -62,7 +62,7 @@ export interface ComponentAnalysis extends Analysis { 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) */ + /** 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 */