fix: use state instead of source in reactive classes (#16239)

* fix: use `state` instead of `source` in reactive classes

* fix: use `active_reaction` as indication to use `source` or `state`

* fix: cleanup `#initial_reaction` on `teardown` to free memory

* fix: use `#source` in `set` too

* unused

* chore: use WeakRef

* use update_version instead of WeakRef in SvelteSet/SvelteMap (#16324)

* tidy up

* tweak comment to remove active_reaction reference

---------

Co-authored-by: Rich Harris <[email protected]>

Author: paoloricciuti

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

@@ -134,6 +134,8 @@ let write_version = 1;
 /** @type {number} Used to version each read of a source of derived to avoid duplicating depedencies inside a reaction */
 let read_version = 0;
 
+export let update_version = read_version;
+
 // If we are working with a get() chain that has no active container,
 // to prevent memory leaks, we skip adding the reaction.
 export let skip_reaction = false;
@@ -267,6 +269,7 @@ export function update_reaction(reaction) {
 	var previous_reaction_sources = source_ownership;
 	var previous_component_context = component_context;
 	var previous_untracking = untracking;
+	var previous_update_version = update_version;
 
 	var flags = reaction.f;
 
@@ -280,7 +283,7 @@ export function update_reaction(reaction) {
 	source_ownership = null;
 	set_component_context(reaction.ctx);
 	untracking = false;
-	read_version++;
+	update_version = ++read_version;
 
 	reaction.f |= EFFECT_IS_UPDATING;
 
@@ -368,6 +371,7 @@ export function update_reaction(reaction) {
 		source_ownership = previous_reaction_sources;
 		set_component_context(previous_component_context);
 		untracking = previous_untracking;
+		update_version = previous_update_version;
 
 		reaction.f ^= EFFECT_IS_UPDATING;
 	}

packages/svelte/src/motion/spring.js

@@ -5,7 +5,7 @@ import { writable } from '../store/shared/index.js';
 import { loop } from '../internal/client/loop.js';
 import { raf } from '../internal/client/timing.js';
 import { is_date } from './utils.js';
-import { set, source } from '../internal/client/reactivity/sources.js';
+import { set, state } from '../internal/client/reactivity/sources.js';
 import { render_effect } from '../internal/client/reactivity/effects.js';
 import { tag } from '../internal/client/dev/tracing.js';
 import { get } from '../internal/client/runtime.js';
@@ -170,9 +170,9 @@ export function spring(value, opts = {}) {
  * @since 5.8.0
  */
 export class Spring {
-	#stiffness = source(0.15);
-	#damping = source(0.8);
-	#precision = source(0.01);
+	#stiffness = state(0.15);
+	#damping = state(0.8);
+	#precision = state(0.01);
 
 	#current;
 	#target;
@@ -194,8 +194,8 @@ export class Spring {
 	 * @param {SpringOpts} [options]
 	 */
 	constructor(value, options = {}) {
-		this.#current = DEV ? tag(source(value), 'Spring.current') : source(value);
-		this.#target = DEV ? tag(source(value), 'Spring.target') : source(value);
+		this.#current = DEV ? tag(state(value), 'Spring.current') : state(value);
+		this.#target = DEV ? tag(state(value), 'Spring.target') : state(value);
 
 		if (typeof options.stiffness === 'number') this.#stiffness.v = clamp(options.stiffness, 0, 1);
 		if (typeof options.damping === 'number') this.#damping.v = clamp(options.damping, 0, 1);

packages/svelte/src/motion/tweened.js

@@ -6,7 +6,7 @@ import { raf } from '../internal/client/timing.js';
 import { loop } from '../internal/client/loop.js';
 import { linear } from '../easing/index.js';
 import { is_date } from './utils.js';
-import { set, source } from '../internal/client/reactivity/sources.js';
+import { set, state } from '../internal/client/reactivity/sources.js';
 import { tag } from '../internal/client/dev/tracing.js';
 import { get, render_effect } from 'svelte/internal/client';
 import { DEV } from 'esm-env';
@@ -191,8 +191,8 @@ export class Tween {
 	 * @param {TweenedOptions<T>} options
 	 */
 	constructor(value, options = {}) {
-		this.#current = source(value);
-		this.#target = source(value);
+		this.#current = state(value);
+		this.#target = state(value);
 		this.#defaults = options;
 
 		if (DEV) {

packages/svelte/src/reactivity/map.js

@@ -2,7 +2,7 @@
 import { DEV } from 'esm-env';
 import { set, source, state } from '../internal/client/reactivity/sources.js';
 import { label, tag } from '../internal/client/dev/tracing.js';
-import { get } from '../internal/client/runtime.js';
+import { get, update_version } from '../internal/client/runtime.js';
 import { increment } from './utils.js';
 
 /**
@@ -56,6 +56,7 @@ export class SvelteMap extends Map {
 	#sources = new Map();
 	#version = state(0);
 	#size = state(0);
+	#update_version = update_version || -1;
 
 	/**
 	 * @param {Iterable<readonly [K, V]> | null | undefined} [value]
@@ -79,6 +80,19 @@ export class SvelteMap extends Map {
 		}
 	}
 
+	/**
+	 * If the source is being created inside the same reaction as the SvelteMap instance,
+	 * we use `state` so that it will not be a dependency of the reaction. Otherwise we
+	 * use `source` so it will be.
+	 *
+	 * @template T
+	 * @param {T} value
+	 * @returns {Source<T>}
+	 */
+	#source(value) {
+		return update_version === this.#update_version ? state(value) : source(value);
+	}
+
 	/** @param {K} key */
 	has(key) {
 		var sources = this.#sources;
@@ -87,7 +101,7 @@ export class SvelteMap extends Map {
 		if (s === undefined) {
 			var ret = super.get(key);
 			if (ret !== undefined) {
-				s = source(0);
+				s = this.#source(0);
 
 				if (DEV) {
 					tag(s, `SvelteMap get(${label(key)})`);
@@ -123,7 +137,7 @@ export class SvelteMap extends Map {
 		if (s === undefined) {
 			var ret = super.get(key);
 			if (ret !== undefined) {
-				s = source(0);
+				s = this.#source(0);
 
 				if (DEV) {
 					tag(s, `SvelteMap get(${label(key)})`);
@@ -154,7 +168,7 @@ export class SvelteMap extends Map {
 		var version = this.#version;
 
 		if (s === undefined) {
-			s = source(0);
+			s = this.#source(0);
 
 			if (DEV) {
 				tag(s, `SvelteMap get(${label(key)})`);
@@ -219,8 +233,7 @@ export class SvelteMap extends Map {
 		if (this.#size.v !== sources.size) {
 			for (var key of super.keys()) {
 				if (!sources.has(key)) {
-					var s = source(0);
-
+					var s = this.#source(0);
 					if (DEV) {
 						tag(s, `SvelteMap get(${label(key)})`);
 					}

packages/svelte/src/reactivity/set.js

@@ -2,7 +2,7 @@
 import { DEV } from 'esm-env';
 import { source, set, state } from '../internal/client/reactivity/sources.js';
 import { label, tag } from '../internal/client/dev/tracing.js';
-import { get } from '../internal/client/runtime.js';
+import { get, update_version } from '../internal/client/runtime.js';
 import { increment } from './utils.js';
 
 var read_methods = ['forEach', 'isDisjointFrom', 'isSubsetOf', 'isSupersetOf'];
@@ -50,6 +50,7 @@ export class SvelteSet extends Set {
 	#sources = new Map();
 	#version = state(0);
 	#size = state(0);
+	#update_version = update_version || -1;
 
 	/**
 	 * @param {Iterable<T> | null | undefined} [value]
@@ -75,6 +76,19 @@ export class SvelteSet extends Set {
 		if (!inited) this.#init();
 	}
 
+	/**
+	 * If the source is being created inside the same reaction as the SvelteSet instance,
+	 * we use `state` so that it will not be a dependency of the reaction. Otherwise we
+	 * use `source` so it will be.
+	 *
+	 * @template T
+	 * @param {T} value
+	 * @returns {Source<T>}
+	 */
+	#source(value) {
+		return update_version === this.#update_version ? state(value) : source(value);
+	}
+
 	// We init as part of the first instance so that we can treeshake this class
 	#init() {
 		inited = true;
@@ -116,7 +130,7 @@ export class SvelteSet extends Set {
 				return false;
 			}
 
-			s = source(true);
+			s = this.#source(true);
 
 			if (DEV) {
 				tag(s, `SvelteSet has(${label(value)})`);

packages/svelte/tests/runtime-runes/samples/side-effect-derived-map/_config.js

@@ -8,7 +8,8 @@ export default test({
 	},
 
 	test({ assert, target }) {
-		const [button1, button2] = target.querySelectorAll('button');
+		const [button1, button2, button3, button4, button5, button6, button7, button8] =
+			target.querySelectorAll('button');
 
 		assert.throws(() => {
 			button1?.click();
@@ -19,5 +20,35 @@ export default test({
 			button2?.click();
 			flushSync();
 		});
+
+		assert.throws(() => {
+			button3?.click();
+			flushSync();
+		}, /state_unsafe_mutation/);
+
+		assert.doesNotThrow(() => {
+			button4?.click();
+			flushSync();
+		});
+
+		assert.throws(() => {
+			button5?.click();
+			flushSync();
+		}, /state_unsafe_mutation/);
+
+		assert.doesNotThrow(() => {
+			button6?.click();
+			flushSync();
+		});
+
+		assert.throws(() => {
+			button7?.click();
+			flushSync();
+		}, /state_unsafe_mutation/);
+
+		assert.doesNotThrow(() => {
+			button8?.click();
+			flushSync();
+		});
 	}
 });

packages/svelte/tests/runtime-runes/samples/side-effect-derived-map/main.svelte

@@ -1,27 +1,101 @@
 <script>
 	import { SvelteMap } from 'svelte/reactivity';
 
-	let visibleExternal = $state(false);
-	let external = new SvelteMap();
-	const throws = $derived.by(() => {
-		external.set(1, 1);
-		return external;
+	let outside_basic = $state(false);
+	let outside_basic_map = new SvelteMap();
+	const throw_basic = $derived.by(() => {
+		outside_basic_map.set(1, 1);
+		return outside_basic_map;
 	});
 
-	let visibleInternal = $state(false);
-	const works = $derived.by(() => {
-		let internal = new SvelteMap();
-		internal.set(1, 1);
-		return internal;
+	let inside_basic = $state(false);
+	const works_basic = $derived.by(() => {
+		let inside = new SvelteMap();
+		inside.set(1, 1);
+		return inside;
+	});
+
+	let outside_has = $state(false);
+	let outside_has_map = new SvelteMap([[1, 1]]);
+	const throw_has = $derived.by(() => {
+		outside_has_map.has(1);
+		outside_has_map.set(1, 2);
+		return outside_has_map;
+	});
+
+	let inside_has = $state(false);
+	const works_has = $derived.by(() => {
+		let inside = new SvelteMap([[1, 1]]);
+		inside.has(1);
+		inside.set(1, 1);
+		return inside;
+	});
+
+	let outside_get = $state(false);
+	let outside_get_map = new SvelteMap([[1, 1]]);
+	const throw_get = $derived.by(() => {
+		outside_get_map.get(1);
+		outside_get_map.set(1, 2);
+		return outside_get_map;
+	});
+
+	let inside_get = $state(false);
+	const works_get = $derived.by(() => {
+		let inside = new SvelteMap([[1, 1]]);
+		inside.get(1);
+		inside.set(1, 1);
+		return inside;
+	});
+
+	let outside_values = $state(false);
+	let outside_values_map = new SvelteMap([[1, 1]]);
+	const throw_values = $derived.by(() => {
+		outside_values_map.values(1);
+		outside_values_map.set(1, 2);
+		return outside_values_map;
+	});
+
+	let inside_values = $state(false);
+	const works_values = $derived.by(() => {
+		let inside = new SvelteMap([[1, 1]]);
+		inside.values();
+		inside.set(1, 1);
+		return inside;
 	});
 </script>
 
-<button onclick={() => (visibleExternal = true)}>external</button>
-{#if visibleExternal}
-	{throws}
+<button onclick={() => (outside_basic = true)}>external</button>
+{#if outside_basic}
+	{throw_basic}
+{/if}
+<button onclick={() => (inside_basic = true)}>internal</button>
+{#if inside_basic}
+	{works_basic}
+{/if}
+
+<button onclick={() => (outside_has = true)}>external</button>
+{#if outside_has}
+	{throw_has}
 {/if}
-<button onclick={() => (visibleInternal = true)}>internal</button>
-{#if visibleInternal}
-	{works}
+<button onclick={() => (inside_has = true)}>internal</button>
+{#if inside_has}
+	{works_has}
 {/if}
 
+<button onclick={() => (outside_get = true)}>external</button>
+{#if outside_get}
+	{throw_get}
+{/if}
+<button onclick={() => (inside_get = true)}>internal</button>
+{#if inside_get}
+	{works_get}
+{/if}
+
+<button onclick={() => (outside_values = true)}>external</button>
+{#if outside_values}
+	{throw_values}
+{/if}
+<button onclick={() => (inside_values = true)}>internal</button>
+{#if inside_values}
+	{works_values}
+{/if}

packages/svelte/tests/runtime-runes/samples/side-effect-derived-set/_config.js

@@ -8,7 +8,7 @@ export default test({
 	},
 
 	test({ assert, target }) {
-		const [button1, button2] = target.querySelectorAll('button');
+		const [button1, button2, button3, button4] = target.querySelectorAll('button');
 
 		assert.throws(() => {
 			button1?.click();
@@ -19,5 +19,15 @@ export default test({
 			button2?.click();
 			flushSync();
 		});
+
+		assert.throws(() => {
+			button3?.click();
+			flushSync();
+		}, /state_unsafe_mutation/);
+
+		assert.doesNotThrow(() => {
+			button4?.click();
+			flushSync();
+		});
 	}
 });