diff --git a/.changeset/brown-months-flow.md b/.changeset/brown-months-flow.md new file mode 100644 index 0000000000..77a1781adb --- /dev/null +++ b/.changeset/brown-months-flow.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +feat: warn on invalid event handlers diff --git a/packages/svelte/messages/client-warnings/warnings.md b/packages/svelte/messages/client-warnings/warnings.md index cae88b0bef..3072bc4df1 100644 --- a/packages/svelte/messages/client-warnings/warnings.md +++ b/packages/svelte/messages/client-warnings/warnings.md @@ -4,6 +4,10 @@ > `%binding%` (%location%) is binding to a non-reactive property +## event_handler_invalid + +> %handler% should be a function. Did you mean to %suggestion%? + ## hydration_attribute_changed > The `%attribute%` attribute on `%html%` changed its value between server and client renders. The client value, `%value%`, will be ignored in favour of the server value 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 9510715a91..3a4ce97345 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 @@ -1,7 +1,8 @@ /** @import { Expression } from 'estree' */ -/** @import { Attribute, ExpressionMetadata, ExpressionTag, OnDirective, SvelteNode } from '#compiler' */ +/** @import { Attribute, ExpressionMetadata, ExpressionTag, SvelteNode } from '#compiler' */ /** @import { ComponentContext } from '../../types' */ -import { is_capture_event, is_passive_event } from '../../../../../../utils.js'; +import { is_capture_event } from '../../../../../../utils.js'; +import { dev, locator } from '../../../../../state.js'; import * as b from '../../../../../utils/builders.js'; /** @@ -136,9 +137,47 @@ export function build_event_handler(node, metadata, context) { } // wrap the handler in a function, so the expression is re-evaluated for each event - return b.function( - null, - [b.rest(b.id('$$args'))], - b.block([b.stmt(b.call(b.member(handler, b.id('apply'), false, true), b.this, b.id('$$args')))]) - ); + let call = b.call(b.member(handler, b.id('apply'), false, true), b.this, b.id('$$args')); + + if (dev) { + const loc = locator(/** @type {number} */ (node.start)); + + const remove_parens = + node.type === 'CallExpression' && + node.arguments.length === 0 && + node.callee.type === 'Identifier'; + + call = b.call( + '$.apply', + b.thunk(handler), + b.this, + b.id('$$args'), + b.id(context.state.analysis.name), + loc && b.array([b.literal(loc.line), b.literal(loc.column)]), + has_side_effects(node) && b.true, + remove_parens && b.true + ); + } + + return b.function(null, [b.rest(b.id('$$args'))], b.block([b.stmt(call)])); +} + +/** + * @param {Expression} node + */ +function has_side_effects(node) { + if ( + node.type === 'CallExpression' || + node.type === 'NewExpression' || + node.type === 'AssignmentExpression' || + node.type === 'UpdateExpression' + ) { + return true; + } + + if (node.type === 'SequenceExpression') { + return node.expressions.some(has_side_effects); + } + + return false; } diff --git a/packages/svelte/src/internal/client/dom/elements/events.js b/packages/svelte/src/internal/client/dom/elements/events.js index 2c9a63436d..7223c969e2 100644 --- a/packages/svelte/src/internal/client/dom/elements/events.js +++ b/packages/svelte/src/internal/client/dom/elements/events.js @@ -1,7 +1,11 @@ +/** @import { Location } from 'locate-character' */ import { teardown } from '../../reactivity/effects.js'; import { define_property, is_array } from '../../../shared/utils.js'; import { hydrating } from '../hydration.js'; import { queue_micro_task } from '../task.js'; +import { dev_current_component_function } from '../../runtime.js'; +import { FILENAME } from '../../../../constants.js'; +import * as w from '../../warnings.js'; /** @type {Set} */ export const all_registered_events = new Set(); @@ -273,3 +277,53 @@ export function handle_event_propagation(event) { current_target = handler_element; } } + +/** + * In dev, warn if an event handler is not a function, as it means the + * user probably called the handler or forgot to add a `() =>` + * @param {() => (event: Event, ...args: any) => void} thunk + * @param {EventTarget} element + * @param {[Event, ...any]} args + * @param {any} component + * @param {[number, number]} [loc] + * @param {boolean} [remove_parens] + */ +export function apply( + thunk, + element, + args, + component, + loc, + has_side_effects = false, + remove_parens = false +) { + let handler; + let error; + + try { + handler = thunk(); + } catch (e) { + error = e; + } + + if (typeof handler === 'function') { + handler.apply(element, args); + } else if (has_side_effects || handler != null) { + const filename = component?.[FILENAME]; + const location = filename + ? loc + ? ` at ${filename}:${loc[0]}:${loc[1]}` + : ` in ${filename}` + : ''; + + const event_name = args[0].type; + const description = `\`${event_name}\` handler${location}`; + const suggestion = remove_parens ? 'remove the trailing `()`' : 'add a leading `() =>`'; + + w.event_handler_invalid(description, suggestion); + + if (error) { + throw error; + } + } +} diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index 489d45df02..5739e9646c 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -36,7 +36,7 @@ export { set_checked } from './dom/elements/attributes.js'; export { set_class, set_svg_class, set_mathml_class, toggle_class } from './dom/elements/class.js'; -export { event, delegate, replay_events } from './dom/elements/events.js'; +export { apply, event, delegate, replay_events } from './dom/elements/events.js'; export { autofocus, remove_textarea_child } from './dom/elements/misc.js'; export { set_style } from './dom/elements/style.js'; export { animation, transition } from './dom/elements/transitions.js'; diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 6ec5946070..6b5fc8dae4 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -91,19 +91,24 @@ let stack = []; * @returns {void} */ export function update_derived(derived) { - if (DEV) { - if (stack.includes(derived)) { - e.derived_references_self(); - } + var value; - stack.push(derived); - } + if (DEV) { + try { + if (stack.includes(derived)) { + e.derived_references_self(); + } - destroy_derived_children(derived); - var value = update_reaction(derived); + stack.push(derived); - if (DEV) { - stack.pop(); + destroy_derived_children(derived); + value = update_reaction(derived); + } finally { + stack.pop(); + } + } else { + destroy_derived_children(derived); + value = update_reaction(derived); } var status = diff --git a/packages/svelte/src/internal/client/warnings.js b/packages/svelte/src/internal/client/warnings.js index 23c00f971d..b7268ee1d7 100644 --- a/packages/svelte/src/internal/client/warnings.js +++ b/packages/svelte/src/internal/client/warnings.js @@ -19,6 +19,20 @@ export function binding_property_non_reactive(binding, location) { } } +/** + * %handler% should be a function. Did you mean to %suggestion%? + * @param {string} handler + * @param {string} suggestion + */ +export function event_handler_invalid(handler, suggestion) { + if (DEV) { + console.warn(`%c[svelte] event_handler_invalid\n%c${handler} should be a function. Did you mean to ${suggestion}?`, bold, normal); + } else { + // TODO print a link to the documentation + console.warn("event_handler_invalid"); + } +} + /** * The `%attribute%` attribute on `%html%` changed its value between server and client renders. The client value, `%value%`, will be ignored in favour of the server value * @param {string} attribute diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/Button.svelte b/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/Button.svelte new file mode 100644 index 0000000000..47dee4df1a --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/Button.svelte @@ -0,0 +1,7 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/_config.js b/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/_config.js new file mode 100644 index 0000000000..7f8e745e01 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/_config.js @@ -0,0 +1,17 @@ +import { test } from '../../test'; + +export default test({ + mode: ['client'], + + compileOptions: { + dev: true + }, + + test({ assert, target, warnings }) { + target.querySelector('button')?.click(); + + assert.deepEqual(warnings, [ + '`click` handler at Button.svelte:5:9 should be a function. Did you mean to add a leading `() =>`?' + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/main.svelte new file mode 100644 index 0000000000..2398cec9a1 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-component-invalid-warning/main.svelte @@ -0,0 +1,9 @@ + + + diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-warning/_config.js b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-warning/_config.js new file mode 100644 index 0000000000..c1461d4ada --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-warning/_config.js @@ -0,0 +1,17 @@ +import { test } from '../../test'; + +export default test({ + mode: ['client'], + + compileOptions: { + dev: true + }, + + test({ assert, target, warnings }) { + target.querySelector('button')?.click(); + + assert.deepEqual(warnings, [ + '`click` handler at main.svelte:9:17 should be a function. Did you mean to remove the trailing `()`?' + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-warning/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-warning/main.svelte new file mode 100644 index 0000000000..ea0b320c18 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-handler-invalid-warning/main.svelte @@ -0,0 +1,11 @@ + + +