fix: better event handling (#12722)

* simplify

* fix/simplify

* fix/simplify

* start getting a grip of this mess

* tidy up

* more

* more

* more

* tidy up

* make things a bit less weird

* tweak

* more

* more

* add once once

* consolidate event handling code

* some progress. man, this stuff is entangled

* more

* tidy up

* simplify

* simplify

* more

* fix

* fix test names

* fix a bug

* tidy up

* changeset

* simplify

* regenerate

* tidy up

* tidy up

* tidy up

* simplify

* the module declaration case is already accounted for, above

* simplify/document

* typo

* "hoistable" is a misnomer

* hoist non_hoistable, rename

* more typos

* tweak

* regenerate

Author: Rich-Harris

.changeset/smart-cars-know.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: avoid recreating handlers for component events

.changeset/wild-pumas-count.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: call correct event handler for properties of non-reactive objects

packages/svelte/src/compiler/phases/2-analyze/visitors/Attribute.js

@@ -40,8 +40,8 @@ export function Attribute(node, context) {
 			const delegated_event = get_delegated_event(node.name.slice(2), expression, context);
 
 			if (delegated_event !== null) {
-				if (delegated_event.type === 'hoistable') {
-					delegated_event.function.metadata.hoistable = true;
+				if (delegated_event.hoisted) {
+					delegated_event.function.metadata.hoisted = true;
 				}
 
 				node.metadata.delegated = delegated_event;
@@ -50,6 +50,9 @@ export function Attribute(node, context) {
 	}
 }
 
+/** @type {DelegatedEvent} */
+const unhoisted = { hoisted: false };
+
 /**
  * Checks if given event attribute can be delegated/hoisted and returns the corresponding info if so
  * @param {string} event_name
@@ -58,26 +61,24 @@ export function Attribute(node, context) {
  * @returns {null | DelegatedEvent}
  */
 function get_delegated_event(event_name, handler, context) {
-	// Handle delegated event handlers. Bail-out if not a delegated event.
+	// Handle delegated event handlers. Bail out if not a delegated event.
 	if (!handler || !is_delegated(event_name)) {
 		return null;
 	}
 
-	// If we are not working with a RegularElement, then bail-out.
+	// If we are not working with a RegularElement, then bail out.
 	const element = context.path.at(-1);
 	if (element?.type !== 'RegularElement') {
 		return null;
 	}
 
-	/** @type {DelegatedEvent} */
-	const non_hoistable = { type: 'non-hoistable' };
 	/** @type {FunctionExpression | FunctionDeclaration | ArrowFunctionExpression | null} */
 	let target_function = null;
 	let binding = null;
 
 	if (element.metadata.has_spread) {
 		// event attribute becomes part of the dynamic spread array
-		return non_hoistable;
+		return unhoisted;
 	}
 
 	if (handler.type === 'ArrowFunctionExpression' || handler.type === 'FunctionExpression') {
@@ -86,14 +87,14 @@ function get_delegated_event(event_name, handler, context) {
 		binding = context.state.scope.get(handler.name);
 
 		if (context.state.analysis.module.scope.references.has(handler.name)) {
-			// If a binding with the same name is referenced in the module scope (even if not declared there), bail-out
-			return non_hoistable;
+			// If a binding with the same name is referenced in the module scope (even if not declared there), bail out
+			return unhoisted;
 		}
 
 		if (binding != null) {
 			for (const { path } of binding.references) {
 				const parent = path.at(-1);
-				if (parent == null) return non_hoistable;
+				if (parent === undefined) return unhoisted;
 
 				const grandparent = path.at(-2);
 
@@ -120,17 +121,17 @@ function get_delegated_event(event_name, handler, context) {
 						element.metadata.has_spread ||
 						!is_delegated(event_name)
 					) {
-						return non_hoistable;
+						return unhoisted;
 					}
 				} else if (parent.type !== 'FunctionDeclaration' && parent.type !== 'VariableDeclarator') {
-					return non_hoistable;
+					return unhoisted;
 				}
 			}
 		}
 
-		// If the binding is exported, bail-out
+		// If the binding is exported, bail out
 		if (context.state.analysis.exports.find((node) => node.name === handler.name)) {
-			return non_hoistable;
+			return unhoisted;
 		}
 
 		if (binding !== null && binding.initial !== null && !binding.mutated && !binding.is_called) {
@@ -146,27 +147,25 @@ function get_delegated_event(event_name, handler, context) {
 		}
 	}
 
-	// If we can't find a function, bail-out
-	if (target_function == null) return non_hoistable;
-	// If the function is marked as non-hoistable, bail-out
-	if (target_function.metadata.hoistable === 'impossible') return non_hoistable;
-	// If the function has more than one arg, then bail-out
-	if (target_function.params.length > 1) return non_hoistable;
+	// If we can't find a function, or the function has multiple parameters, bail out
+	if (target_function == null || target_function.params.length > 1) {
+		return unhoisted;
+	}
 
 	const visited_references = new Set();
 	const scope = target_function.metadata.scope;
 	for (const [reference] of scope.references) {
-		// Bail-out if the arguments keyword is used
-		if (reference === 'arguments') return non_hoistable;
-		// Bail-out if references a store subscription
-		if (scope.get(`$${reference}`)?.kind === 'store_sub') return non_hoistable;
+		// Bail out if the arguments keyword is used
+		if (reference === 'arguments') return unhoisted;
+		// Bail out if references a store subscription
+		if (scope.get(`$${reference}`)?.kind === 'store_sub') return unhoisted;
 
 		const binding = scope.get(reference);
 		const local_binding = context.state.scope.get(reference);
 
 		// If we are referencing a binding that is shadowed in another scope then bail out.
 		if (local_binding !== null && binding !== null && local_binding.node !== binding.node) {
-			return non_hoistable;
+			return unhoisted;
 		}
 
 		// If we have multiple references to the same store using $ prefix, bail out.
@@ -175,30 +174,30 @@ function get_delegated_event(event_name, handler, context) {
 			binding.kind === 'store_sub' &&
 			visited_references.has(reference.slice(1))
 		) {
-			return non_hoistable;
+			return unhoisted;
 		}
 
-		// If we reference the index within an each block, then bail-out.
-		if (binding !== null && binding.initial?.type === 'EachBlock') return non_hoistable;
+		// If we reference the index within an each block, then bail out.
+		if (binding !== null && binding.initial?.type === 'EachBlock') return unhoisted;
 
 		if (
 			binding !== null &&
-			// Bail-out if the the binding is a rest param
+			// Bail out if the the binding is a rest param
 			(binding.declaration_kind === 'rest_param' ||
-				// Bail-out if we reference anything from the EachBlock (for now) that mutates in non-runes mode,
+				// Bail out if we reference anything from the EachBlock (for now) that mutates in non-runes mode,
 				(((!context.state.analysis.runes && binding.kind === 'each') ||
 					// or any normal not reactive bindings that are mutated.
 					binding.kind === 'normal' ||
 					// or any reactive imports (those are rewritten) (can only happen in legacy mode)
 					binding.kind === 'legacy_reactive_import') &&
 					binding.mutated))
 		) {
-			return non_hoistable;
+			return unhoisted;
 		}
 		visited_references.add(reference);
 	}
 
-	return { type: 'hoistable', function: target_function };
+	return { hoisted: true, function: target_function };
 }
 
 /**

packages/svelte/src/compiler/phases/2-analyze/visitors/shared/function.js

@@ -8,9 +8,8 @@
 export function visit_function(node, context) {
 	// TODO retire this in favour of a more general solution based on bindings
 	node.metadata = {
-		// module context -> already hoisted
-		hoistable: context.state.ast_type === 'module' ? 'impossible' : false,
-		hoistable_params: [],
+		hoisted: false,
+		hoisted_params: [],
 		scope: context.state.scope
 	};
 

packages/svelte/src/compiler/phases/3-transform/client/utils.js

@@ -481,7 +481,7 @@ export function build_proxy_reassignment(value, proxy_reference) {
  * @param {ComponentContext} context
  * @returns {Pattern[]}
  */
-function get_hoistable_params(node, context) {
+function get_hoisted_params(node, context) {
 	const scope = context.state.scope;
 
 	/** @type {Identifier[]} */
@@ -549,15 +549,15 @@ function get_hoistable_params(node, context) {
  * @param {ComponentContext} context
  * @returns {Pattern[]}
  */
-export function build_hoistable_params(node, context) {
-	const hoistable_params = get_hoistable_params(node, context);
-	node.metadata.hoistable_params = hoistable_params;
+export function build_hoisted_params(node, context) {
+	const hoisted_params = get_hoisted_params(node, context);
+	node.metadata.hoisted_params = hoisted_params;
 
 	/** @type {Pattern[]} */
 	const params = [];
 
 	if (node.params.length === 0) {
-		if (hoistable_params.length > 0) {
+		if (hoisted_params.length > 0) {
 			// For the event object
 			params.push(b.id('_'));
 		}
@@ -567,7 +567,7 @@ export function build_hoistable_params(node, context) {
 		}
 	}
 
-	params.push(...hoistable_params);
+	params.push(...hoisted_params);
 	return params;
 }
 

packages/svelte/src/compiler/phases/3-transform/client/visitors/Attribute.js

@@ -1,14 +1,14 @@
 /** @import { Attribute } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
 import { is_event_attribute } from '../../../../utils/ast.js';
-import { build_event_attribute } from './shared/element.js';
+import { visit_event_attribute } from './shared/events.js';
 
 /**
  * @param {Attribute} node
  * @param {ComponentContext} context
  */
 export function Attribute(node, context) {
 	if (is_event_attribute(node)) {
-		build_event_attribute(node, context);
+		visit_event_attribute(node, context);
 	}
 }

packages/svelte/src/compiler/phases/3-transform/client/visitors/FunctionDeclaration.js

@@ -1,28 +1,20 @@
 /** @import { FunctionDeclaration } from 'estree' */
 /** @import { ComponentContext } from '../types' */
-import { build_hoistable_params } from '../utils.js';
+import { build_hoisted_params } from '../utils.js';
 import * as b from '../../../../utils/builders.js';
 
 /**
  * @param {FunctionDeclaration} node
  * @param {ComponentContext} context
  */
 export function FunctionDeclaration(node, context) {
-	const metadata = node.metadata;
-
 	const state = { ...context.state, in_constructor: false };
 
-	if (metadata?.hoistable === true) {
-		const params = build_hoistable_params(node, context);
+	if (node.metadata?.hoisted === true) {
+		const params = build_hoisted_params(node, context);
+		const body = context.visit(node.body, state);
 
-		context.state.hoisted.push(
-			/** @type {FunctionDeclaration} */ ({
-				...node,
-				id: node.id !== null ? context.visit(node.id, state) : null,
-				params,
-				body: context.visit(node.body, state)
-			})
-		);
+		context.state.hoisted.push(/** @type {FunctionDeclaration} */ ({ ...node, params, body }));
 
 		return b.empty;
 	}

packages/svelte/src/compiler/phases/3-transform/client/visitors/OnDirective.js

@@ -1,11 +1,38 @@
-/** @import { OnDirective } from '#compiler' */
+/** @import { OnDirective, SvelteNode } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
-import { build_event } from './shared/element.js';
+import * as b from '../../../../utils/builders.js';
+import { build_event, build_event_handler } from './shared/events.js';
+
+const modifiers = [
+	'stopPropagation',
+	'stopImmediatePropagation',
+	'preventDefault',
+	'self',
+	'trusted',
+	'once'
+];
 
 /**
  * @param {OnDirective} node
  * @param {ComponentContext} context
  */
 export function OnDirective(node, context) {
-	build_event(node, node.metadata.expression, context);
+	if (!node.expression) {
+		context.state.analysis.needs_props = true;
+	}
+
+	let handler = build_event_handler(node.expression, node.metadata.expression, context);
+
+	for (const modifier of modifiers) {
+		if (node.modifiers.includes(modifier)) {
+			handler = b.call('$.' + modifier, handler);
+		}
+	}
+
+	const capture = node.modifiers.includes('capture');
+	const passive =
+		node.modifiers.includes('passive') ||
+		(node.modifiers.includes('nonpassive') ? false : undefined);
+
+	return build_event(node.name, context.state.node, handler, capture, passive);
 }

packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js

@@ -24,11 +24,11 @@ import {
 	get_attribute_name,
 	build_attribute_value,
 	build_class_directives,
-	build_event_attribute,
 	build_style_directives
 } from './shared/element.js';
 import { process_children } from './shared/fragment.js';
 import { build_render_statement, build_update, build_update_assignment } from './shared/utils.js';
+import { visit_event_attribute } from './shared/events.js';
 
 /**
  * @param {RegularElement} node
@@ -138,6 +138,13 @@ export function RegularElement(node, context) {
 			style_directives.push(attribute);
 		} else if (attribute.type === 'LetDirective') {
 			lets.push(/** @type {ExpressionStatement} */ (context.visit(attribute)));
+		} else if (attribute.type === 'OnDirective') {
+			const handler = /** @type {Expression} */ (context.visit(attribute));
+			const has_action_directive = node.attributes.find((a) => a.type === 'UseDirective');
+
+			context.state.after_update.push(
+				b.stmt(has_action_directive ? b.call('$.effect', b.thunk(handler)) : handler)
+			);
 		} else {
 			if (attribute.type === 'BindDirective') {
 				if (attribute.name === 'group' || attribute.name === 'checked') {
@@ -214,7 +221,7 @@ export function RegularElement(node, context) {
 				) {
 					might_need_event_replaying = true;
 				}
-				build_event_attribute(attribute, context);
+				visit_event_attribute(attribute, context);
 				continue;
 			}
 

packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteBody.js

@@ -1,14 +1,11 @@
 /** @import { SvelteBody } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
-import * as b from '../../../../utils/builders.js';
+import { visit_special_element } from './shared/special_element.js';
 
 /**
  * @param {SvelteBody} node
  * @param {ComponentContext} context
  */
 export function SvelteBody(node, context) {
-	context.next({
-		...context.state,
-		node: b.id('$.document.body')
-	});
+	visit_special_element(node, '$.document.body', context);
 }

packages/svelte/src/compiler/phases/3-transform/client/visitors/SvelteDocument.js

@@ -1,14 +1,11 @@
 /** @import { SvelteDocument } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
-import * as b from '../../../../utils/builders.js';
+import { visit_special_element } from './shared/special_element.js';
 
 /**
  * @param {SvelteDocument} node
  * @param {ComponentContext} context
  */
 export function SvelteDocument(node, context) {
-	context.next({
-		...context.state,
-		node: b.id('$.document')
-	});
+	visit_special_element(node, '$.document', context);
 }