fix: stable attachments (#15961)

* fix: stable attachments

* WIP

* WIP

* WIP

* WIP

* WIP

* WIP

* fix

* unused

* unused

* unused

* changeset

* ugh

* remove nonsensical comment, this is unused in spread

* rename

* Update packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

.changeset/soft-jobs-sip.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: avoid rerunning attachments when unrelated spread attributes change

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

@@ -17,7 +17,7 @@ import { build_getter } from '../utils.js';
 import {
 	get_attribute_name,
 	build_attribute_value,
-	build_set_attributes,
+	build_attribute_effect,
 	build_set_class,
 	build_set_style
 } from './shared/element.js';
@@ -201,37 +201,7 @@ export function RegularElement(node, context) {
 	const node_id = context.state.node;
 
 	if (has_spread) {
-		const attributes_id = b.id(context.state.scope.generate('attributes'));
-
-		build_set_attributes(
-			attributes,
-			class_directives,
-			style_directives,
-			context,
-			node,
-			node_id,
-			attributes_id
-		);
-
-		// If value binding exists, that one takes care of calling $.init_select
-		if (node.name === 'select' && !bindings.has('value')) {
-			context.state.init.push(
-				b.stmt(b.call('$.init_select', node_id, b.thunk(b.member(attributes_id, 'value'))))
-			);
-
-			context.state.update.push(
-				b.if(
-					b.binary('in', b.literal('value'), attributes_id),
-					b.block([
-						// This ensures a one-way street to the DOM in case it's <select {value}>
-						// and not <select bind:value>. We need it in addition to $.init_select
-						// because the select value is not reflected as an attribute, so the
-						// mutation observer wouldn't notice.
-						b.stmt(b.call('$.select_option', node_id, b.member(attributes_id, 'value')))
-					])
-				)
-			);
-		}
+		build_attribute_effect(attributes, class_directives, style_directives, context, node, node_id);
 	} else {
 		/** If true, needs `__value` for inputs */
 		const needs_special_value_handling =
@@ -290,7 +260,8 @@ export function RegularElement(node, context) {
 				const { value, has_state } = build_attribute_value(
 					attribute.value,
 					context,
-					(value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value)
+					(value, metadata) =>
+						metadata.has_call ? get_expression_id(context.state.expressions, value) : value
 				);
 
 				const update = build_element_attribute_update(node, node_id, name, value, attributes);
@@ -482,10 +453,11 @@ function setup_select_synchronization(value_binding, context) {
 
 /**
  * @param {AST.ClassDirective[]} class_directives
+ * @param {Expression[]} expressions
  * @param {ComponentContext} context
  * @return {ObjectExpression | Identifier}
  */
-export function build_class_directives_object(class_directives, context) {
+export function build_class_directives_object(class_directives, expressions, context) {
 	let properties = [];
 	let has_call_or_state = false;
 
@@ -497,15 +469,16 @@ export function build_class_directives_object(class_directives, context) {
 
 	const directives = b.object(properties);
 
-	return has_call_or_state ? get_expression_id(context.state, directives) : directives;
+	return has_call_or_state ? get_expression_id(expressions, directives) : directives;
 }
 
 /**
  * @param {AST.StyleDirective[]} style_directives
+ * @param {Expression[]} expressions
  * @param {ComponentContext} context
  * @return {ObjectExpression | ArrayExpression}}
  */
-export function build_style_directives_object(style_directives, context) {
+export function build_style_directives_object(style_directives, expressions, context) {
 	let normal_properties = [];
 	let important_properties = [];
 
@@ -514,7 +487,7 @@ export function build_style_directives_object(style_directives, context) {
 			directive.value === true
 				? build_getter({ name: directive.name, type: 'Identifier' }, context.state)
 				: build_attribute_value(directive.value, context, (value, metadata) =>
-						metadata.has_call ? get_expression_id(context.state, value) : value
+						metadata.has_call ? get_expression_id(expressions, value) : value
 					).value;
 		const property = b.init(directive.name, expression);
 
@@ -653,7 +626,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
 			? // if is a select with value we will also invoke `init_select` which need a reference before the template effect so we memoize separately
 				is_select_with_value
 				? memoize_expression(state, value)
-				: get_expression_id(state, value)
+				: get_expression_id(state.expressions, value)
 			: value
 	);
 

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

@@ -5,7 +5,11 @@ import { dev, locator } from '../../../../state.js';
 import { is_text_attribute } from '../../../../utils/ast.js';
 import * as b from '#compiler/builders';
 import { determine_namespace_for_children } from '../../utils.js';
-import { build_attribute_value, build_set_attributes, build_set_class } from './shared/element.js';
+import {
+	build_attribute_value,
+	build_attribute_effect,
+	build_set_class
+} from './shared/element.js';
 import { build_render_statement } from './shared/utils.js';
 
 /**
@@ -80,18 +84,15 @@ export function SvelteElement(node, context) {
 	) {
 		build_set_class(node, element_id, attributes[0], class_directives, inner_context, false);
 	} else if (attributes.length) {
-		const attributes_id = b.id(context.state.scope.generate('attributes'));
-
 		// 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.
-		build_set_attributes(
+		build_attribute_effect(
 			attributes,
 			class_directives,
 			style_directives,
 			inner_context,
 			node,
-			element_id,
-			attributes_id
+			element_id
 		);
 	}
 

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

@@ -16,28 +16,30 @@ import { build_template_chunk, get_expression_id } from './utils.js';
  * @param {ComponentContext} context
  * @param {AST.RegularElement | AST.SvelteElement} element
  * @param {Identifier} element_id
- * @param {Identifier} attributes_id
  */
-export function build_set_attributes(
+export function build_attribute_effect(
 	attributes,
 	class_directives,
 	style_directives,
 	context,
 	element,
-	element_id,
-	attributes_id
+	element_id
 ) {
-	let is_dynamic = false;
-
 	/** @type {ObjectExpression['properties']} */
 	const values = [];
 
+	/** @type {Expression[]} */
+	const expressions = [];
+
+	/** @param {Expression} value */
+	function memoize(value) {
+		return b.id(`$${expressions.push(value) - 1}`);
+	}
+
 	for (const attribute of attributes) {
 		if (attribute.type === 'Attribute') {
-			const { value, has_state } = build_attribute_value(
-				attribute.value,
-				context,
-				(value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value)
+			const { value } = build_attribute_value(attribute.value, context, (value, metadata) =>
+				metadata.has_call ? memoize(value) : value
 			);
 
 			if (
@@ -51,16 +53,11 @@ export function build_set_attributes(
 			} else {
 				values.push(b.init(attribute.name, value));
 			}
-
-			is_dynamic ||= has_state;
 		} else {
-			// objects could contain reactive getters -> play it safe and always assume spread attributes are reactive
-			is_dynamic = true;
-
 			let value = /** @type {Expression} */ (context.visit(attribute));
 
 			if (attribute.metadata.expression.has_call) {
-				value = get_expression_id(context.state, value);
+				value = memoize(value);
 			}
 
 			values.push(b.spread(value));
@@ -72,12 +69,9 @@ export function build_set_attributes(
 			b.prop(
 				'init',
 				b.array([b.id('$.CLASS')]),
-				build_class_directives_object(class_directives, context)
+				build_class_directives_object(class_directives, expressions, context)
 			)
 		);
-
-		is_dynamic ||=
-			class_directives.find((directive) => directive.metadata.expression.has_state) !== null;
 	}
 
 	if (style_directives.length) {
@@ -85,31 +79,28 @@ export function build_set_attributes(
 			b.prop(
 				'init',
 				b.array([b.id('$.STYLE')]),
-				build_style_directives_object(style_directives, context)
+				build_style_directives_object(style_directives, expressions, context)
 			)
 		);
-
-		is_dynamic ||= style_directives.some((directive) => directive.metadata.expression.has_state);
 	}
 
-	const call = b.call(
-		'$.set_attributes',
+	context.state.init.push(
+		b.stmt(
+			b.call(
+				'$.attribute_effect',
 				element_id,
-		is_dynamic ? attributes_id : b.null,
-		b.object(values),
+				b.arrow(
+					expressions.map((_, i) => b.id(`$${i}`)),
+					b.object(values)
+				),
+				expressions.length > 0 && b.array(expressions.map((expression) => b.thunk(expression))),
 				element.metadata.scoped &&
 					context.state.analysis.css.hash !== '' &&
 					b.literal(context.state.analysis.css.hash),
 				is_ignored(element, 'hydration_attribute_changed') && b.true
+			)
+		)
 	);
-
-	if (is_dynamic) {
-		context.state.init.push(b.let(attributes_id));
-		const update = b.stmt(b.assignment('=', attributes_id, call));
-		context.state.update.push(update);
-	} else {
-		context.state.init.push(b.stmt(call));
-	}
 }
 
 /**
@@ -167,7 +158,7 @@ export function build_set_class(element, node_id, attribute, class_directives, c
 			value = b.call('$.clsx', value);
 		}
 
-		return metadata.has_call ? get_expression_id(context.state, value) : value;
+		return metadata.has_call ? get_expression_id(context.state.expressions, value) : value;
 	});
 
 	/** @type {Identifier | undefined} */
@@ -180,7 +171,7 @@ export function build_set_class(element, node_id, attribute, class_directives, c
 	let next;
 
 	if (class_directives.length) {
-		next = build_class_directives_object(class_directives, context);
+		next = build_class_directives_object(class_directives, context.state.expressions, context);
 		has_state ||= class_directives.some((d) => d.metadata.expression.has_state);
 
 		if (has_state) {
@@ -235,7 +226,7 @@ export function build_set_class(element, node_id, attribute, class_directives, c
  */
 export function build_set_style(node_id, attribute, style_directives, context) {
 	let { value, has_state } = build_attribute_value(attribute.value, context, (value, metadata) =>
-		metadata.has_call ? get_expression_id(context.state, value) : value
+		metadata.has_call ? get_expression_id(context.state.expressions, value) : value
 	);
 
 	/** @type {Identifier | undefined} */
@@ -248,7 +239,7 @@ export function build_set_style(node_id, attribute, style_directives, context) {
 	let next;
 
 	if (style_directives.length) {
-		next = build_style_directives_object(style_directives, context);
+		next = build_style_directives_object(style_directives, context.state.expressions, context);
 		has_state ||= style_directives.some((d) => d.metadata.expression.has_state);
 
 		if (has_state) {

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

@@ -21,12 +21,12 @@ export function memoize_expression(state, value) {
 }
 
 /**
- *
- * @param {ComponentClientTransformState} state
+ * Pushes `value` into `expressions` and returns a new id
+ * @param {Expression[]} expressions
  * @param {Expression} value
  */
-export function get_expression_id(state, value) {
-	return b.id(`$${state.expressions.push(value) - 1}`);
+export function get_expression_id(expressions, value) {
+	return b.id(`$${expressions.push(value) - 1}`);
 }
 
 /**
@@ -40,7 +40,8 @@ export function build_template_chunk(
 	values,
 	visit,
 	state,
-	memoize = (value, metadata) => (metadata.has_call ? get_expression_id(state, value) : value)
+	memoize = (value, metadata) =>
+		metadata.has_call ? get_expression_id(state.expressions, value) : value
 ) {
 	/** @type {Expression[]} */
 	const expressions = [];

packages/svelte/src/internal/client/dom/elements/attributes.js

@@ -1,3 +1,4 @@
+/** @import { Effect } from '#client' */
 import { DEV } from 'esm-env';
 import { hydrating, set_hydrating } from '../hydration.js';
 import { get_descriptors, get_prototype_of } from '../../../shared/utils.js';
@@ -10,6 +11,7 @@ import { is_capture_event, is_delegated, normalize_attribute } from '../../../..
 import {
 	active_effect,
 	active_reaction,
+	get,
 	set_active_effect,
 	set_active_reaction
 } from '../../runtime.js';
@@ -18,6 +20,9 @@ import { clsx } from '../../../shared/attributes.js';
 import { set_class } from './class.js';
 import { set_style } from './style.js';
 import { ATTACHMENT_KEY, NAMESPACE_HTML } from '../../../../constants.js';
+import { block, branch, destroy_effect } from '../../reactivity/effects.js';
+import { derived } from '../../reactivity/deriveds.js';
+import { init_select, select_option } from './bindings/select.js';
 
 export const CLASS = Symbol('class');
 export const STYLE = Symbol('style');
@@ -447,13 +452,68 @@ export function set_attributes(element, prev, next, css_hash, skip_warning = fal
 		set_hydrating(true);
 	}
 
+	return current;
+}
+
+/**
+ * @param {Element & ElementCSSInlineStyle} element
+ * @param {(...expressions: any) => Record<string | symbol, any>} fn
+ * @param {Array<() => any>} thunks
+ * @param {string} [css_hash]
+ * @param {boolean} [skip_warning]
+ */
+export function attribute_effect(
+	element,
+	fn,
+	thunks = [],
+	css_hash,
+	skip_warning = false,
+	d = derived
+) {
+	const deriveds = thunks.map(d);
+
+	/** @type {Record<string | symbol, any> | undefined} */
+	var prev = undefined;
+
+	/** @type {Record<symbol, Effect>} */
+	var effects = {};
+
+	var is_select = element.nodeName === 'SELECT';
+	var inited = false;
+
+	block(() => {
+		var next = fn(...deriveds.map(get));
+
+		set_attributes(element, prev, next, css_hash, skip_warning);
+
+		if (inited && is_select) {
+			select_option(/** @type {HTMLSelectElement} */ (element), next.value, false);
+		}
+
+		for (let symbol of Object.getOwnPropertySymbols(effects)) {
+			if (!next[symbol]) destroy_effect(effects[symbol]);
+		}
+
 		for (let symbol of Object.getOwnPropertySymbols(next)) {
-		if (symbol.description === ATTACHMENT_KEY) {
-			attach(element, () => next[symbol]);
+			var n = next[symbol];
+
+			if (symbol.description === ATTACHMENT_KEY && (!prev || n !== prev[symbol])) {
+				if (effects[symbol]) destroy_effect(effects[symbol]);
+				effects[symbol] = branch(() => attach(element, () => n));
 			}
 		}
 
-	return current;
+		prev = next;
+	});
+
+	if (is_select) {
+		init_select(
+			/** @type {HTMLSelectElement} */ (element),
+			() => /** @type {Record<string | symbol, any>} */ (prev).value
+		);
+	}
+
+	inited = true;
 }
 
 /**

packages/svelte/src/internal/client/dom/elements/bindings/select.js

@@ -25,10 +25,14 @@ export function select_option(select, value, mounting) {
 		}
 
 		// Otherwise, update the selection
-		return select_options(select, value);
+		for (var option of select.options) {
+			option.selected = value.includes(get_option_value(option));
 		}
 
-	for (var option of select.options) {
+		return;
+	}
+
+	for (option of select.options) {
 		var option_value = get_option_value(option);
 		if (is(option_value, value)) {
 			option.selected = true;
@@ -136,16 +140,6 @@ export function bind_select_value(select, get, set = get) {
 	init_select(select);
 }
 
-/**
- * @param {HTMLSelectElement} select
- * @param {unknown[]} value
- */
-function select_options(select, value) {
-	for (var option of select.options) {
-		option.selected = value.includes(get_option_value(option));
-	}
-}
-
 /** @param {HTMLOptionElement} option */
 function get_option_value(option) {
 	// __value only exists if the <option> has a value attribute

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

@@ -28,6 +28,7 @@ export {
 	remove_input_defaults,
 	set_attribute,
 	set_attributes,
+	attribute_effect,
 	set_custom_element_data,
 	set_xlink_attribute,
 	set_value,

packages/svelte/tests/runtime-runes/samples/attachment-spread-stable/Component.svelte

@@ -0,0 +1,5 @@
+<script>
+	let props = $props();
+</script>
+
+<p {...props}>hello</p>

packages/svelte/tests/runtime-runes/samples/attachment-spread-stable/_config.js

@@ -0,0 +1,16 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	test({ assert, target, logs }) {
+		assert.deepEqual(logs, ['up']);
+
+		const button = target.querySelector('button');
+
+		flushSync(() => button?.click());
+		assert.deepEqual(logs, ['up']);
+
+		flushSync(() => button?.click());
+		assert.deepEqual(logs, ['up', 'down']);
+	}
+});

packages/svelte/tests/runtime-runes/samples/attachment-spread-stable/main.svelte

@@ -0,0 +1,17 @@
+<script>
+	import Component from './Component.svelte';
+
+	let count = $state(0);
+</script>
+
+<button onclick={() => count++}>{count}</button>
+
+{#if count < 2}
+	<Component
+		data-count={count}
+		{@attach () => {
+			console.log('up');
+			return () => console.log('down');
+		}}
+	/>
+{/if}