fix: run effect roots in tree order (#15446)

* process effect roots in tree order

* bring test over

* add test

* changeset

* tidy

Author: Rich-Harris

.changeset/shy-falcons-occur.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: run effect roots in tree order

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

@@ -82,13 +82,12 @@ function push_effect(effect, parent_effect) {
  * @returns {Effect}
  */
 function create_effect(type, fn, sync, push = true) {
-	var is_root = (type & ROOT_EFFECT) !== 0;
-	var parent_effect = active_effect;
+	var parent = active_effect;
 
 	if (DEV) {
 		// Ensure the parent is never an inspect effect
-		while (parent_effect !== null && (parent_effect.f & INSPECT_EFFECT) !== 0) {
-			parent_effect = parent_effect.parent;
+		while (parent !== null && (parent.f & INSPECT_EFFECT) !== 0) {
+			parent = parent.parent;
 		}
 	}
 
@@ -103,7 +102,7 @@ function create_effect(type, fn, sync, push = true) {
 		fn,
 		last: null,
 		next: null,
-		parent: is_root ? null : parent_effect,
+		parent,
 		prev: null,
 		teardown: null,
 		transitions: null,
@@ -136,9 +135,9 @@ function create_effect(type, fn, sync, push = true) {
 		effect.teardown === null &&
 		(effect.f & (EFFECT_HAS_DERIVED | BOUNDARY_EFFECT)) === 0;
 
-	if (!inert && !is_root && push) {
-		if (parent_effect !== null) {
-			push_effect(effect, parent_effect);
+	if (!inert && push) {
+		if (parent !== null) {
+			push_effect(effect, parent);
 		}
 
 		// if we're in a derived, add the effect there too
@@ -391,7 +390,14 @@ export function destroy_effect_children(signal, remove_dom = false) {
 
 	while (effect !== null) {
 		var next = effect.next;
-		destroy_effect(effect, remove_dom);
+
+		if ((effect.f & ROOT_EFFECT) !== 0) {
+			// this is now an independent root
+			effect.parent = null;
+		} else {
+			destroy_effect(effect, remove_dom);
+		}
+
 		effect = next;
 	}
 }

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

@@ -661,13 +661,7 @@ function flush_queued_root_effects() {
 			queued_root_effects = [];
 
 			for (var i = 0; i < length; i++) {
-				var root = root_effects[i];
-
-				if ((root.f & CLEAN) === 0) {
-					root.f ^= CLEAN;
-				}
-
-				var collected_effects = process_effects(root);
+				var collected_effects = process_effects(root_effects[i]);
 				flush_queued_effects(collected_effects);
 			}
 		}
@@ -759,11 +753,12 @@ function process_effects(root) {
 	/** @type {Effect[]} */
 	var effects = [];
 
-	var effect = root.first;
+	/** @type {Effect | null} */
+	var effect = root;
 
 	while (effect !== null) {
 		var flags = effect.f;
-		var is_branch = (flags & BRANCH_EFFECT) !== 0;
+		var is_branch = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) !== 0;
 		var is_skippable_branch = is_branch && (flags & CLEAN) !== 0;
 
 		if (!is_skippable_branch && (flags & INERT) === 0) {
@@ -788,6 +783,7 @@ function process_effects(root) {
 				}
 			}
 
+			/** @type {Effect | null} */
 			var child = effect.first;
 
 			if (child !== null) {

packages/svelte/tests/runtime-runes/samples/effect-root-5/_config.js

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

packages/svelte/tests/runtime-runes/samples/effect-root-5/main.svelte

@@ -0,0 +1,23 @@
+<script>
+	let obj = $state({ count: 0 });
+
+	$effect.root(() => {
+		let teardown;
+
+		$effect.pre(() => {
+			if (obj) {
+				teardown ??= $effect.root(() => {
+					$effect.pre(() => {
+						console.log(obj.count);
+					});
+				});
+			} else {
+				teardown?.();
+				teardown = null;
+			}
+		});
+	});
+</script>
+
+<button onclick={() => ((obj ??= { count: 0 }).count += 1)}>+1</button>
+<button onclick={() => (obj = null)}>null</button>

packages/svelte/tests/runtime-runes/samples/toStore-teardown/_config.js

@@ -0,0 +1,13 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, target }) {
+		let [, btn2] = target.querySelectorAll('button');
+
+		btn2.click();
+		flushSync();
+
+		assert.htmlEqual(target.innerHTML, `<button>Set data</button><button>Clear data</button>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/toStore-teardown/child.svelte

@@ -0,0 +1,11 @@
+<script>
+	import { toStore } from 'svelte/store'
+
+	let { data } = $props()
+	const currentValue = toStore(() => data.value)
+</script>
+
+<p>
+	Current value:
+	<span>{$currentValue}</span>
+</p>

packages/svelte/tests/runtime-runes/samples/toStore-teardown/main.svelte

@@ -0,0 +1,15 @@
+<script>
+	import Child from './child.svelte'
+
+	let data = $state({ value: 'hello' });
+
+	const setData = () => (data = { value: 'hello' })
+	const clearData = () => (data = undefined)
+</script>
+
+<button onclick={setData}>Set data</button>
+<button onclick={clearData}>Clear data</button>
+
+{#if data}
+  <Child {data} />
+{/if}