fix: use local mutable sources for props in legacy mode in case they are indirectly invalidated

.changeset/orange-tips-pull.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: use local mutable sources for props in legacy mode in case they are indirectly invalidated

packages/svelte/src/compiler/phases/2-analyze/visitors/EachBlock.js

@@ -1,4 +1,4 @@
-/** @import { AST } from '#compiler' */
+/** @import { AST, Binding } from '#compiler' */
 /** @import { Context } from '../types' */
 /** @import { Scope } from '../../scope' */
 import * as e from '../../../errors.js';
@@ -38,5 +38,38 @@ export function EachBlock(node, context) {
 	if (node.key) context.visit(node.key);
 	if (node.fallback) context.visit(node.fallback);
 
+	// collect transitive dependencies...
+	for (const binding of node.metadata.expression.dependencies) {
+		collect_transitive_dependencies(binding, node.metadata.transitive_deps);
+	}
+
+	// ...and ensure they are marked as state, so they can be turned
+	// into mutable sources and invalidated
+	for (const binding of node.metadata.transitive_deps) {
+		if (
+			binding.kind === 'normal' &&
+			(binding.declaration_kind === 'const' ||
+				binding.declaration_kind === 'let' ||
+				binding.declaration_kind === 'var')
+		) {
+			binding.kind = 'state';
+		}
+	}
+
 	mark_subtree_dynamic(context.path);
 }
+
+/**
+ * @param {Binding} binding
+ * @param {Set<Binding>} bindings
+ * @returns {void}
+ */
+function collect_transitive_dependencies(binding, bindings) {
+	bindings.add(binding);
+
+	if (binding.kind === 'legacy_reactive') {
+		for (const dep of binding.legacy_dependencies) {
+			collect_transitive_dependencies(dep, bindings);
+		}
+	}
+}

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

@@ -1,4 +1,4 @@
-/** @import { BlockStatement, Expression, Identifier, Pattern, Statement } from 'estree' */
+/** @import { BlockStatement, Expression, Identifier, Pattern, SequenceExpression, Statement } from 'estree' */
 /** @import { AST, Binding } from '#compiler' */
 /** @import { ComponentContext } from '../types' */
 /** @import { Scope } from '../../../scope' */
@@ -106,16 +106,6 @@ export function EachBlock(node, context) {
 		}
 	}
 
-	// Legacy mode: find the parent each blocks which contain the arrays to invalidate
-	const indirect_dependencies = collect_parent_each_blocks(context).flatMap((block) => {
-		const array = /** @type {Expression} */ (context.visit(block.expression));
-		const transitive_dependencies = build_transitive_dependencies(
-			block.metadata.expression.dependencies,
-			context
-		);
-		return [array, ...transitive_dependencies];
-	});
-
 	/** @type {Identifier | null} */
 	let collection_id = null;
 
@@ -129,18 +119,6 @@ export function EachBlock(node, context) {
 		}
 	}
 
-	if (collection_id) {
-		indirect_dependencies.push(b.call(collection_id));
-	} else {
-		indirect_dependencies.push(collection);
-
-		const transitive_dependencies = build_transitive_dependencies(
-			each_node_meta.expression.dependencies,
-			context
-		);
-		indirect_dependencies.push(...transitive_dependencies);
-	}
-
 	const child_state = {
 		...context.state,
 		transform: { ...context.state.transform },
@@ -181,19 +159,51 @@ export function EachBlock(node, context) {
 	/** @type {Statement[]} */
 	const declarations = [];
 
-	const invalidate = b.call(
-		'$.invalidate_inner_signals',
-		b.thunk(b.sequence(indirect_dependencies))
-	);
-
 	const invalidate_store = store_to_invalidate
 		? b.call('$.invalidate_store', b.id('$$stores'), b.literal(store_to_invalidate))
 		: undefined;
 
 	/** @type {Expression[]} */
 	const sequence = [];
-	if (!context.state.analysis.runes) sequence.push(invalidate);
-	if (invalidate_store) sequence.push(invalidate_store);
+
+	if (!context.state.analysis.runes) {
+		/** @type {Set<Identifier>} */
+		const transitive_deps = new Set();
+
+		if (collection_id) {
+			transitive_deps.add(collection_id);
+			child_state.transform[collection_id.name] = { read: b.call };
+		} else {
+			for (const binding of each_node_meta.transitive_deps) {
+				transitive_deps.add(binding.node);
+			}
+		}
+
+		for (const block of collect_parent_each_blocks(context)) {
+			for (const binding of block.metadata.transitive_deps) {
+				transitive_deps.add(binding.node);
+			}
+		}
+
+		if (transitive_deps.size > 0) {
+			const invalidate = b.call(
+				'$.invalidate_inner_signals',
+				b.thunk(
+					b.sequence(
+						[...transitive_deps].map(
+							(node) => /** @type {Expression} */ (context.visit({ ...node }, child_state))
+						)
+					)
+				)
+			);
+
+			sequence.push(invalidate);
+		}
+	}
+
+	if (invalidate_store) {
+		sequence.push(invalidate_store);
+	}
 
 	if (node.context?.type === 'Identifier') {
 		const binding = /** @type {Binding} */ (context.state.scope.get(node.context.name));
@@ -329,41 +339,3 @@ export function EachBlock(node, context) {
 function collect_parent_each_blocks(context) {
 	return /** @type {AST.EachBlock[]} */ (context.path.filter((node) => node.type === 'EachBlock'));
 }
-
-/**
- * @param {Set<Binding>} references
- * @param {ComponentContext} context
- */
-function build_transitive_dependencies(references, context) {
-	/** @type {Set<Binding>} */
-	const dependencies = new Set();
-
-	for (const ref of references) {
-		const deps = collect_transitive_dependencies(ref);
-		for (const dep of deps) {
-			dependencies.add(dep);
-		}
-	}
-
-	return [...dependencies].map((dep) => build_getter({ ...dep.node }, context.state));
-}
-
-/**
- * @param {Binding} binding
- * @param {Set<Binding>} seen
- * @returns {Binding[]}
- */
-function collect_transitive_dependencies(binding, seen = new Set()) {
-	if (binding.kind !== 'legacy_reactive') return [];
-
-	for (const dep of binding.legacy_dependencies) {
-		if (!seen.has(dep)) {
-			seen.add(dep);
-			for (const transitive_dep of collect_transitive_dependencies(dep, seen)) {
-				seen.add(transitive_dep);
-			}
-		}
-	}
-
-	return [...seen];
-}

packages/svelte/src/compiler/phases/scope.js

@@ -1169,7 +1169,9 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) {
 				contains_group_binding: false,
 				index: scope.root.unique('$$index'),
 				declarations: scope.declarations,
-				is_controlled: false
+				is_controlled: false,
+				// filled in during analysis
+				transitive_deps: new Set()
 			};
 		},
 

packages/svelte/src/compiler/types/template.d.ts

@@ -432,6 +432,11 @@ export namespace AST {
 			 * This saves us from creating an extra comment and insertion being faster.
 			 */
 			is_controlled: boolean;
+			/**
+			 * Bindings this each block transitively depends on. In legacy mode, we
+			 * invalidate these bindings when mutations happen to each block items
+			 */
+			transitive_deps: Set<Binding>;
 		};
 	}
 

packages/svelte/src/internal/client/reactivity/props.js

@@ -335,7 +335,8 @@ export function prop(props, key, flags, fallback) {
 	}
 
 	// easy mode — prop is never written to
-	if ((flags & PROPS_IS_UPDATED) === 0) {
+	// TODO i think the `&& runes` might now be superfluous
+	if ((flags & PROPS_IS_UPDATED) === 0 && runes) {
 		return getter;
 	}
 

packages/svelte/tests/migrate/samples/each-block-const/output.svelte

@@ -1,5 +1,5 @@
 <script>
-	const { foo } = x();
+	const { foo } = $state(x());
 </script>
 
-{#each foo as f}{/each}
+{#each foo as f}{/each}

packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/_config.js

@@ -0,0 +1,156 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	get props() {
+		return {
+			items: [
+				{ done: false, text: 'one' },
+				{ done: true, text: 'two' },
+				{ done: false, text: 'three' }
+			]
+		};
+	},
+
+	html: `
+		<div>
+			<input type="checkbox">
+			<input type="text"><p>one</p>
+		</div>
+		<div>
+			<input type="checkbox">
+			<input type="text"><p>two</p>
+		</div>
+		<div>
+			<input type="checkbox">
+			<input type="text"><p>three</p>
+		</div>
+
+		<p>remaining:one / done:two / remaining:three</p>
+	`,
+
+	ssrHtml: `
+		<div>
+			<input type="checkbox">
+			<input type="text" value=one><p>one</p>
+		</div>
+		<div>
+			<input type="checkbox" checked="">
+			<input type="text" value=two><p>two</p>
+		</div>
+		<div>
+			<input type="checkbox">
+			<input type="text" value=three><p>three</p>
+		</div>
+
+		<p>remaining:one / done:two / remaining:three</p>
+	`,
+
+	test({ assert, component, target, window }) {
+		/**
+		 * @param {number} i
+		 * @param {string} text
+		 */
+		function set_text(i, text) {
+			const input = /** @type {HTMLInputElement} */ (
+				target.querySelectorAll('input[type="text"]')[i]
+			);
+			input.value = text;
+			input.dispatchEvent(new window.Event('input'));
+		}
+
+		/**
+		 * @param {number} i
+		 * @param {boolean} done
+		 */
+		function set_done(i, done) {
+			const input = /** @type {HTMLInputElement} */ (
+				target.querySelectorAll('input[type="checkbox"]')[i]
+			);
+			input.checked = done;
+			input.dispatchEvent(new window.Event('change'));
+		}
+
+		component.filter = 'remaining';
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>one</p>
+			</div>
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>three</p>
+			</div>
+
+			<p>remaining:one / done:two / remaining:three</p>
+		`
+		);
+
+		set_text(1, 'four');
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>one</p>
+			</div>
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>four</p>
+			</div>
+
+			<p>remaining:one / done:two / remaining:four</p>
+		`
+		);
+
+		assert.deepEqual(component.items, [
+			{ done: false, text: 'one' },
+			{ done: true, text: 'two' },
+			{ done: false, text: 'four' }
+		]);
+
+		set_done(0, true);
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>four</p>
+			</div>
+
+			<p>done:one / done:two / remaining:four</p>
+		`
+		);
+
+		assert.deepEqual(component.items, [
+			{ done: true, text: 'one' },
+			{ done: true, text: 'two' },
+			{ done: false, text: 'four' }
+		]);
+
+		component.filter = 'done';
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>one</p>
+			</div>
+			<div>
+				<input type="checkbox">
+				<input type="text"><p>two</p>
+			</div>
+
+			<p>done:one / done:two / remaining:four</p>
+		`
+		);
+	}
+});