Skip to content

fix: simplify event delegation logic #10169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jan 13, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rich-cobras-exist.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: simplify event delegation logic, only delegate event attributes
3 changes: 1 addition & 2 deletions packages/svelte/src/compiler/phases/1-parse/state/element.js
Original file line number Diff line number Diff line change
@@ -128,8 +128,7 @@ export default function tag(parser) {
fragment: create_fragment(true),
metadata: {
svg: false,
has_spread: false,
can_delegate_events: null
has_spread: false
},
parent: null
}
93 changes: 15 additions & 78 deletions packages/svelte/src/compiler/phases/2-analyze/index.js
Original file line number Diff line number Diff line change
@@ -59,35 +59,31 @@ 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
* @returns {null | import('#compiler').DelegatedEvent}
*/
function get_delegated_event(node, context) {
const handler = node.expression;
const event_name = node.name;

function get_delegated_event(event_name, handler, context) {
// 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;
}

// If we are not working with a RegularElement, then bail-out.
const element = context.path.at(-1);
if (element?.type !== 'RegularElement') {
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} */
const non_hoistable = { type: 'non-hoistable' };
/** @type {import('estree').FunctionExpression | import('estree').FunctionDeclaration | import('estree').ArrowFunctionExpression | null} */
let target_function = 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
return non_hoistable;
}
@@ -123,8 +119,7 @@ function get_delegated_event(node, context) {
if (element && event_name) {
if (
element.type !== 'RegularElement' ||
!determine_element_spread_and_delegatable(element).metadata.can_delegate_events ||
(element.metadata.has_spread && node.type === 'Attribute') ||
determine_element_spread(element).metadata.has_spread ||
!DelegatedEvents.includes(event_name)
) {
return non_hoistable;
@@ -183,7 +178,8 @@ function get_delegated_event(node, context) {
) {
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') {
return non_hoistable;
}
@@ -204,6 +200,7 @@ function get_delegated_event(node, context) {
}
visited_references.add(reference);
}

return { type: 'hoistable', function: target_function };
}

@@ -858,21 +855,9 @@ const common_visitors = {
});

if (is_event_attribute(node)) {
/** @type {string[]} */
const modifiers = [];
const expression = node.value[0].expression;

let name = node.name.slice(2);

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
);
const delegated_event = get_delegated_event(node.name.slice(2), expression, context);

if (delegated_event !== null) {
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,
FunctionExpression: function_visitor,
FunctionDeclaration: function_visitor,
@@ -1052,7 +1025,7 @@ const common_visitors = {
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
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
*/
function determine_element_spread_and_delegatable(node) {
if (typeof node.metadata.can_delegate_events === 'boolean') {
return node; // did this already
}

let events = new Map();
function determine_element_spread(node) {
let has_spread = false;
let has_on = false;
let has_action_or_bind = false;
for (const attribute of node.attributes) {
if (
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') {
if (!has_spread && attribute.type === 'SpreadAttribute') {
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;

return node;
Original file line number Diff line number Diff line change
@@ -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`
* @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
*/
function serialize_event(node, context) {
@@ -1318,9 +1318,9 @@ function serialize_event(node, context) {
if (node.expression) {
let handler = serialize_event_handler(node, context);
const event_name = node.name;
const delegated = node.metadata.delegated;
const delegated = node.delegated;

if (delegated !== null) {
if (delegated != null) {
let delegated_assignment;

if (!state.events.has(event_name)) {
@@ -1415,7 +1415,7 @@ function serialize_event_attribute(node, context) {
name: event_name,
expression: node.value[0].expression,
modifiers,
metadata: node.metadata
delegated: node.metadata.delegated
},
context
);
9 changes: 0 additions & 9 deletions packages/svelte/src/compiler/types/template.d.ts
Original file line number Diff line number Diff line change
@@ -202,9 +202,6 @@ export interface OnDirective extends BaseNode {
/** The 'y' in `on:x={y}` */
expression: null | Expression;
modifiers: string[]; // TODO specify
metadata: {
delegated: null | DelegatedEvent;
};
}

export type DelegatedEvent =
@@ -288,12 +285,6 @@ export interface RegularElement extends BaseElement {
svg: boolean;
/** `true` if contains a SpreadAttribute */
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;
};
}

Original file line number Diff line number Diff line change
@@ -31,9 +31,7 @@ export default test({
b2.click();
await Promise.resolve();

assert.ok(
log[0].stack.startsWith('Error:') && log[0].stack.includes('HTMLButtonElement.on_click')
);
assert.ok(log[0].stack.startsWith('Error:') && log[0].stack.includes('HTMLButtonElement.'));
assert.deepEqual(log[1], 1);
}
});
9 changes: 0 additions & 9 deletions packages/svelte/types/index.d.ts
Original file line number Diff line number Diff line change
@@ -1241,9 +1241,6 @@ declare module 'svelte/compiler' {
/** The 'y' in `on:x={y}` */
expression: null | Expression;
modifiers: string[]; // TODO specify
metadata: {
delegated: null | DelegatedEvent;
};
}

type DelegatedEvent =
@@ -1327,12 +1324,6 @@ declare module 'svelte/compiler' {
svg: boolean;
/** `true` if contains a SpreadAttribute */
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;
};
}