From fd7e8b70cf7d4a861ba0eae072d3c7c27d532b8a Mon Sep 17 00:00:00 2001
From: Dominic Gannaway <trueadm@users.noreply.github.com>
Date: Tue, 10 Sep 2024 13:58:35 +0100
Subject: [PATCH] fix: ensure unowned derived signals correctly re-connect to
 graph (#13184)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes #13174. Turns out that we we skipping this important work because we return true early – thus not connecting the unowned derived back to the reaction.
---
 .changeset/rotten-actors-rush.md              |  5 ++
 .../svelte/src/internal/client/runtime.js     | 22 +++---
 .../samples/derived-unowned-9/_config.js      | 71 +++++++++++++++++++
 .../samples/derived-unowned-9/main.svelte     | 24 +++++++
 4 files changed, 112 insertions(+), 10 deletions(-)
 create mode 100644 .changeset/rotten-actors-rush.md
 create mode 100644 packages/svelte/tests/runtime-runes/samples/derived-unowned-9/_config.js
 create mode 100644 packages/svelte/tests/runtime-runes/samples/derived-unowned-9/main.svelte

diff --git a/.changeset/rotten-actors-rush.md b/.changeset/rotten-actors-rush.md
new file mode 100644
index 0000000000..46f15bdf27
--- /dev/null
+++ b/.changeset/rotten-actors-rush.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure unowned derived signals correctly re-connect to graph
diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
index bd4e387c3d..dc103bbea6 100644
--- a/packages/svelte/src/internal/client/runtime.js
+++ b/packages/svelte/src/internal/client/runtime.js
@@ -196,18 +196,20 @@ export function check_dirtiness(reaction) {
 					update_derived(/** @type {Derived} */ (dependency));
 				}
 
-				if (dependency.version > reaction.version) {
-					return true;
+				// If we are working with an unowned signal as part of an effect (due to !current_skip_reaction)
+				// and the version hasn't changed, we still need to check that this reaction
+				// is linked to the dependency source – otherwise future updates will not be caught.
+				if (
+					is_unowned &&
+					current_effect !== null &&
+					!current_skip_reaction &&
+					!dependency?.reactions?.includes(reaction)
+				) {
+					(dependency.reactions ??= []).push(reaction);
 				}
 
-				if (is_unowned) {
-					// TODO is there a more logical place to do this work?
-					if (!current_skip_reaction && !dependency?.reactions?.includes(reaction)) {
-						// If we are working with an unowned signal as part of an effect (due to !current_skip_reaction)
-						// and the version hasn't changed, we still need to check that this reaction
-						// if linked to the dependency source – otherwise future updates will not be caught.
-						(dependency.reactions ??= []).push(reaction);
-					}
+				if (dependency.version > reaction.version) {
+					return true;
 				}
 			}
 		}
diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-9/_config.js b/packages/svelte/tests/runtime-runes/samples/derived-unowned-9/_config.js
new file mode 100644
index 0000000000..ce3cfa122f
--- /dev/null
+++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-9/_config.js
@@ -0,0 +1,71 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	mode: ['client'],
+	async test({ assert, target, logs }) {
+		let [btn1, btn2] = target.querySelectorAll('button');
+
+		btn1.click();
+		btn2.click();
+
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>a</button><button>b</button><div>1</div>\ndouble:\n2`
+		);
+
+		btn1.click();
+		btn2.click();
+
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>a</button><button>b</button><div>2</div>\ndouble:\n4`
+		);
+
+		btn1.click();
+
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>a</button><button>b</button><div>3</div>\ndouble:\n6`
+		);
+
+		btn1.click();
+
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>a</button><button>b</button><div>4</div>\ndouble:\n8`
+		);
+
+		btn1.click();
+
+		flushSync();
+
+		assert.htmlEqual(target.innerHTML, `<button>a</button><button>b</button><div>5</div>`);
+
+		btn1.click();
+
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>a</button><button>b</button><div>6</div>\ndouble:\n12`
+		);
+
+		btn1.click();
+
+		flushSync();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`<button>a</button><button>b</button><div>7</div>\ndouble:\n14`
+		);
+	}
+});
diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-9/main.svelte b/packages/svelte/tests/runtime-runes/samples/derived-unowned-9/main.svelte
new file mode 100644
index 0000000000..6c624e94e0
--- /dev/null
+++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-9/main.svelte
@@ -0,0 +1,24 @@
+<script>
+	let count = $state(0);
+
+	function increment() {
+		count += 1;
+	}
+
+	let double = $state();
+	function setDerived() {
+		const d = $derived(count * 2);
+		double = {
+			get v() { return d }
+		};
+	}
+</script>
+
+<button onclick={increment}>a</button>
+<button onclick={setDerived}>b</button>
+
+<div>{count}</div>
+
+{#if double && count % 5}
+	double: {double.v}
+{/if}