fix: improve attribute directive reactivity detection (#9907)

Author: trueadm

.changeset/happy-suits-film.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: improve attribute directive reactivity detection

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 =

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.

packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/_config.js

@@ -0,0 +1,16 @@
+import { test } from '../../test';
+
+export default test({
+	html: `<div style="color: red;"></div><div class="red"></div><button>toggle</button`,
+
+	async test({ assert, target }) {
+		const [b1] = target.querySelectorAll('button');
+
+		b1?.click();
+		await Promise.resolve();
+		assert.htmlEqual(
+			target.innerHTML,
+			'<div class="blue" style="color: blue;"></div><div class="blue"></div><button>toggle</button>'
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/main.svelte

@@ -0,0 +1,18 @@
+<script>
+	let value = $state('red');
+
+	const getValue = () => {
+		return value;
+	}
+	const getClass = () => {
+		return value === 'blue';
+	}
+	const getSpread = () => {
+		return { class: value };
+	}
+</script>
+
+<div class:blue={getClass()} style:color={getValue()}></div>
+<div {...getSpread()}></div>
+<button on:click={() => value = 'blue'}>toggle</button>
+