fix: spread attributes reactivity improvements (#10071)

- the objects could contain getters with reactive values, so we play it safe and assume they're always reactive - fixes #10065
- isolate spreads with call expression similar to how we do it with other effects -fixes #10013

.changeset/sharp-kids-happen.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: always treat spread attributes as reactive and separate them if needed

packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js

@@ -289,7 +289,7 @@ function setup_select_synchronization(value_binding, context) {
  * 	value = $.spread_attributes(element, value, [...])
  * });
  * ```
- * Returns the id of the spread_attribute variable if spread is deemed reactive, `null` otherwise.
+ * Returns the id of the spread_attribute variable if spread isn't isolated, `null` otherwise.
  * @param {Array<import('#compiler').Attribute | import('#compiler').SpreadAttribute>} attributes
  * @param {import('../types.js').ComponentContext} context
  * @param {import('#compiler').RegularElement} element
@@ -297,7 +297,7 @@ function setup_select_synchronization(value_binding, context) {
  * @returns {string | null}
  */
 function serialize_element_spread_attributes(attributes, context, element, element_id) {
-	let is_reactive = false;
+	let needs_isolation = false;
 
 	/** @type {import('estree').Expression[]} */
 	const values = [];
@@ -312,18 +312,32 @@ function serialize_element_spread_attributes(attributes, context, element, eleme
 			values.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
 		}
 
-		is_reactive ||=
+		needs_isolation ||=
-			attribute.metadata.dynamic ||
+			attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression;
-			(attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression);
 	}
 
 	const lowercase_attributes =
 		element.metadata.svg || is_custom_element_node(element) ? b.false : b.true;
 
-	if (is_reactive) {
+	const isolated = b.stmt(
+		b.call(
+			'$.spread_attributes_effect',
+			element_id,
+			b.thunk(b.array(values)),
+			lowercase_attributes,
+			b.literal(context.state.analysis.stylesheet.id)
+		)
+	);
+
+	// objects could contain reactive getters -> play it safe and always assume spread attributes are reactive
+	if (needs_isolation) {
+		context.state.update_effects.push(isolated);
+		return null;
+	} else {
 		const id = context.state.scope.generate('spread_attributes');
-		context.state.init.push(b.let(id, undefined));
+		context.state.init.push(b.let(id));
 		context.state.update.push({
+			singular: isolated,
 			grouped: b.stmt(
 				b.assignment(
 					'=',
@@ -340,20 +354,6 @@ function serialize_element_spread_attributes(attributes, context, element, eleme
 			)
 		});
 		return id;
-	} else {
-		context.state.init.push(
-			b.stmt(
-				b.call(
-					'$.spread_attributes',
-					element_id,
-					b.literal(null),
-					b.array(values),
-					lowercase_attributes,
-					b.literal(context.state.analysis.stylesheet.id)
-				)
-			)
-		);
-		return null;
 	}
 }
 
@@ -365,7 +365,7 @@ function serialize_element_spread_attributes(attributes, context, element, eleme
  * @param {import('estree').Identifier} element_id
  * @returns {boolean}
  */
-function serialize_dynamic_element_spread_attributes(attributes, context, element_id) {
+function serialize_dynamic_element_attributes(attributes, context, element_id) {
 	if (attributes.length === 0) {
 		if (context.state.analysis.stylesheet.id) {
 			context.state.init.push(
@@ -375,6 +375,7 @@ function serialize_dynamic_element_spread_attributes(attributes, context, elemen
 		return false;
 	}
 
+	let needs_isolation = false;
 	let is_reactive = false;
 
 	/** @type {import('estree').Expression[]} */
@@ -388,13 +389,31 @@ function serialize_dynamic_element_spread_attributes(attributes, context, elemen
 			values.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
 		}
 
-		is_reactive ||= attribute.metadata.dynamic;
+		is_reactive ||=
+			attribute.metadata.dynamic ||
+			// objects could contain reactive getters -> play it safe and always assume spread attributes are reactive
+			attribute.type === 'SpreadAttribute';
+		needs_isolation ||=
+			attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression;
 	}
 
-	if (is_reactive) {
+	const isolated = b.stmt(
+		b.call(
+			'$.spread_dynamic_element_attributes_effect',
+			element_id,
+			b.thunk(b.array(values)),
+			b.literal(context.state.analysis.stylesheet.id)
+		)
+	);
+
+	if (needs_isolation) {
+		context.state.update_effects.push(isolated);
+		return false;
+	} else if (is_reactive) {
 		const id = context.state.scope.generate('spread_attributes');
 		context.state.init.push(b.let(id));
 		context.state.update.push({
+			singular: isolated,
 			grouped: b.stmt(
 				b.assignment(
 					'=',
@@ -2101,7 +2120,7 @@ export const template_visitors = {
 		// Always use spread because we don't know whether the element is a custom element or not,
 		// therefore we need to do the "how to set an attribute" logic at runtime.
 		const is_attributes_reactive =
-			serialize_dynamic_element_spread_attributes(attributes, inner_context, element_id) !== null;
+			serialize_dynamic_element_attributes(attributes, inner_context, element_id) !== null;
 
 		// class/style directives must be applied last since they could override class/style attributes
 		serialize_class_directives(class_directives, element_id, inner_context, is_attributes_reactive);

packages/svelte/src/internal/client/render.js

@@ -2428,10 +2428,26 @@ function get_setters(element) {
 	return setters;
 }
 
+/**
+ * Like `spread_attributes` but self-contained
+ * @param {Element & ElementCSSInlineStyle} dom
+ * @param {() => Record<string, unknown>[]} attrs
+ * @param {boolean} lowercase_attributes
+ * @param {string} css_hash
+ */
+export function spread_attributes_effect(dom, attrs, lowercase_attributes, css_hash) {
+	/** @type {Record<string, any> | undefined} */
+	let current = undefined;
+
+	render_effect(() => {
+		current = spread_attributes(dom, current, attrs(), lowercase_attributes, css_hash);
+	});
+}
+
 /**
  * Spreads attributes onto a DOM element, taking into account the currently set attributes
  * @param {Element & ElementCSSInlineStyle} dom
- * @param {Record<string, unknown> | null} prev
+ * @param {Record<string, unknown> | undefined} prev
  * @param {Record<string, unknown>[]} attrs
  * @param {boolean} lowercase_attributes
  * @param {string} css_hash
@@ -2528,18 +2544,30 @@ export function spread_attributes(dom, prev, attrs, lowercase_attributes, css_ha
 
 /**
  * @param {Element} node
- * @param {Record<string, unknown> | null} prev
+ * @param {() => Record<string, unknown>[]} attrs
+ * @param {string} css_hash
+ */
+export function spread_dynamic_element_attributes_effect(node, attrs, css_hash) {
+	/** @type {Record<string, any> | undefined} */
+	let current = undefined;
+
+	render_effect(() => {
+		current = spread_dynamic_element_attributes(node, current, attrs(), css_hash);
+	});
+}
+
+/**
+ * @param {Element} node
+ * @param {Record<string, unknown> | undefined} prev
  * @param {Record<string, unknown>[]} attrs
  * @param {string} css_hash
  */
 export function spread_dynamic_element_attributes(node, prev, attrs, css_hash) {
 	if (node.tagName.includes('-')) {
 		const next = object_assign({}, ...attrs);
-		if (prev !== null) {
+		for (const key in prev) {
-			for (const key in prev) {
+			if (!(key in next)) {
-				if (!(key in next)) {
+				next[key] = null;
-					next[key] = null;
-				}
 			}
 		}
 		for (const key in next) {

packages/svelte/tests/runtime-runes/samples/attribute-spread-call-expression/_config.js

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

packages/svelte/tests/runtime-runes/samples/attribute-spread-call-expression/main.svelte

@@ -0,0 +1,24 @@
+<script>
+	let tag = $state('button');
+	let values = $state({ a: 'red', b: 'red', c: 'red', d: 'red' });
+
+	let count = 0;
+	const factory = (name) => {
+		count++;
+		// check that spread effects are isolated from each other
+		if (count > 8) throw new Error('too many calls');
+
+		return {
+			class: values[name],
+			onclick: () => {
+				values[name] = 'blue';
+			}
+		}
+	}
+</script>
+
+<button {...factory('a')}>{values.a}</button>
+<button {...factory('b')}>{values.b}</button>
+
+<svelte:element this={tag} {...factory('c')}>{values.c}</svelte:element>
+<svelte:element this={tag} {...factory('d')}>{values.d}</svelte:element>

packages/svelte/tests/runtime-runes/samples/attribute-spread-reactivitiy/_config.js

@@ -0,0 +1,24 @@
+import { test } from '../../test';
+
+export default test({
+	html: `
+		<div style="color: red;"></div><div class="red"></div><div class="red"></div>
+		<div style="color: red;"></div><div class="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><div class="blue"></div>
+				<div class="blue" style="color: blue;"></div><div class="blue"></div><div class="blue"></div>
+				<button>toggle</button
+		`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/attribute-spread-reactivitiy/main.svelte

@@ -1,5 +1,6 @@
 <script>
 	let value = $state('red');
+	let tag = $state('div');
 
 	const getValue = () => {
 		return value;
@@ -10,9 +11,19 @@
 	const getSpread = () => {
 		return { class: value };
 	}
+	const props = {
+		get class() {
+			return value;
+		}
+	}
 </script>
 
 <div class:blue={getClass()} style:color={getValue()}></div>
 <div {...getSpread()}></div>
-<button on:click={() => value = 'blue'}>toggle</button>
+<div {...props}></div>
 
+<svelte:element this={tag} class:blue={getClass()} style:color={getValue()}></svelte:element>
+<svelte:element this={tag} {...getSpread()}></svelte:element>
+<svelte:element this={tag} {...props}></svelte:element>
+
+<button on:click={() => value = 'blue'}>toggle</button>

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

@@ -1,16 +0,0 @@
-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>'
-		);
-	}
-});