fix: on teardown, use the last known value for the signal before the set

.changeset/sharp-elephants-invite.md

@@ -0,0 +1,5 @@
+---
+'svelte': minor
+---
+
+fix: make values consistent between effects and their cleanup functions

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

@@ -42,6 +42,9 @@ export function CallExpression(node, context) {
 				e.bindable_invalid_location(node);
 			}
 
+			// We need context in case the bound prop is stale
+			context.state.analysis.needs_context = true;
+
 			break;
 
 		case '$host':

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

@@ -11,7 +11,7 @@ import {
 	set_active_reaction,
 	untrack
 } from './runtime.js';
-import { effect } from './reactivity/effects.js';
+import { effect, teardown } from './reactivity/effects.js';
 import { legacy_mode_flag } from '../flags/index.js';
 
 /** @type {ComponentContext | null} */
@@ -112,15 +112,16 @@ export function getAllContexts() {
  * @returns {void}
  */
 export function push(props, runes = false, fn) {
-	component_context = {
+	var ctx = (component_context = {
 		p: component_context,
 		c: null,
+		d: false,
 		e: null,
 		m: false,
 		s: props,
 		x: null,
 		l: null
-	};
+	});
 
 	if (legacy_mode_flag && !runes) {
 		component_context.l = {
@@ -131,6 +132,10 @@ export function push(props, runes = false, fn) {
 		};
 	}
 
+	teardown(() => {
+		/** @type {ComponentContext} */ (ctx).d = true;
+	});
+
 	if (DEV) {
 		// component function
 		component_context.function = fn;

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

@@ -1,4 +1,4 @@
-/** @import { Source } from './types.js' */
+/** @import { Derived, Source } from './types.js' */
 import { DEV } from 'esm-env';
 import {
 	PROPS_IS_BINDABLE,
@@ -10,24 +10,10 @@ import {
 import { get_descriptor, is_function } from '../../shared/utils.js';
 import { mutable_source, set, source, update } from './sources.js';
 import { derived, derived_safe_equal } from './deriveds.js';
-import {
-	active_effect,
-	get,
-	captured_signals,
-	set_active_effect,
-	untrack,
-	active_reaction,
-	set_active_reaction
-} from '../runtime.js';
+import { get, captured_signals, untrack } from '../runtime.js';
 import { safe_equals } from './equality.js';
 import * as e from '../errors.js';
-import {
-	BRANCH_EFFECT,
-	LEGACY_DERIVED_PROP,
-	LEGACY_PROPS,
-	ROOT_EFFECT,
-	STATE_SYMBOL
-} from '../constants.js';
+import { LEGACY_DERIVED_PROP, LEGACY_PROPS, STATE_SYMBOL } from '../constants.js';
 import { proxy } from '../proxy.js';
 import { capture_store_binding } from './store.js';
 import { legacy_mode_flag } from '../../flags/index.js';
@@ -249,6 +235,14 @@ export function spread_props(...props) {
 	return new Proxy({ props }, spread_props_handler);
 }
 
+/**
+ * @param {Derived} current_value
+ * @returns {boolean}
+ */
+function has_destroyed_component_ctx(current_value) {
+	return current_value.ctx?.d ?? false;
+}
+
 /**
  * This function is responsible for synchronizing a possibly bound prop with the inner component state.
  * It is used whenever the compiler sees that the component writes to the prop, or when it has a default prop_value.
@@ -382,6 +376,11 @@ export function prop(props, key, flags, fallback) {
 		return (inner_current_value.v = parent_value);
 	});
 
+	// Ensure we eagerly capture the initial value if it's bindable
+	if (bindable) {
+		get(current_value);
+	}
+
 	if (!immutable) current_value.equals = safe_equals;
 
 	return function (/** @type {any} */ value, /** @type {boolean} */ mutation) {
@@ -408,11 +407,21 @@ export function prop(props, key, flags, fallback) {
 				if (fallback_used && fallback_value !== undefined) {
 					fallback_value = new_value;
 				}
+
+				if (has_destroyed_component_ctx(current_value)) {
+					return value;
+				}
+
 				untrack(() => get(current_value)); // force a synchronisation immediately
 			}
 
 			return value;
 		}
+
+		if (has_destroyed_component_ctx(current_value)) {
+			return current_value.v;
+		}
+
 		return get(current_value);
 	};
 }

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

@@ -14,7 +14,8 @@ import {
 	derived_sources,
 	set_derived_sources,
 	check_dirtiness,
-	untracking
+	untracking,
+	is_destroying_effect
 } from '../runtime.js';
 import { equals, safe_equals } from './equality.js';
 import {
@@ -34,6 +35,7 @@ import { get_stack } from '../dev/tracing.js';
 import { component_context, is_runes } from '../context.js';
 
 export let inspect_effects = new Set();
+export const old_values = new Map();
 
 /**
  * @param {Set<any>} v
@@ -168,6 +170,13 @@ export function set(source, value) {
 export function internal_set(source, value) {
 	if (!source.equals(value)) {
 		var old_value = source.v;
+
+		if (is_destroying_effect) {
+			old_values.set(source, value);
+		} else {
+			old_values.set(source, old_value);
+		}
+
 		source.v = value;
 		source.wv = increment_write_version();
 

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

@@ -25,7 +25,7 @@ import {
 	BOUNDARY_EFFECT
 } from './constants.js';
 import { flush_tasks } from './dom/task.js';
-import { internal_set } from './reactivity/sources.js';
+import { internal_set, old_values } from './reactivity/sources.js';
 import { destroy_derived_effects, update_derived } from './reactivity/deriveds.js';
 import * as e from './errors.js';
 import { FILENAME } from '../../constants.js';
@@ -673,6 +673,7 @@ function flush_queued_root_effects() {
 		if (DEV) {
 			dev_effect_stack = [];
 		}
+		old_values.clear();
 	}
 }
 
@@ -923,6 +924,10 @@ export function get(signal) {
 		}
 	}
 
+	if (is_destroying_effect && old_values.has(signal)) {
+		return old_values.get(signal);
+	}
+
 	return signal.v;
 }
 

packages/svelte/src/internal/client/types.d.ts

@@ -14,6 +14,8 @@ export type ComponentContext = {
 	p: null | ComponentContext;
 	/** context */
 	c: null | Map<unknown, unknown>;
+	/** destroyed */
+	d: boolean;
 	/** deferred effects */
 	e: null | Array<{
 		fn: () => void | (() => void);

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

@@ -0,0 +1,11 @@
+<script>
+	import { onDestroy } from 'svelte';
+
+	export let my_prop;
+
+	onDestroy(() => {
+		console.log(my_prop.foo);
+	});
+</script>
+
+{my_prop.foo}

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

@@ -0,0 +1,14 @@
+import { test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	async test({ assert, target, logs }) {
+		const [btn1] = target.querySelectorAll('button');
+
+		flushSync(() => {
+			btn1.click();
+		});
+
+		assert.deepEqual(logs, ['bar']);
+	}
+});

packages/svelte/tests/runtime-legacy/samples/ondestroy-prop-access-2/main.svelte

@@ -0,0 +1,15 @@
+<script>
+	import Component from './Component.svelte';
+
+	let value = { foo: 'bar' };
+</script>
+
+<button
+	onclick={() => {
+		value = undefined;
+	}}>Reset value</button
+>
+
+{#if value !== undefined}
+	<Component my_prop={value} />
+{/if}

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

@@ -0,0 +1,5 @@
+<script lang="ts">
+	export let ref;
+</script>
+
+<input bind:this={ref} />

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

@@ -0,0 +1,11 @@
+import { test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	async test({ target }) {
+		const [btn1] = target.querySelectorAll('button');
+
+		btn1.click();
+		flushSync();
+	}
+});

packages/svelte/tests/runtime-legacy/samples/ondestroy-prop-access-3/main.svelte

@@ -0,0 +1,16 @@
+<script>
+	import Component from './Component.svelte';
+	let state = { title: 'foo' };
+</script>
+
+{#if state}
+	{@const attributes = { title: state.title }}
+	<Component {...attributes} />
+{/if}
+<button
+	onclick={() => {
+		state = undefined;
+	}}
+>
+	Del
+</button>

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

@@ -0,0 +1,12 @@
+<script>
+	import { onDestroy } from "svelte";
+	export let checked;
+	export let count;
+	onDestroy(() => {
+		console.log(count, checked);
+	});
+</script>
+
+<p>{count}</p>
+
+<button onclick={()=> count-- }></button>

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

@@ -0,0 +1,68 @@
+import { 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
+		]);
+	}
+});

packages/svelte/tests/runtime-legacy/samples/ondestroy-prop-access/main.svelte

@@ -0,0 +1,41 @@
+<script>
+	import Component from "./Component.svelte";
+	let show = true;
+	let count = 0;
+	$: spread = { checked: show, count };
+</script>
+
+<button onclick={()=> count++ }></button>
+<button onclick={()=> show = !show }></button>
+
+<!-- count with bind -->
+{#if count < 2}
+	<Component bind:count bind:checked={show} />
+{/if}
+
+<!-- spread syntax -->
+{#if count < 2}
+	<Component {...spread} />
+{/if}
+
+<!-- normal prop -->
+{#if count < 2}
+	<Component {count} checked={show} />
+{/if}
+
+<!-- prop only accessed in destroy -->
+{#if show}
+	<Component {count} checked={show} />
+{/if}
+
+<!-- dynamic component -->
+<svelte:component this={count < 2 ? Component : undefined} {count} checked={show} />
+
+<!-- dynamic component spread -->
+<svelte:component this={count < 2 ? Component : undefined} {...spread} />
+
+<!-- dynamic component with prop only accessed on destroy -->
+<svelte:component this={show ? Component : undefined} {count} checked={show} />
+
+<!-- dynamic component with prop only accessed on destroy spread -->
+<svelte:component this={show ? Component : undefined} {...spread} />

packages/svelte/tests/runtime-runes/samples/nested-effect-conflict/_config.js

@@ -10,14 +10,6 @@ export default test({
 		});
 
 		await Promise.resolve();
-		assert.deepEqual(logs, [
-			'top level',
-			'inner',
-			0,
-			'destroy inner',
-			undefined,
-			'destroy outer',
-			undefined
-		]);
+		assert.deepEqual(logs, ['top level', 'inner', 0, 'destroy inner', 0, 'destroy outer', 0]);
 	}
 });