msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: simplify event delegation logic (#10169)

Browse Source 
* fix: simplify event delegation logic

Only delegate event attributes, and don't take into account bindings/actions while determining that. Never delegate `on:` directives. This simplifies the logic and makes it easier to explain, while avoiding any accidental breaking changes when upgrading from Svelte 4 to 5 without changing code.
Fixes #10165
Related to #9714

* update types
pull/10164/head
Simon H 12 months ago committed by GitHub
parent c628904861
commit b3ba25da94
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 26 additions and 105 deletions
Show Stats Download Patch File Download Diff File

5
.changeset/rich-cobras-exist.md
Unescape View File

@ -0,0 +1,5 @@
---
"svelte": patch
---
fix: simplify event delegation logic, only delegate event attributes

3
packages/svelte/src/compiler/phases/1-parse/state/element.js
Unescape View File

@ -128,8 +128,7 @@ export default function tag(parser) {
fragment: create_fragment(true), fragment: create_fragment(true),
metadata: { metadata: {
svg: false, svg: false,
has_spread: false, has_spread: false
can_delegate_events: null
}, },
parent: null parent: null
} }

93
packages/svelte/src/compiler/phases/2-analyze/index.js
Unescape View File

@ -59,27 +59,23 @@ function get_component_name(filename) {
} }
/** /**
* @param {Pick<import('#compiler').OnDirective, 'expression'| 'name' | 'modifiers'> & { type: string }} node * Checks if given event attribute can be delegated/hoisted and returns the corresponding info if so
* @param {string} event_name
* @param {import('estree').Expression | null} handler
* @param {import('./types').Context} context * @param {import('./types').Context} context
* @returns {null | import('#compiler').DelegatedEvent} * @returns {null | import('#compiler').DelegatedEvent}
*/ */
function get_delegated_event(node, context) { function get_delegated_event(event_name, handler, context) {
const handler = node.expression;
const event_name = node.name;
// Handle delegated event handlers. Bail-out if not a delegated event. // Handle delegated event handlers. Bail-out if not a delegated event.
if (!handler || node.modifiers.includes('capture') || !DelegatedEvents.includes(event_name)) { if (!handler || !DelegatedEvents.includes(event_name)) {
return null; 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); const element = context.path.at(-1);
if (element?.type !== 'RegularElement') { if (element?.type !== 'RegularElement') {
return null; return null;
} }
// If element says we can't delegate because we have multiple OnDirectives of the same type, bail-out.
if (!element.metadata.can_delegate_events) {
return null;
}
/** @type {import('#compiler').DelegatedEvent} */ /** @type {import('#compiler').DelegatedEvent} */
const non_hoistable = { type: 'non-hoistable' }; const non_hoistable = { type: 'non-hoistable' };
@ -87,7 +83,7 @@ function get_delegated_event(node, context) {
let target_function = null; let target_function = null;
let binding = null; let binding = null;
if (node.type === 'Attribute' && element.metadata.has_spread) { if (element.metadata.has_spread) {
// event attribute becomes part of the dynamic spread array // event attribute becomes part of the dynamic spread array
return non_hoistable; return non_hoistable;
} }
@ -123,8 +119,7 @@ function get_delegated_event(node, context) {
if (element && event_name) { if (element && event_name) {
if ( if (
element.type !== 'RegularElement' || element.type !== 'RegularElement' ||
!determine_element_spread_and_delegatable(element).metadata.can_delegate_events || determine_element_spread(element).metadata.has_spread ||
(element.metadata.has_spread && node.type === 'Attribute') ||
!DelegatedEvents.includes(event_name) !DelegatedEvents.includes(event_name)
) { ) {
return non_hoistable; return non_hoistable;
@ -183,7 +178,8 @@ function get_delegated_event(node, context) {
) { ) {
return non_hoistable; return non_hoistable;
} }
// If we referebnce the index within an each block, then bail-out.
// If we reference the index within an each block, then bail-out.
if (binding !== null && binding.initial?.type === 'EachBlock') { if (binding !== null && binding.initial?.type === 'EachBlock') {
return non_hoistable; return non_hoistable;
} }
@ -204,6 +200,7 @@ function get_delegated_event(node, context) {
} }
visited_references.add(reference); visited_references.add(reference);
} }
return { type: 'hoistable', function: target_function }; return { type: 'hoistable', function: target_function };
} }
@ -858,21 +855,9 @@ const common_visitors = {
}); });
if (is_event_attribute(node)) { if (is_event_attribute(node)) {
/** @type {string[]} */
const modifiers = [];
const expression = node.value[0].expression; const expression = node.value[0].expression;
let name = node.name.slice(2); const delegated_event = get_delegated_event(node.name.slice(2), expression, context);
if (is_capture_event(name)) {
name = name.slice(0, -7);
modifiers.push('capture');
}
const delegated_event = get_delegated_event(
{ type: node.type, name, expression, modifiers },
context
);
if (delegated_event !== null) { if (delegated_event !== null) {
if (delegated_event.type === 'hoistable') { if (delegated_event.type === 'hoistable') {
@ -1032,18 +1017,6 @@ const common_visitors = {
) )
}; };
}, },
OnDirective(node, context) {
node.metadata = { delegated: null };
context.next();
const delegated_event = get_delegated_event(node, context);
if (delegated_event !== null) {
if (delegated_event.type === 'hoistable') {
delegated_event.function.metadata.hoistable = true;
}
node.metadata.delegated = delegated_event;
}
},
ArrowFunctionExpression: function_visitor, ArrowFunctionExpression: function_visitor,
FunctionExpression: function_visitor, FunctionExpression: function_visitor,
FunctionDeclaration: function_visitor, FunctionDeclaration: function_visitor,
@ -1052,7 +1025,7 @@ const common_visitors = {
node.metadata.svg = true; node.metadata.svg = true;
} }
determine_element_spread_and_delegatable(node); determine_element_spread(node);
// Special case: Move the children of <textarea> into a value attribute if they are dynamic // Special case: Move the children of <textarea> into a value attribute if they are dynamic
if ( if (
@ -1110,51 +1083,15 @@ const common_visitors = {
}; };
/** /**
* Check if events on this element can theoretically be delegated. They can if there's no
* possibility of an OnDirective and an event attribute on the same element, and if there's
* no OnDirectives of the same type (the latter is a bit too strict because `on:click on:click on:keyup`
* means that `on:keyup` can be delegated but we gloss over this edge case).
* @param {import('#compiler').RegularElement} node * @param {import('#compiler').RegularElement} node
*/ */
function determine_element_spread_and_delegatable(node) { function determine_element_spread(node) {
if (typeof node.metadata.can_delegate_events === 'boolean') {
return node; // did this already
}
let events = new Map();
let has_spread = false; let has_spread = false;
let has_on = false;
let has_action_or_bind = false;
for (const attribute of node.attributes) { for (const attribute of node.attributes) {
if ( if (!has_spread && attribute.type === 'SpreadAttribute') {
attribute.type === 'OnDirective' ||
(attribute.type === 'Attribute' && is_event_attribute(attribute))
) {
let event_name = attribute.name;
if (attribute.type === 'Attribute') {
event_name = get_attribute_event_name(event_name);
}
events.set(event_name, (events.get(event_name) || 0) + 1);
if (!has_on && attribute.type === 'OnDirective') {
has_on = true;
}
} else if (!has_spread && attribute.type === 'SpreadAttribute') {
has_spread = true; has_spread = true;
} else if (
!has_action_or_bind &&
((attribute.type === 'BindDirective' && attribute.name !== 'this') ||
attribute.type === 'UseDirective')
) {
has_action_or_bind = true;
} }
} }
node.metadata.can_delegate_events =
// Actions/bindings need the old on:-events to fire in order
!has_action_or_bind &&
// spreading events means we don't know if there's an event attribute with the same name as an on:-event
!(has_spread && has_on) &&
// multiple on:-events/event attributes with the same name
![...events.values()].some((count) => count > 1);
node.metadata.has_spread = has_spread; node.metadata.has_spread = has_spread;
return node; return node;

8
packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js
Unescape View File

@ -1309,7 +1309,7 @@ function serialize_event_handler(node, { state, visit }) {
/** /**
* Serializes an event handler function of the `on:` directive or an attribute starting with `on` * Serializes an event handler function of the `on:` directive or an attribute starting with `on`
* @param {Pick<import('#compiler').OnDirective, 'name' | 'modifiers' | 'expression' | 'metadata'>} node * @param {{name: string; modifiers: string[]; expression: import('estree').Expression | null; delegated?: import('#compiler').DelegatedEvent | null; }} node
* @param {import('../types.js').ComponentContext} context * @param {import('../types.js').ComponentContext} context
*/ */
function serialize_event(node, context) { function serialize_event(node, context) {
@ -1318,9 +1318,9 @@ function serialize_event(node, context) {
if (node.expression) { if (node.expression) {
let handler = serialize_event_handler(node, context); let handler = serialize_event_handler(node, context);
const event_name = node.name; const event_name = node.name;
const delegated = node.metadata.delegated; const delegated = node.delegated;
if (delegated !== null) { if (delegated != null) {
let delegated_assignment; let delegated_assignment;
if (!state.events.has(event_name)) { if (!state.events.has(event_name)) {
@ -1415,7 +1415,7 @@ function serialize_event_attribute(node, context) {
name: event_name, name: event_name,
expression: node.value[0].expression, expression: node.value[0].expression,
modifiers, modifiers,
metadata: node.metadata delegated: node.metadata.delegated
}, },
context context
); );

9
packages/svelte/src/compiler/types/template.d.ts vendored
Unescape View File

@ -202,9 +202,6 @@ export interface OnDirective extends BaseNode {
/** The 'y' in `on:x={y}` */ /** The 'y' in `on:x={y}` */
expression: null | Expression; expression: null | Expression;
modifiers: string[]; // TODO specify modifiers: string[]; // TODO specify
metadata: {
delegated: null | DelegatedEvent;
};
} }
export type DelegatedEvent = export type DelegatedEvent =
@ -288,12 +285,6 @@ export interface RegularElement extends BaseElement {
svg: boolean; svg: boolean;
/** `true` if contains a SpreadAttribute */ /** `true` if contains a SpreadAttribute */
has_spread: boolean; has_spread: boolean;
/**
* `true` if events on this element can theoretically be delegated. This doesn't necessarily mean that
* a specific event will be delegated, as there are other factors which affect the final outcome.
* `null` only until it was determined whether this element can be delegated or not.
*/
can_delegate_events: boolean | null;
}; };
} }

4
packages/svelte/tests/runtime-runes/samples/inspect-trace/_config.js
Unescape View File

@ -31,9 +31,7 @@ export default test({
b2.click(); b2.click();
await Promise.resolve(); await Promise.resolve();
assert.ok( assert.ok(log[0].stack.startsWith('Error:') && log[0].stack.includes('HTMLButtonElement.'));
log[0].stack.startsWith('Error:') && log[0].stack.includes('HTMLButtonElement.on_click')
);
assert.deepEqual(log[1], 1); assert.deepEqual(log[1], 1);
} }
}); });

9
packages/svelte/types/index.d.ts vendored
Unescape View File

@ -1241,9 +1241,6 @@ declare module 'svelte/compiler' {
/** The 'y' in `on:x={y}` */ /** The 'y' in `on:x={y}` */
expression: null | Expression; expression: null | Expression;
modifiers: string[]; // TODO specify modifiers: string[]; // TODO specify
metadata: {
delegated: null | DelegatedEvent;
};
} }
type DelegatedEvent = type DelegatedEvent =
@ -1327,12 +1324,6 @@ declare module 'svelte/compiler' {
svg: boolean; svg: boolean;
/** `true` if contains a SpreadAttribute */ /** `true` if contains a SpreadAttribute */
has_spread: boolean; has_spread: boolean;
/**
* `true` if events on this element can theoretically be delegated. This doesn't necessarily mean that
* a specific event will be delegated, as there are other factors which affect the final outcome.
* `null` only until it was determined whether this element can be delegated or not.
*/
can_delegate_events: boolean | null;
}; };
} }

Write Preview
Loading…
Cancel
Save