fix: better binding interop between runes/non-runes components (#12123)

dummdidumm · Rich-Harris · web-flow · commit afe84450c6d5 · 2024-06-21T15:44:00.000-04:00
* Ensure binding from legacy component passed to runes component updates correctly. This is done by also using the `prop(..)` variant for a property if it's mutated in runes mode, and then figuring out at runtime whether or not the parent should be notified or not fixes #12032 * fix adjacent bug around wrong value getting return upon mutation * deduplicate * changeset * simplify * move comment * rename --------- Co-authored-by: Rich Harris <rich.harris@vercel.com>
diff --git a/.changeset/nervous-ducks-repeat.md b/.changeset/nervous-ducks-repeat.md
@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: better binding interop between runes/non-runes components
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js b/packages/svelte/src/compiler/phases/3-transform/client/transform-client.js
@@ -363,7 +363,12 @@ export function client_component(source, analysis, options) {
 	}
 
 	if (analysis.uses_props || analysis.uses_rest_props) {
-		const to_remove = [b.literal('children'), b.literal('$$slots'), b.literal('$$events')];
+		const to_remove = [
+			b.literal('children'),
+			b.literal('$$slots'),
+			b.literal('$$events'),
+			b.literal('$$legacy')
+		];
 		if (analysis.custom_element) {
 			to_remove.push(b.literal('$$host'));
 		}
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js
@@ -89,7 +89,7 @@ export function serialize_get_binding(node, state) {
 	}
 
 	if (binding.kind === 'prop' || binding.kind === 'bindable_prop') {
-		if (!state.analysis.runes || binding.reassigned || binding.initial) {
+		if (is_prop_source(binding, state)) {
 			return b.call(node);
 		}
 
@@ -391,18 +391,20 @@ export function serialize_set_binding(node, context, fallback, prefix, options)
 					),
 					b.call('$.untrack', b.id('$' + left_name))
 				);
-			} else if (!state.analysis.runes) {
+			} else if (
+				!state.analysis.runes ||
+				// this condition can go away once legacy mode is gone; only necessary for interop with legacy parent bindings
+				(binding.mutated && binding.kind === 'bindable_prop')
+			) {
 				if (binding.kind === 'bindable_prop') {
 					return b.call(
 						left,
-						b.sequence([
-							b.assignment(
-								node.operator,
-								/** @type {import('estree').Pattern} */ (visit(node.left)),
-								value
-							),
-							b.call(left)
-						])
+						b.assignment(
+							node.operator,
+							/** @type {import('estree').Pattern} */ (visit(node.left)),
+							value
+						),
+						b.true
 					);
 				} else {
 					return b.call(
@@ -538,9 +540,7 @@ function get_hoistable_params(node, context) {
 			} else if (
 				// If we are referencing a simple $$props value, then we need to reference the object property instead
 				(binding.kind === 'prop' || binding.kind === 'bindable_prop') &&
-				!binding.reassigned &&
-				binding.initial === null &&
-				!context.state.analysis.accessors
+				!is_prop_source(binding, context.state)
 			) {
 				push_unique(b.id('$$props'));
 			} else {
@@ -602,7 +602,9 @@ export function get_prop_source(binding, state, name, initial) {
 
 	if (
 		state.analysis.accessors ||
-		(state.analysis.immutable ? binding.reassigned : binding.mutated)
+		(state.analysis.immutable
+			? binding.reassigned || (state.analysis.runes && binding.mutated)
+			: binding.mutated)
 	) {
 		flags |= PROPS_IS_UPDATED;
 	}
@@ -637,6 +639,25 @@ export function get_prop_source(binding, state, name, initial) {
 	return b.call('$.prop', ...args);
 }
 
+/**
+ *
+ * @param {import('#compiler').Binding} binding
+ * @param {import('./types').ClientTransformState} state
+ * @returns
+ */
+export function is_prop_source(binding, state) {
+	return (
+		(binding.kind === 'prop' || binding.kind === 'bindable_prop') &&
+		(!state.analysis.runes ||
+			state.analysis.accessors ||
+			binding.reassigned ||
+			binding.initial ||
+			// Until legacy mode is gone, we also need to use the prop source when only mutated is true,
+			// because the parent could be a legacy component which needs coarse-grained reactivity
+			binding.mutated)
+	);
+}
+
 /**
  * @param {import('estree').Expression} node
  * @param {import("../../scope.js").Scope | null} scope
diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-runes.js
@@ -4,6 +4,7 @@ import * as b from '../../../../utils/builders.js';
 import * as assert from '../../../../utils/assert.js';
 import {
 	get_prop_source,
+	is_prop_source,
 	is_state_source,
 	serialize_proxy_reassignment,
 	should_proxy_or_freeze
@@ -240,7 +241,11 @@ export const javascript_visitors_runes = {
 				assert.equal(declarator.id.type, 'ObjectPattern');
 
 				/** @type {string[]} */
-				const seen = [];
+				const seen = ['$$slots', '$$events', '$$legacy'];
+
+				if (state.analysis.custom_element) {
+					seen.push('$$host');
+				}
 
 				for (const property of declarator.id.properties) {
 					if (property.type === 'Property') {
@@ -264,7 +269,7 @@ export const javascript_visitors_runes = {
 							initial = b.call('$.proxy', initial);
 						}
 
-						if (binding.reassigned || state.analysis.accessors || initial) {
+						if (is_prop_source(binding, state)) {
 							declarations.push(b.declarator(id, get_prop_source(binding, state, name, initial)));
 						}
 					} else {
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
@@ -900,6 +900,10 @@ function serialize_inline_component(node, component_name, context) {
 		push_prop(b.init('$$slots', b.object(serialized_slots)));
 	}
 
+	if (!context.state.analysis.runes) {
+		push_prop(b.init('$$legacy', b.true));
+	}
+
 	const props_expression =
 		props_and_spreads.length === 0 ||
 		(props_and_spreads.length === 1 && Array.isArray(props_and_spreads[0]))
diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js
@@ -267,9 +267,16 @@ export function prop(props, key, flags, fallback) {
 	// intermediate mode — prop is written to, but the parent component had
 	// `bind:foo` which means we can just call `$$props.foo = value` directly
 	if (setter) {
-		return function (/** @type {V} */ value) {
-			if (arguments.length === 1) {
-				/** @type {Function} */ (setter)(value);
+		var legacy_parent = props.$$legacy;
+		return function (/** @type {any} */ value, /** @type {boolean} */ mutation) {
+			if (arguments.length > 0) {
+				// We don't want to notify if the value was mutated and the parent is in runes mode.
+				// In that case the state proxy (if it exists) should take care of the notification.
+				// If the parent is not in runes mode, we need to notify on mutation, too, that the prop
+				// has changed because the parent will not be able to detect the change otherwise.
+				if (!runes || !mutation || legacy_parent) {
+					/** @type {Function} */ (setter)(mutation ? getter() : value);
+				}
 				return value;
 			} else {
 				return getter();
@@ -302,7 +309,7 @@ export function prop(props, key, flags, fallback) {
 
 	if (!immutable) current_value.equals = safe_equals;
 
-	return function (/** @type {V} */ value) {
+	return function (/** @type {any} */ value, /** @type {boolean} */ mutation) {
 		var current = get(current_value);
 
 		// legacy nonsense — need to ensure the source is invalidated when necessary
@@ -318,9 +325,11 @@ export function prop(props, key, flags, fallback) {
 		}
 
 		if (arguments.length > 0) {
-			if (!current_value.equals(value)) {
+			const new_value = mutation ? get(current_value) : value;
+
+			if (!current_value.equals(new_value)) {
 				from_child = true;
-				set(inner_current_value, value);
+				set(inner_current_value, new_value);
 				get(current_value); // force a synchronisation immediately
 			}
 
diff --git a/packages/svelte/tests/runtime-legacy/samples/mutation-correct-return-value/_config.js b/packages/svelte/tests/runtime-legacy/samples/mutation-correct-return-value/_config.js
@@ -0,0 +1,8 @@
+import { test } from '../../test';
+
+export default test({
+	mode: ['client'],
+	test({ assert, logs }) {
+		assert.deepEqual(logs, [true]);
+	}
+});
diff --git a/packages/svelte/tests/runtime-legacy/samples/mutation-correct-return-value/main.svelte b/packages/svelte/tests/runtime-legacy/samples/mutation-correct-return-value/main.svelte
@@ -0,0 +1,4 @@
+<script>
+	export let a = {};
+	console.log((a.b = true));
+</script>
diff --git a/packages/svelte/tests/runtime-runes/samples/binding-interop/Component1.svelte b/packages/svelte/tests/runtime-runes/samples/binding-interop/Component1.svelte
@@ -0,0 +1,9 @@
+<script>
+	let { object = $bindable(), primitive = $bindable() } = $props();
+</script>
+
+{#if primitive}
+	<button onclick={() => (primitive = 'bar')}>{primitive}</button>
+{:else}
+	<button onclick={() => (object.value = 'bar')}>{object.value}</button>
+{/if}
diff --git a/packages/svelte/tests/runtime-runes/samples/binding-interop/Component2.svelte b/packages/svelte/tests/runtime-runes/samples/binding-interop/Component2.svelte
@@ -0,0 +1,9 @@
+<script>
+	let { object = $bindable({}), primitive = $bindable('') } = $props();
+</script>
+
+{#if primitive}
+	<button onclick={() => (primitive = 'bar')}>{primitive}</button>
+{:else}
+	<button onclick={() => (object.value = 'bar')}>{object.value}</button>
+{/if}
diff --git a/packages/svelte/tests/runtime-runes/samples/binding-interop/Legacy.svelte b/packages/svelte/tests/runtime-runes/samples/binding-interop/Legacy.svelte
@@ -0,0 +1,24 @@
+<svelte:options runes={false} />
+
+<script>
+	import Component1 from './Component1.svelte';
+	import Component2 from './Component2.svelte';
+
+	let object1 = { value: 'foo' };
+	let object2 = { value: 'foo' };
+
+	let primitive1 = 'foo';
+	let primitive2 = 'foo';
+</script>
+
+{object1.value}
+<Component1 bind:object={object1} />
+
+{object2.value}
+<Component2 bind:object={object2} />
+
+{primitive1}
+<Component1 bind:primitive={primitive1} />
+
+{primitive2}
+<Component2 bind:primitive={primitive2} />
diff --git a/packages/svelte/tests/runtime-runes/samples/binding-interop/Runes.svelte b/packages/svelte/tests/runtime-runes/samples/binding-interop/Runes.svelte
@@ -0,0 +1,39 @@
+<script>
+	import Component1 from './Component1.svelte';
+	import Component2 from './Component2.svelte';
+
+	let object1 = $state({ value: 'foo' });
+	let object2 = $state({ value: 'foo' });
+
+	class Frozen {
+		constructor(value) {
+			this.value = value;
+		}
+	}
+	let object3 = $state(new Frozen('foo'));
+	let object4 = $state(new Frozen('foo'));
+
+	let primitive1 = $state('foo');
+	let primitive2 = $state('foo');
+</script>
+
+{object1.value}
+<Component1 bind:object={object1} />
+
+{object2.value}
+<Component2 bind:object={object2} />
+
+<!-- force them into a different render effect so they don't coincidently update with the others -->
+{#if true}
+	{object3.value}
+	<Component1 bind:object={object3} />
+
+	{object4.value}
+	<Component2 bind:object={object4} />
+{/if}
+
+{primitive1}
+<Component1 bind:primitive={primitive1} />
+
+{primitive2}
+<Component2 bind:primitive={primitive2} />
diff --git a/packages/svelte/tests/runtime-runes/samples/binding-interop/_config.js b/packages/svelte/tests/runtime-runes/samples/binding-interop/_config.js
@@ -0,0 +1,24 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	mode: ['client'],
+	async test({ assert, target }) {
+		const buttons = target.querySelectorAll('button');
+
+		for (const button of buttons) {
+			await button.click();
+			flushSync();
+		}
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+			bar <button>bar</button> bar <button>bar</button> bar <button>bar</button> bar <button>bar</button>
+			<hr>
+			bar <button>bar</button> bar <button>bar</button> foo <button>foo</button> foo <button>foo</button> bar <button>bar</button> bar <button>bar</button>
+			`
+		);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/binding-interop/main.svelte b/packages/svelte/tests/runtime-runes/samples/binding-interop/main.svelte
@@ -0,0 +1,10 @@
+<script>
+	import Legacy from './Legacy.svelte';
+	import Runes from './Runes.svelte';
+</script>
+
+<Legacy />
+
+<hr />
+
+<Runes />
diff --git a/packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/bind-this/_expected/client/index.svelte.js
@@ -5,6 +5,6 @@ export default function Bind_this($$anchor) {
 	var fragment = $.comment();
 	var node = $.first_child(fragment);
 
-	$.bind_this(Foo(node, {}), ($$value) => foo = $$value, () => foo);
+	$.bind_this(Foo(node, { $$legacy: true }), ($$value) => foo = $$value, () => foo);
 	$.append($$anchor, fragment);
 }

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"svelte": patch
 +---
++
 +fix: better binding interop between runes/non-runes components
Original file line number	Diff line number	Diff line change
`@@ -363,7 +363,12 @@ export function client_component(source, analysis, options) {`
`363`	`363`	`}`
`364`	`364`
`365`	`365`	`if (analysis.uses_props \|\| analysis.uses_rest_props) {`
`366`		`- const to_remove = [b.literal('children'), b.literal('$$slots'), b.literal('$$events')];`
	`366`	`+ const to_remove = [`
	`367`	`+ b.literal('children'),`
	`368`	`+ b.literal('$$slots'),`
	`369`	`+ b.literal('$$events'),`
	`370`	`+ b.literal('$$legacy')`
	`371`	`+ ];`
`367`	`372`	`if (analysis.custom_element) {`
`368`	`373`	`to_remove.push(b.literal('$$host'));`
`369`	`374`	`}`