From 73ae5ef5bcec0ef416537267910877617fb77659 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 14 Nov 2023 21:27:27 +0000 Subject: [PATCH] fix: tighten up event attributes and hoisting logic (#9433) - add event delegation to spread_attributes - add event attributes to spread - don't delegate when bindings/actions on the same element in order to preserve backwards compatibility of ordering - don't hoist identifiers when one of them is used in an event that is not delegateable --------- Co-authored-by: Simon Holthausen --- .changeset/strong-lemons-provide.md | 5 + .../compiler/phases/1-parse/state/element.js | 4 +- .../src/compiler/phases/2-analyze/index.js | 136 ++++++++++++++---- .../phases/3-transform/client/types.d.ts | 2 +- .../3-transform/client/visitors/template.js | 39 +++-- .../svelte/src/compiler/phases/constants.js | 26 ---- .../svelte/src/compiler/types/template.d.ts | 8 ++ packages/svelte/src/constants.js | 29 ++++ packages/svelte/src/internal/client/render.js | 19 ++- .../event-attribute-not-hoistable/_config.js | 18 +++ .../event-attribute-not-hoistable/main.svelte | 11 ++ .../_config.js | 67 +++++++++ .../main.svelte | 21 +++ 13 files changed, 307 insertions(+), 78 deletions(-) create mode 100644 .changeset/strong-lemons-provide.md create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-not-hoistable/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-not-hoistable/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-spread-collision/main.svelte diff --git a/.changeset/strong-lemons-provide.md b/.changeset/strong-lemons-provide.md new file mode 100644 index 0000000000..1c8dc17a24 --- /dev/null +++ b/.changeset/strong-lemons-provide.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: handle event attribute spreading with event delegation diff --git a/packages/svelte/src/compiler/phases/1-parse/state/element.js b/packages/svelte/src/compiler/phases/1-parse/state/element.js index 8e69a92c6b..51eac9ca53 100644 --- a/packages/svelte/src/compiler/phases/1-parse/state/element.js +++ b/packages/svelte/src/compiler/phases/1-parse/state/element.js @@ -127,7 +127,9 @@ export default function tag(parser) { attributes: [], fragment: create_fragment(true), metadata: { - svg: false + svg: false, + has_spread: false, + can_delegate_events: null }, parent: null } diff --git a/packages/svelte/src/compiler/phases/2-analyze/index.js b/packages/svelte/src/compiler/phases/2-analyze/index.js index 99baf37307..cfb6194c77 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/index.js +++ b/packages/svelte/src/compiler/phases/2-analyze/index.js @@ -11,7 +11,7 @@ import { object } from '../../utils/ast.js'; import * as b from '../../utils/builders.js'; -import { DelegatedEvents, ReservedKeywords, Runes, SVGElements } from '../constants.js'; +import { ReservedKeywords, Runes, SVGElements } from '../constants.js'; import { Scope, ScopeRoot, create_scopes, get_rune, set_scope } from '../scope.js'; import { merge } from '../visitors.js'; import Stylesheet from './css/Stylesheet.js'; @@ -20,6 +20,7 @@ import { warn } from '../../warnings.js'; import check_graph_for_cycles from './utils/check_graph_for_cycles.js'; import { regex_starts_with_newline } from '../patterns.js'; import { create_attribute, is_element_node } from '../nodes.js'; +import { DelegatedEvents } from '../../../constants.js'; /** * @param {import('#compiler').Script | null} script @@ -58,7 +59,7 @@ function get_component_name(filename) { } /** - * @param {Pick} node + * @param {Pick & { type: string }} node * @param {import('./types').Context} context * @returns {null | import('#compiler').DelegatedEvent} */ @@ -70,16 +71,13 @@ function get_delegated_event(node, context) { if (!handler || node.modifiers.includes('capture') || !DelegatedEvents.includes(event_name)) { return null; } - // If we are not working with a RegularElement/SlotElement, then bail-out. + // If we are not working with a RegularElement, then bail-out. const element = context.path.at(-1); - if (element == null || (element.type !== 'RegularElement' && element.type !== 'SlotElement')) { + if (element?.type !== 'RegularElement') { return null; } - // If we have multiple OnDirectives of the same type, bail-out. - if ( - element.attributes.filter((attr) => attr.type === 'OnDirective' && attr.name === event_name) - .length > 1 - ) { + // 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; } @@ -89,6 +87,11 @@ function get_delegated_event(node, context) { let target_function = null; let binding = null; + if (node.type === 'Attribute' && element.metadata.has_spread) { + // event attribute becomes part of the dynamic spread array + return non_hoistable; + } + if (handler.type === 'ArrowFunctionExpression' || handler.type === 'FunctionExpression') { target_function = handler; } else if (handler.type === 'Identifier') { @@ -101,16 +104,29 @@ function get_delegated_event(node, context) { return non_hoistable; } - const element = - parent.type === 'OnDirective' - ? path.at(-2) - : parent.type === 'ExpressionTag' && - is_event_attribute(/** @type {import('#compiler').Attribute} */ (path.at(-2))) - ? path.at(-3) - : null; + /** @type {import('#compiler').RegularElement | null} */ + let element = null; + /** @type {string | null} */ + let event_name = null; + if (parent.type === 'OnDirective') { + element = /** @type {import('#compiler').RegularElement} */ (path.at(-2)); + event_name = parent.name; + } else if ( + parent.type === 'ExpressionTag' && + is_event_attribute(/** @type {import('#compiler').Attribute} */ (path.at(-2))) + ) { + element = /** @type {import('#compiler').RegularElement} */ (path.at(-3)); + const attribute = /** @type {import('#compiler').Attribute} */ (path.at(-2)); + event_name = get_attribute_event_name(attribute.name); + } - if (element) { - if (element.type !== 'RegularElement' && element.type !== 'SlotElement') { + 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') || + !DelegatedEvents.includes(event_name) + ) { return non_hoistable; } } else if (parent.type !== 'FunctionDeclaration' && parent.type !== 'VariableDeclarator') { @@ -772,16 +788,15 @@ const common_visitors = { let name = node.name.slice(2); - if ( - name.endsWith('capture') && - name !== 'ongotpointercapture' && - name !== 'onlostpointercapture' - ) { + if (is_capture_event(name)) { name = name.slice(0, -7); modifiers.push('capture'); } - const delegated_event = get_delegated_event({ name, expression, modifiers }, context); + const delegated_event = get_delegated_event( + { type: node.type, name, expression, modifiers }, + context + ); if (delegated_event !== null) { if (delegated_event.type === 'hoistable') { @@ -950,6 +965,8 @@ const common_visitors = { node.metadata.svg = true; } + determine_element_spread_and_delegatable(node); + // Special case: Move the children of