fix: derived reactivity and perf regressions (#17362)

* add test sample

* add test for sveltejs/kit#15059

* fix: reconnect deriveds inside branch effects

* add changeset

* fix: derived with no deps always set as MAYBE_DIRTY

fixes #17342

* add test for #17342

* additional changeset

* refactor: extract setting derived status to helper, apply to sources.js

* add test case for #17352

* fix: reconnect child deriveds when evaluating connected parent derived

fixes #17352

* fix import order causing Cannot read properties of undefined on dev load

* remove duplicate iteration over deps

* minor style tweaks

* oops, fix merge

* use update_derived_status, so that we never set a dep-less derived MAYBE_DIRTY

* tweak

* reaction.deps cannot be null for a MAYBE_DIRTY derived

* make it such that reactions without deps are never MAYBE_DIRTY

* since we no longer need to check reaction.deps === null, we can revert this bit

* more explicit check

* tidy up

* more

* gah whoops

* move import

* simplify test

* make dep-less derived behaviour more explicit, move it above is_destroying_effect handling

* remove test - this is adequately covered by #17445

* replace tricky unit test with component-based test

* remove incorrect test

* remove the BRANCH_EFFECT stuff

* tidy up

* DRY

* tweak, add explanatory comment

* tweak

* explanatory comment

* remove changeset

* update changeset

---------

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

Author: 3 people

.changeset/beige-sloths-cry.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: reconnect clean deriveds when they are read in a reactive context

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

@@ -620,15 +620,27 @@ export function get(signal) {
 			return value;
 		}
 
-		// TODO this should probably just be `!batch_values?.has(derived)` — the second bit
-		// should be taken care of by clearing `batch_values` in `mark_reactions`?
-		if (!batch_values?.has(derived) || (current_batch?.is_fork && !effect_tracking())) {
-			if (is_dirty(derived)) {
-				update_derived(derived);
+		// connect disconnected deriveds if we are reading them inside an effect,
+		// or inside another derived that is already connected
+		var should_connect =
+			(derived.f & CONNECTED) === 0 &&
+			!untracking &&
+			active_reaction !== null &&
+			(is_updating_effect || (active_reaction.f & CONNECTED) !== 0);
+
+		var is_new = derived.deps === null;
+
+		if (is_dirty(derived)) {
+			if (should_connect) {
+				// set the flag before `update_derived`, so that the derived
+				// is added as a reaction to its dependencies
+				derived.f |= CONNECTED;
 			}
+
+			update_derived(derived);
 		}
 
-		if (is_updating_effect && effect_tracking() && (derived.f & CONNECTED) === 0) {
+		if (should_connect && !is_new) {
 			reconnect(derived);
 		}
 	}
@@ -652,7 +664,7 @@ export function get(signal) {
 function reconnect(derived) {
 	if (derived.deps === null) return;
 
-	derived.f ^= CONNECTED;
+	derived.f |= CONNECTED;
 
 	for (const dep of derived.deps) {
 		(dep.reactions ??= []).push(derived);

packages/svelte/tests/runtime-runes/samples/derived-read-outside-reaction/_config.js

@@ -0,0 +1,47 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	html: `
+		<button>+1</button>
+		<button>add number</button>
+		<p>1, 2, 3</p>
+	`,
+
+	test({ assert, target }) {
+		const [button1, button2] = target.querySelectorAll('button');
+
+		button1.click();
+		flushSync();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>+1</button>
+				<button>add number</button>
+				<p>2, 4, 6</p>
+			`
+		);
+
+		button2.click();
+		flushSync();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>+1</button>
+				<button>add number</button>
+				<p>2, 4, 6, 8</p>
+			`
+		);
+
+		button1.click();
+		flushSync();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>+1</button>
+				<button>add number</button>
+				<p>3, 6, 9, 12</p>
+			`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/derived-read-outside-reaction/main.svelte

@@ -0,0 +1,30 @@
+<script lang="ts">
+	class Item {
+		product: number;
+
+		constructor(n: number) {
+			this.product = $derived(multiplier * n);
+		}
+	}
+
+	let numbers = $state([1, 2, 3]);
+	let multiplier = $state(1);
+
+	let items = $derived(numbers.map((n) => new Item(n)))
+	let products = $derived(items.map(item => item.product));
+</script>
+
+<button onclick={() => {
+	multiplier += 1;
+}}>+1</button>
+
+<button onclick={() => {
+	numbers.push(numbers.length + 1);
+
+	// this is load-bearing — by reading it outside a reaction, we recompute
+	// `products`, removing it as a reaction from `Item.product` dependencies,
+	// but we don't add it as a reaction to the new `Item.product` dependencies
+	products;
+}}>add number</button>
+
+<p>{products.join(', ')}</p>