fix: when re-connecting unowned deriveds, remove their unowned flag (#15255)

* fix: when an unowned derived is tracked again, remove unowned flag

* fix: when an unowned derived is tracked again, remove unowned flag

* test case

* test case

* cleanup unused logic

* simplify

* simplify

* simplify

* simplify

* add test

* fix other issue

* back to original plan

* Apply suggestions from code review

---------

Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>

.changeset/nasty-pigs-lay.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: when re-connecting unowned deriveds, remove their unowned flag

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

@@ -1,18 +1,9 @@
 /** @import { Derived, Effect } from '#client' */
 import { DEV } from 'esm-env';
-import {
-	CLEAN,
-	DERIVED,
-	DESTROYED,
-	DIRTY,
-	EFFECT_HAS_DERIVED,
-	MAYBE_DIRTY,
-	UNOWNED
-} from '../constants.js';
+import { CLEAN, DERIVED, DIRTY, EFFECT_HAS_DERIVED, MAYBE_DIRTY, UNOWNED } from '../constants.js';
 import {
 	active_reaction,
 	active_effect,
-	remove_reactions,
 	set_signal_status,
 	skip_reaction,
 	update_reaction,

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

@@ -10,7 +10,15 @@ 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 } from '../runtime.js';
+import {
+	active_effect,
+	get,
+	captured_signals,
+	set_active_effect,
+	untrack,
+	active_reaction,
+	set_active_reaction
+} from '../runtime.js';
 import { safe_equals } from './equality.js';
 import * as e from '../errors.js';
 import {
@@ -241,26 +249,6 @@ export function spread_props(...props) {
 	return new Proxy({ props }, spread_props_handler);
 }
 
-/**
- * @template T
- * @param {() => T} fn
- * @returns {T}
- */
-function with_parent_branch(fn) {
-	var effect = active_effect;
-	var previous_effect = active_effect;
-
-	while (effect !== null && (effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0) {
-		effect = effect.parent;
-	}
-	try {
-		set_active_effect(effect);
-		return fn();
-	} finally {
-		set_active_effect(previous_effect);
-	}
-}
-
 /**
  * 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.
@@ -335,8 +323,8 @@ export function prop(props, key, flags, fallback) {
 	} else {
 		// Svelte 4 did not trigger updates when a primitive value was updated to the same value.
 		// Replicate that behavior through using a derived
-		var derived_getter = with_parent_branch(() =>
-			(immutable ? derived : derived_safe_equal)(() => /** @type {V} */ (props[key]))
+		var derived_getter = (immutable ? derived : derived_safe_equal)(
+			() => /** @type {V} */ (props[key])
 		);
 		derived_getter.f |= LEGACY_DERIVED_PROP;
 		getter = () => {
@@ -380,21 +368,19 @@ export function prop(props, key, flags, fallback) {
 	// The derived returns the current value. The underlying mutable
 	// source is written to from various places to persist this value.
 	var inner_current_value = mutable_source(prop_value);
-	var current_value = with_parent_branch(() =>
-		derived(() => {
-			var parent_value = getter();
-			var child_value = get(inner_current_value);
-
-			if (from_child) {
-				from_child = false;
-				was_from_child = true;
-				return child_value;
-			}
+	var current_value = derived(() => {
+		var parent_value = getter();
+		var child_value = get(inner_current_value);
+
+		if (from_child) {
+			from_child = false;
+			was_from_child = true;
+			return child_value;
+		}
 
-			was_from_child = false;
-			return (inner_current_value.v = parent_value);
-		})
-	);
+		was_from_child = false;
+		return (inner_current_value.v = parent_value);
+	});
 
 	if (!immutable) current_value.equals = safe_equals;
 

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

@@ -184,19 +184,28 @@ export function check_dirtiness(reaction) {
 			// If we are working with a disconnected or an unowned signal that is now connected (due to an active effect)
 			// then we need to re-connect the reaction to the dependency
 			if (is_disconnected || is_unowned_connected) {
+				var derived = /** @type {Derived} */ (reaction);
+				var parent = derived.parent;
+
 				for (i = 0; i < length; i++) {
 					dependency = dependencies[i];
 
 					// We always re-add all reactions (even duplicates) if the derived was
 					// previously disconnected, however we don't if it was unowned as we
 					// de-duplicate dependencies in that case
-					if (is_disconnected || !dependency?.reactions?.includes(reaction)) {
-						(dependency.reactions ??= []).push(reaction);
+					if (is_disconnected || !dependency?.reactions?.includes(derived)) {
+						(dependency.reactions ??= []).push(derived);
 					}
 				}
 
 				if (is_disconnected) {
-					reaction.f ^= DISCONNECTED;
+					derived.f ^= DISCONNECTED;
+				}
+				// If the unowned derived is now fully connected to the graph again (it's unowned and reconnected, has a parent
+				// and the parent is not unowned), then we can mark it as connected again, removing the need for the unowned
+				// flag
+				if (is_unowned_connected && parent !== null && (parent.f & UNOWNED) === 0) {
+					derived.f ^= UNOWNED;
 				}
 			}
 

packages/svelte/tests/runtime-runes/samples/derived-unowned-12/_config.js

@@ -0,0 +1,25 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		let [btn1, btn2] = target.querySelectorAll('button');
+
+		btn1?.click();
+		flushSync();
+
+		btn2?.click();
+		flushSync();
+
+		btn1?.click();
+		flushSync();
+
+		btn1?.click();
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>linked.current</button>\n3\n<button>count</button>\n1`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte

@@ -0,0 +1,18 @@
+<script>
+	import { untrack } from 'svelte';
+
+	let count = $state(0);
+	let state = $state({current: count});
+
+	let linked = $derived.by(() => {
+		count;
+
+		untrack(() => state.current = count);
+		return untrack(() => state);
+	});
+
+	linked.current++;
+</script>
+
+<button onclick={() => linked.current++}>linked.current</button> {linked.current}
+<button onclick={() => count++}>count</button> {count}

packages/svelte/tests/signals/test.ts

@@ -224,20 +224,75 @@ describe('signals', () => {
 		};
 	});
 
-	test('effects correctly handle unowned derived values that do not change', () => {
-		const log: number[] = [];
+	test('https://perf.js.hyoo.ru/#!bench=9h2as6_u0mfnn #2', () => {
+		let res: number[] = [];
 
-		let count = state(0);
-		const read = () => {
-			const x = derived(() => ({ count: $.get(count) }));
-			return $.get(x);
+		const numbers = Array.from({ length: 2 }, (_, i) => i);
+		const fib = (n: number): number => (n < 2 ? 1 : fib(n - 1) + fib(n - 2));
+		const hard = (n: number, l: string) => n + fib(16);
+
+		const A = state(0);
+		const B = state(0);
+
+		return () => {
+			const C = derived(() => ($.get(A) % 2) + ($.get(B) % 2));
+			const D = derived(() => numbers.map((i) => i + ($.get(A) % 2) - ($.get(B) % 2)));
+			const E = derived(() => hard($.get(C) + $.get(A) + $.get(D)[0]!, 'E'));
+			const F = derived(() => hard($.get(D)[0]! && $.get(B), 'F'));
+			const G = derived(() => $.get(C) + ($.get(C) || $.get(E) % 2) + $.get(D)[0]! + $.get(F));
+
+			const destroy = effect_root(() => {
+				effect(() => {
+					res.push(hard($.get(G), 'H'));
+				});
+				effect(() => {
+					res.push($.get(G));
+				});
+				effect(() => {
+					res.push(hard($.get(F), 'J'));
+				});
+			});
+
+			flushSync();
+
+			let i = 2;
+			while (--i) {
+				res.length = 0;
+				set(B, 1);
+				set(A, 1 + i * 2);
+				flushSync();
+
+				set(A, 2 + i * 2);
+				set(B, 2);
+				flushSync();
+
+				assert.equal(res.length, 4);
+				assert.deepEqual(res, [3198, 1601, 3195, 1598]);
+			}
+
+			destroy();
+			assert(A.reactions === null);
+			assert(B.reactions === null);
 		};
-		const derivedCount = derived(() => read().count);
-		user_effect(() => {
-			log.push($.get(derivedCount));
-		});
+	});
+
+	test('effects correctly handle unowned derived values that do not change', () => {
+		const log: number[] = [];
 
 		return () => {
+			let count = state(0);
+			const read = () => {
+				const x = derived(() => ({ count: $.get(count) }));
+				return $.get(x);
+			};
+			const derivedCount = derived(() => read().count);
+
+			const destroy = effect_root(() => {
+				user_effect(() => {
+					log.push($.get(derivedCount));
+				});
+			});
+
 			flushSync(() => set(count, 1));
 			// Ensure we're not leaking consumers
 			assert.deepEqual(count.reactions?.length, 1);
@@ -248,6 +303,8 @@ describe('signals', () => {
 			// Ensure we're not leaking consumers
 			assert.deepEqual(count.reactions?.length, 1);
 			assert.deepEqual(log, [0, 1, 2, 3]);
+
+			destroy();
 		};
 	});
 
@@ -343,25 +400,69 @@ describe('signals', () => {
 		};
 	});
 
-	let some_state = state({});
-	let some_deps = derived(() => {
-		return [$.get(some_state)];
-	});
-
 	test('two effects with an unowned derived that has some dependencies', () => {
 		const log: Array<Array<any>> = [];
 
-		render_effect(() => {
-			log.push($.get(some_deps));
-		});
+		return () => {
+			let some_state = state({});
+			let some_deps = derived(() => {
+				return [$.get(some_state)];
+			});
+			let destroy2: any;
 
-		render_effect(() => {
-			log.push($.get(some_deps));
-		});
+			const destroy = effect_root(() => {
+				render_effect(() => {
+					$.untrack(() => {
+						log.push($.get(some_deps));
+					});
+				});
 
-		return () => {
+				destroy2 = effect_root(() => {
+					render_effect(() => {
+						log.push($.get(some_deps));
+					});
+
+					render_effect(() => {
+						log.push($.get(some_deps));
+					});
+				});
+			});
+
+			set(some_state, {});
+			flushSync();
+
+			assert.deepEqual(log, [[{}], [{}], [{}], [{}], [{}]]);
+
+			destroy2();
+
+			set(some_state, {});
 			flushSync();
-			assert.deepEqual(log, [[{}], [{}]]);
+
+			assert.deepEqual(log, [[{}], [{}], [{}], [{}], [{}]]);
+
+			log.length = 0;
+
+			const destroy3 = effect_root(() => {
+				render_effect(() => {
+					$.untrack(() => {
+						log.push($.get(some_deps));
+					});
+					log.push($.get(some_deps));
+				});
+			});
+
+			set(some_state, {});
+			flushSync();
+
+			assert.deepEqual(log, [[{}], [{}], [{}], [{}]]);
+
+			destroy3();
+
+			assert(some_state.reactions === null);
+
+			destroy();
+
+			assert(some_state.reactions === null);
 		};
 	});