From 40e690835f607bc8e618a8ff703ff4cb83894935 Mon Sep 17 00:00:00 2001 From: adiguba Date: Thu, 6 Mar 2025 16:08:11 +0100 Subject: [PATCH 1/4] fix null and warning for local handlers --- .../client/visitors/shared/events.js | 24 +++++++++++++------ .../internal/client/dom/elements/events.js | 11 ++++----- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js index f23f7548ece1..a88d656ddb17 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js @@ -46,8 +46,12 @@ export function visit_event_attribute(node, context) { // When we hoist a function we assign an array with the function and all // hoisted closure params. - const args = [handler, ...hoisted_params]; - delegated_assignment = b.array(args); + if (hoisted_params) { + const args = [handler, ...hoisted_params]; + delegated_assignment = b.array(args); + } else { + delegated_assignment = handler; + } } else { delegated_assignment = handler; } @@ -123,11 +127,17 @@ export function build_event_handler(node, metadata, context) { } // function declared in the script - if ( - handler.type === 'Identifier' && - context.state.scope.get(handler.name)?.declaration_kind !== 'import' - ) { - return handler; + if (handler.type === 'Identifier') { + const kind = context.state.scope.get(handler.name)?.declaration_kind; + if (kind === 'function') { + return handler; + } + // local variable can be assigned directly + // except in dev mode where when need $.apply() + // in order to handle warnings. + if (!dev && kind !== 'import') { + return handler; + } } if (metadata.has_call) { diff --git a/packages/svelte/src/internal/client/dom/elements/events.js b/packages/svelte/src/internal/client/dom/elements/events.js index 25ece5f569d7..0c1bb1dada83 100644 --- a/packages/svelte/src/internal/client/dom/elements/events.js +++ b/packages/svelte/src/internal/client/dom/elements/events.js @@ -238,7 +238,7 @@ export function handle_event_propagation(event) { var delegated = current_target['__' + event_name]; if ( - delegated !== undefined && + delegated != null && (!(/** @type {any} */ (current_target).disabled) || // DOM could've been updated already by the time this is reached, so we check this as well // -> the target could not have been disabled because it emits the event in the first place @@ -311,13 +311,11 @@ export function apply( error = e; } - if (typeof handler === 'function') { - handler.apply(element, args); - } else if (has_side_effects || handler != null || error) { + if (typeof handler !== 'function' && (has_side_effects || handler != null || error)) { const filename = component?.[FILENAME]; const location = loc ? ` at ${filename}:${loc[0]}:${loc[1]}` : ` in ${filename}`; - - const event_name = args[0].type; + const phase = args[0]?.eventPhase < Event.BUBBLING_PHASE ? 'capture' : ''; + const event_name = args[0]?.type + phase; const description = `\`${event_name}\` handler${location}`; const suggestion = remove_parens ? 'remove the trailing `()`' : 'add a leading `() =>`'; @@ -327,4 +325,5 @@ export function apply( throw error; } } + handler?.apply(element, args); } From 8bc9587536a2bfb3f1c2a28d1c1fd8c3144c0f89 Mon Sep 17 00:00:00 2001 From: adiguba Date: Thu, 6 Mar 2025 16:08:22 +0100 Subject: [PATCH 2/4] test --- .../event-handler-invalid-values/_config.js | 48 +++++++++++++++++++ .../event-handler-invalid-values/main.svelte | 10 ++++ 2 files changed, 58 insertions(+) create mode 100644 packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/main.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/_config.js b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/_config.js new file mode 100644 index 000000000000..d53812d4c39e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/_config.js @@ -0,0 +1,48 @@ +import { assertType } from 'vitest'; +import { test } from '../../test'; + +export default test({ + mode: ['client'], + + compileOptions: { + dev: true + }, + + test({ assert, target, warnings, logs }) { + /** @type {any} */ + let error = null; + + const handler = (/** @type {any} */ e) => { + error = e.error; + e.stopImmediatePropagation(); + }; + + window.addEventListener('error', handler, true); + + const [b1, b2, b3] = target.querySelectorAll('button'); + + b1.click(); + assert.deepEqual(logs, []); + assert.equal(error, null); + + error = null; + logs.length = 0; + + b2.click(); + assert.deepEqual(logs, ['clicked']); + assert.equal(error, null); + + error = null; + logs.length = 0; + + b3.click(); + assert.deepEqual(logs, []); + assert.deepEqual(warnings, [ + '`click` handler at main.svelte:10:17 should be a function. Did you mean to add a leading `() =>`?' + ]); + assert.isNotNull(error); + assert.match(error.message, /is not a function/); + + window.removeEventListener('error', handler, true); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/main.svelte new file mode 100644 index 000000000000..f6e344ece8cf --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-values/main.svelte @@ -0,0 +1,10 @@ + + + + + From 2671dc697e79e7e83c4fa792cc03d1f702e45ccf Mon Sep 17 00:00:00 2001 From: adiguba Date: Thu, 6 Mar 2025 16:09:26 +0100 Subject: [PATCH 3/4] changeset --- .changeset/shy-mirrors-remain.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/shy-mirrors-remain.md diff --git a/.changeset/shy-mirrors-remain.md b/.changeset/shy-mirrors-remain.md new file mode 100644 index 000000000000..028f7beb6898 --- /dev/null +++ b/.changeset/shy-mirrors-remain.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: null and warnings for local handlers From a6a8339797f7d1120f89f09efc1e556294e80feb Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Fri, 7 Mar 2025 09:25:20 -0500 Subject: [PATCH 4/4] treat `let handler = () => {...}` the same as `function handler() {...}` --- .../3-transform/client/visitors/shared/events.js | 8 +++++--- packages/svelte/src/compiler/phases/scope.js | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js index a88d656ddb17..2667a96f6aef 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/events.js @@ -128,14 +128,16 @@ export function build_event_handler(node, metadata, context) { // function declared in the script if (handler.type === 'Identifier') { - const kind = context.state.scope.get(handler.name)?.declaration_kind; - if (kind === 'function') { + const binding = context.state.scope.get(handler.name); + + if (binding?.is_function()) { return handler; } + // local variable can be assigned directly // except in dev mode where when need $.apply() // in order to handle warnings. - if (!dev && kind !== 'import') { + if (!dev && binding?.declaration_kind !== 'import') { return handler; } } diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index 7d9f90982afb..a5227c1b5113 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -79,6 +79,21 @@ export class Binding { get updated() { return this.mutated || this.reassigned; } + + is_function() { + if (this.reassigned) { + // even if it's reassigned to another function, + // we can't use it directly as e.g. an event handler + return false; + } + + if (this.declaration_kind === 'function') { + return true; + } + + const type = this.initial?.type; + return type === 'ArrowFunctionExpression' || type === 'FunctionExpression'; + } } export class Scope {