From b0511a59660c9ec4c717a5e823186c9b292bfb44 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 13 Dec 2023 14:15:26 +0000 Subject: [PATCH] fix: improve attribute directive reactivity detection (#9907) --- .changeset/happy-suits-film.md | 5 ++ .../3-transform/client/visitors/template.js | 90 +++++++++++-------- packages/svelte/src/internal/client/render.js | 38 +++++++- .../_config.js | 16 ++++ .../main.svelte | 18 ++++ 5 files changed, 128 insertions(+), 39 deletions(-) create mode 100644 .changeset/happy-suits-film.md create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/main.svelte diff --git a/.changeset/happy-suits-film.md b/.changeset/happy-suits-film.md new file mode 100644 index 0000000000..84fdcbdb82 --- /dev/null +++ b/.changeset/happy-suits-film.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: improve attribute directive reactivity detection diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js index 7c06cac973..6ad1026cc7 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js @@ -63,32 +63,48 @@ function get_attribute_name(element, attribute, context) { * @param {boolean} is_attributes_reactive */ function serialize_style_directives(style_directives, element_id, context, is_attributes_reactive) { - if (style_directives.length > 0) { - const values = style_directives.map((directive) => { - let value = - directive.value === true - ? serialize_get_binding({ name: directive.name, type: 'Identifier' }, context.state) - : serialize_attribute_value(directive.value, context)[1]; - return b.stmt( - b.call( - '$.style', - element_id, - b.literal(directive.name), - value, - /** @type {import('estree').Expression} */ ( - directive.modifiers.includes('important') ? b.true : undefined - ) + const state = context.state; + + for (const directive of style_directives) { + let value = + directive.value === true + ? serialize_get_binding({ name: directive.name, type: 'Identifier' }, context.state) + : serialize_attribute_value(directive.value, context)[1]; + const grouped = b.stmt( + b.call( + '$.style', + element_id, + b.literal(directive.name), + value, + /** @type {import('estree').Expression} */ ( + directive.modifiers.includes('important') ? b.true : undefined + ) + ) + ); + const singular = b.stmt( + b.call( + '$.style_effect', + element_id, + b.literal(directive.name), + b.arrow([], value), + /** @type {import('estree').Expression} */ ( + directive.modifiers.includes('important') ? b.true : undefined ) + ) + ); + + const contains_call_expression = + Array.isArray(directive.value) && + directive.value.some( + (v) => v.type === 'ExpressionTag' && v.metadata.contains_call_expression ); - }); - if ( - is_attributes_reactive || - style_directives.some((directive) => directive.metadata.dynamic) - ) { - context.state.update.push(...values.map((v) => ({ grouped: v }))); + if (!is_attributes_reactive && contains_call_expression) { + state.update_effects.push(singular); + } else if (is_attributes_reactive || directive.metadata.dynamic || contains_call_expression) { + state.update.push({ grouped, singular }); } else { - context.state.init.push(...values); + state.init.push(grouped); } } } @@ -123,21 +139,21 @@ function parse_directive_name(name) { * @param {boolean} is_attributes_reactive */ function serialize_class_directives(class_directives, element_id, context, is_attributes_reactive) { - if (class_directives.length > 0) { - const values = class_directives.map((directive) => { - const value = /** @type {import('estree').Expression} */ ( - context.visit(directive.expression) - ); - return b.stmt(b.call('$.class_toggle', element_id, b.literal(directive.name), value)); - }); + const state = context.state; + for (const directive of class_directives) { + const value = /** @type {import('estree').Expression} */ (context.visit(directive.expression)); + const grouped = b.stmt(b.call('$.class_toggle', element_id, b.literal(directive.name), value)); + const singular = b.stmt( + b.call('$.class_toggle_effect', element_id, b.literal(directive.name), b.arrow([], value)) + ); + const contains_call_expression = directive.expression.type === 'CallExpression'; - if ( - is_attributes_reactive || - class_directives.some((directive) => directive.metadata.dynamic) - ) { - context.state.update.push(...values.map((v) => ({ grouped: v }))); + if (!is_attributes_reactive && contains_call_expression) { + state.update_effects.push(singular); + } else if (is_attributes_reactive || directive.metadata.dynamic || contains_call_expression) { + state.update.push({ grouped, singular }); } else { - context.state.init.push(...values); + state.init.push(grouped); } } } @@ -295,7 +311,9 @@ function serialize_element_spread_attributes(attributes, context, element, eleme values.push(/** @type {import('estree').Expression} */ (context.visit(attribute))); } - is_reactive ||= attribute.metadata.dynamic; + is_reactive ||= + attribute.metadata.dynamic || + (attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression); } const lowercase_attributes = diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index 93c823ce33..a963c832c3 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -444,6 +444,20 @@ export function class_toggle(dom, class_name, value) { dom.classList.remove(class_name); } } + +/** + * @param {Element} dom + * @param {string} class_name + * @param {() => boolean} value + * @returns {void} + */ +export function class_toggle_effect(dom, class_name, value) { + render_effect(() => { + const string = value(); + class_toggle(dom, class_name, string); + }); +} + /** * Selects the correct option(s) (depending on whether this is a multiple select) * @template V @@ -2359,13 +2373,31 @@ export function set_custom_element_data(node, prop, value) { * @param {boolean} [important] */ export function style(dom, key, value, important) { + const style = dom.style; + const prev_value = style.getPropertyValue(key); if (value == null) { - dom.style.removeProperty(key); - } else { - dom.style.setProperty(key, value, important ? 'important' : ''); + if (prev_value !== '') { + style.removeProperty(key); + } + } else if (prev_value !== value) { + style.setProperty(key, value, important ? 'important' : ''); } } +/** + * @param {HTMLElement} dom + * @param {string} key + * @param {() => string} value + * @param {boolean} [important] + * @returns {void} + */ +export function style_effect(dom, key, value, important) { + render_effect(() => { + const string = value(); + style(dom, key, string, important); + }); +} + /** * List of attributes that should always be set through the attr method, * because updating them through the property setter doesn't work reliably. diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/_config.js b/packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/_config.js new file mode 100644 index 0000000000..e9dae16277 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/_config.js @@ -0,0 +1,16 @@ +import { test } from '../../test'; + +export default test({ + html: `
' + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/main.svelte b/packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/main.svelte new file mode 100644 index 0000000000..00c337bdfd --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/main.svelte @@ -0,0 +1,18 @@ + + +
+
+ +