fix: add test and change strategy

Author: paoloricciuti

packages/svelte/src/compiler/phases/3-transform/client/types.d.ts

@@ -23,8 +23,6 @@ export interface ClientTransformState extends TransformState {
 	 * us to rewrite `this.foo` as `this.#foo.value`
 	 */
 	readonly in_constructor: boolean;
-	readonly safe_props_ids?: Map<string, Expression>;
-	readonly safe_props_name?: string;
 
 	readonly transform: Record<
 		string,
@@ -47,6 +45,7 @@ export interface ComponentClientTransformState extends ClientTransformState {
 	readonly hoisted: Array<Statement | ModuleDeclaration>;
 	readonly events: Set<string>;
 	readonly is_instance: boolean;
+	readonly needs_safe_props: boolean;
 	readonly store_to_invalidate?: string;
 
 	/** Stuff that happens before the render effect(s) */

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

@@ -10,10 +10,6 @@ import { build_component } from './shared/component.js';
  */
 export function Component(node, context) {
 	if (node.metadata.dynamic) {
-		let safe_props_ids = new Map();
-
-		const safe_props_name = context.state.scope.generate('$$safe_props');
-
 		// Handle dynamic references to what seems like static inline components
 		const component = build_component(
 			node,
@@ -22,8 +18,7 @@ export function Component(node, context) {
 				...context,
 				state: {
 					...context.state,
-					safe_props_ids,
-					safe_props_name
+					needs_safe_props: true
 				}
 			},
 			b.id('$$anchor')
@@ -36,19 +31,7 @@ export function Component(node, context) {
 					// TODO use untrack here to not update when binding changes?
 					// Would align with Svelte 4 behavior, but it's arguably nicer/expected to update this
 					b.thunk(/** @type {Expression} */ (context.visit(b.member_id(node.name)))),
-					b.arrow(
-						[b.id('$$anchor'), b.id('$$component')],
-						b.block([
-							b.const(
-								safe_props_name,
-								b.call(
-									'$.safe_props',
-									b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
-								)
-							),
-							component
-						])
-					)
+					b.arrow([b.id('$$anchor'), b.id('$$component')], b.block([component]))
 				)
 			)
 		);

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

@@ -9,9 +9,9 @@ import { build_getter } from '../utils.js';
  * @param {Context} context
  */
 export function Identifier(node, context) {
-	let parent = context.path.at(-1);
+	const parent = /** @type {Node} */ (context.path.at(-1));
 
-	if (is_reference(node, /** @type {Node} */ (parent))) {
+	if (is_reference(node, parent)) {
 		if (node.name === '$$props') {
 			return b.id('$$sanitized_props');
 		}
@@ -36,36 +36,6 @@ export function Identifier(node, context) {
 			}
 		}
 
-		const getter = build_getter(node, context.state);
-
-		if (
-			// this means we are inside an if or as an attribute of a dynamic component
-			// and we want to access `$$safe_props` to allow for the component to access them
-			// after destructuring
-			context.state.safe_props_name != null &&
-			context.state.safe_props_ids != null &&
-			// the parent can either be a component/svelte component in that case we
-			// check if this identifier is one of the attributes
-			(((parent?.type === 'Component' || parent?.type === 'SvelteComponent') &&
-				parent.attributes.some(
-					(el) =>
-						(el.type === 'Attribute' &&
-							typeof el.value !== 'boolean' &&
-							!Array.isArray(el.value) &&
-							el.value.expression === node) ||
-						(el.type === 'BindDirective' && el.expression === node)
-				)) ||
-				// or a spread and we check the expression
-				(parent?.type === 'SpreadAttribute' && parent.expression === node)) &&
-			// we also don't want to transform bindings that are defined withing the if block
-			// itself (for example an each local variable)
-			!binding?.references[0].path.some((node) => node.type === 'IfBlock')
-		) {
-			// we store the getter in the safe props id and return an access to `$$safe_props.name`
-			context.state.safe_props_ids.set(node.name, getter);
-			return b.member(b.id(context.state.safe_props_name), b.id(node.name));
-		}
-
-		return getter;
+		return build_getter(node, context.state);
 	}
 }

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

@@ -11,37 +11,26 @@ export function IfBlock(node, context) {
 	context.state.template.push('<!>');
 	const statements = [];
 
-	let safe_props_ids = new Map();
-
-	const safe_props_id = context.state.scope.generate('$$safe_props');
-
 	const consequent = /** @type {BlockStatement} */ (
 		context.visit(node.consequent, {
 			...context.state,
-			safe_props_ids,
-			safe_props_name: safe_props_id
+			needs_safe_props: true
 		})
 	);
 
-	if (consequent.body.length > 0 && safe_props_ids) {
-		consequent.body.unshift(
-			b.const(
-				safe_props_id,
-				b.call(
-					'$.safe_props',
-					b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
-				)
-			)
-		);
-	}
 	const consequent_id = context.state.scope.generate('consequent');
 
 	statements.push(b.var(b.id(consequent_id), b.arrow([b.id('$$anchor')], consequent)));
 
 	let alternate_id;
 
 	if (node.alternate) {
-		const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate));
+		const alternate = /** @type {BlockStatement} */ (
+			context.visit(node.alternate, {
+				...context.state,
+				needs_safe_props: true
+			})
+		);
 		alternate_id = context.state.scope.generate('alternate');
 		statements.push(b.var(b.id(alternate_id), b.arrow([b.id('$$anchor')], alternate)));
 	}

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

@@ -92,6 +92,21 @@ export function build_component(node, component_name, context, anchor = context.
 		}
 	}
 
+	let safe_props_ids = new Map();
+	let safe_props_name = context.state.scope.generate('$$safe_props');
+
+	/**
+	 * @param {string} name
+	 * @param {Expression} expression
+	 */
+	function safe_propify(name, expression) {
+		if (context.state.needs_safe_props) {
+			safe_props_ids.set(name, expression);
+			return b.member(b.id(safe_props_name), b.id(name));
+		}
+		return expression;
+	}
+
 	for (const attribute of node.attributes) {
 		if (attribute.type === 'LetDirective') {
 			if (!slot_scope_applies_to_itself) {
@@ -118,13 +133,14 @@ export function build_component(node, component_name, context, anchor = context.
 			if (attribute.metadata.expression.has_state) {
 				let value = expression;
 
+				const name = context.state.scope.generate('spread_element');
 				if (attribute.metadata.expression.has_call) {
-					const id = b.id(context.state.scope.generate('spread_element'));
+					const id = b.id(name);
 					context.state.init.push(b.var(id, b.call('$.derived', b.thunk(value))));
 					value = b.call('$.get', id);
 				}
 
-				props_and_spreads.push(b.thunk(value));
+				props_and_spreads.push(b.thunk(safe_propify(name, value)));
 			} else {
 				props_and_spreads.push(expression);
 			}
@@ -172,7 +188,7 @@ export function build_component(node, component_name, context, anchor = context.
 			);
 
 			if (has_state) {
-				push_prop(b.get(attribute.name, [b.return(value)]));
+				push_prop(b.get(attribute.name, [b.return(safe_propify(attribute.name, value))]));
 			} else {
 				push_prop(b.init(attribute.name, value));
 			}
@@ -205,7 +221,9 @@ export function build_component(node, component_name, context, anchor = context.
 					context.state.init.push(b.var(get_id, get));
 					context.state.init.push(b.var(set_id, set));
 
-					push_prop(b.get(attribute.name, [b.return(b.call(get_id))]));
+					push_prop(
+						b.get(attribute.name, [b.return(safe_propify(attribute.name, b.call(get_id)))])
+					);
 					push_prop(b.set(attribute.name, [b.stmt(b.call(set_id, b.id('$$value')))]));
 				}
 			} else {
@@ -228,11 +246,17 @@ export function build_component(node, component_name, context, anchor = context.
 					// Delay prop pushes so bindings come at the end, to avoid spreads overwriting them
 					if (is_store_sub) {
 						push_prop(
-							b.get(attribute.name, [b.stmt(b.call('$.mark_store_binding')), b.return(expression)]),
+							b.get(attribute.name, [
+								b.stmt(b.call('$.mark_store_binding')),
+								b.return(safe_propify(attribute.name, expression))
+							]),
 							true
 						);
 					} else {
-						push_prop(b.get(attribute.name, [b.return(expression)]), true);
+						push_prop(
+							b.get(attribute.name, [b.return(safe_propify(attribute.name, expression))]),
+							true
+						);
 					}
 
 					const assignment = b.assignment(
@@ -403,6 +427,32 @@ export function build_component(node, component_name, context, anchor = context.
 
 	const statements = [...snippet_declarations];
 
+	if (safe_props_ids.size > 0) {
+		// if it is a dynamic component we need to include the safe props call inside the component
+		// function otherwise in the init (which in case of the if will be in the consequent/alternate function)
+		if (component_name === '$$component') {
+			statements.push(
+				b.const(
+					safe_props_name,
+					b.call(
+						'$.safe_props',
+						b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
+					)
+				)
+			);
+		} else {
+			context.state.init.push(
+				b.const(
+					safe_props_name,
+					b.call(
+						'$.safe_props',
+						b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)])))
+					)
+				)
+			);
+		}
+	}
+
 	if (node.type === 'SvelteComponent') {
 		const prev = fn;
 

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

@@ -185,7 +185,7 @@ export function build_attribute_value(value, context, memoize = (value) => value
 			return { value: b.literal(chunk.data), has_state: false };
 		}
 
-		let expression = /** @type {Expression} */ (context.visit(chunk.expression, context.state));
+		let expression = /** @type {Expression} */ (context.visit(chunk.expression));
 
 		return {
 			value: memoize(expression, chunk.metadata.expression),

packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/Component.svelte

@@ -0,0 +1,11 @@
+<script>
+	let { checked = $bindable(), count = $bindable() } = $props();
+
+	$effect(() => ()=>{
+		console.log(count, checked);
+	});
+</script>
+
+<p>{count}</p>
+
+<button onclick={()=> count-- }></button>

packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/_config.js

@@ -0,0 +1,68 @@
+import { ok, test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	async test({ assert, target, logs }) {
+		const [btn1, btn2, btn3] = target.querySelectorAll('button');
+		let ps = [...target.querySelectorAll('p')];
+
+		for (const p of ps) {
+			assert.equal(p.innerHTML, '0');
+		}
+
+		flushSync(() => {
+			btn1.click();
+		});
+
+		// prop update normally if we are not unmounting
+		for (const p of ps) {
+			assert.equal(p.innerHTML, '1');
+		}
+
+		flushSync(() => {
+			btn3.click();
+		});
+
+		// binding still works and update the value correctly
+		for (const p of ps) {
+			assert.equal(p.innerHTML, '0');
+		}
+
+		flushSync(() => {
+			btn1.click();
+		});
+
+		flushSync(() => {
+			btn1.click();
+		});
+
+		console.warn(logs);
+
+		// the five components guarded by `count < 2` unmount and log
+		assert.deepEqual(logs, [1, true, 1, true, 1, true, 1, true, 1, true]);
+
+		flushSync(() => {
+			btn2.click();
+		});
+
+		// the three components guarded by `show` unmount and log
+		assert.deepEqual(logs, [
+			1,
+			true,
+			1,
+			true,
+			1,
+			true,
+			1,
+			true,
+			1,
+			true,
+			2,
+			true,
+			2,
+			true,
+			2,
+			true
+		]);
+	}
+});