fix: guard contents updated before the guard itself (#16930)

Fixes #16850, fixes #16775, fixes #16795, fixes #16982

#16631 introduced a bug that results in the effects within guards being evaluated before the guards themselves. This fix makes sure to iterate the block effects in the correct order (top down)

---------

Co-authored-by: Simon H <[email protected]>
Co-authored-by: Simon Holthausen <[email protected]>

Author: hmnd

.changeset/witty-seas-learn.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure guards (eg. if, each, key) run before their contents

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

@@ -561,7 +561,7 @@ function infinite_loop_guard() {
 	}
 }
 
-/** @type {Effect[] | null} */
+/** @type {Set<Effect> | null} */
 export let eager_block_effects = null;
 
 /**
@@ -578,7 +578,7 @@ function flush_queued_effects(effects) {
 		var effect = effects[i++];
 
 		if ((effect.f & (DESTROYED | INERT)) === 0 && is_dirty(effect)) {
-			eager_block_effects = [];
+			eager_block_effects = new Set();
 
 			update_effect(effect);
 
@@ -601,15 +601,34 @@ function flush_queued_effects(effects) {
 
 			// If update_effect() has a flushSync() in it, we may have flushed another flush_queued_effects(),
 			// which already handled this logic and did set eager_block_effects to null.
-			if (eager_block_effects?.length > 0) {
-				// TODO this feels incorrect! it gets the tests passing
+			if (eager_block_effects?.size > 0) {
 				old_values.clear();
 
 				for (const e of eager_block_effects) {
-					update_effect(e);
+					// Skip eager effects that have already been unmounted
+					if ((e.f & (DESTROYED | INERT)) !== 0) continue;
+
+					// Run effects in order from ancestor to descendant, else we could run into nullpointers
+					/** @type {Effect[]} */
+					const ordered_effects = [e];
+					let ancestor = e.parent;
+					while (ancestor !== null) {
+						if (eager_block_effects.has(ancestor)) {
+							eager_block_effects.delete(ancestor);
+							ordered_effects.push(ancestor);
+						}
+						ancestor = ancestor.parent;
+					}
+
+					for (let j = ordered_effects.length - 1; j >= 0; j--) {
+						const e = ordered_effects[j];
+						// Skip eager effects that have already been unmounted
+						if ((e.f & (DESTROYED | INERT)) !== 0) continue;
+						update_effect(e);
+					}
 				}
 
-				eager_block_effects = [];
+				eager_block_effects.clear();
 			}
 		}
 	}

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

@@ -336,7 +336,7 @@ function mark_reactions(signal, status) {
 		} else if (not_dirty) {
 			if ((flags & BLOCK_EFFECT) !== 0) {
 				if (eager_block_effects !== null) {
-					eager_block_effects.push(/** @type {Effect} */ (reaction));
+					eager_block_effects.add(/** @type {Effect} */ (reaction));
 				}
 			}
 

packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js

@@ -0,0 +1,20 @@
+import { test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	mode: ['client'],
+	async test({ target, assert, logs }) {
+		const button = target.querySelector('button');
+
+		button?.click();
+		flushSync();
+		button?.click();
+		flushSync();
+		button?.click();
+		flushSync();
+		button?.click();
+		flushSync();
+
+		assert.deepEqual(logs, ['two', 'one', 'two', 'one', 'two']);
+	}
+});

packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte

@@ -0,0 +1,18 @@
+<script lang="ts">
+	let b = $state(false);
+	let v = $state("two");
+
+	$effect(() => {
+		v = b ? "one" : "two";
+	})
+</script>
+
+<button onclick={() => b = !b}>Trigger</button>
+
+{#if v === "one"}
+	<div>if1 matched! {console.log('one')}</div>
+{:else if v === "two"}
+	<div>if2 matched! {console.log('two')}</div>
+{:else}
+	<div>nothing matched {console.log('else')}</div>
+{/if}

packages/svelte/tests/runtime-runes/samples/guard-if-nested/_config.js

@@ -0,0 +1,13 @@
+import { test } from '../../test';
+import { flushSync } from 'svelte';
+
+export default test({
+	mode: ['client'],
+	async test({ target, assert }) {
+		const button = target.querySelector('button');
+
+		flushSync(() => button?.click());
+
+		assert.equal(target.textContent?.trim(), 'Trigger');
+	}
+});

packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte

@@ -0,0 +1,18 @@
+<script>
+	let centerRow = $state({ nested: { optional: 2, required: 3 } });
+
+	let someChange = $state(false);
+	$effect(() => {
+		if (someChange) centerRow = undefined;
+	});
+</script>
+
+{#if centerRow?.nested}
+	{#if centerRow?.nested?.optional != undefined && centerRow.nested.optional > 0}
+		op: {centerRow.nested.optional}<br />
+	{:else}
+		req: {centerRow.nested.required}<br />
+	{/if}
+{/if}
+
+<button onclick={() => (someChange = true)}>Trigger</button>

packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/Component.svelte

@@ -0,0 +1,6 @@
+<script>
+	let { p } = $props();
+	$effect.pre(() => {
+		console.log('running ' + p)
+	})
+</script>

packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/_config.js

@@ -0,0 +1,13 @@
+import { flushSync } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	mode: ['client'],
+	async test({ assert, target, logs }) {
+		const button = target.querySelector('button');
+
+		button?.click();
+		flushSync();
+		assert.deepEqual(logs, ['pre', 'running b', 'pre', 'pre']);
+	}
+});

packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/main.svelte

@@ -0,0 +1,18 @@
+<script>
+	import Component from './Component.svelte';
+
+	let p = $state('b');
+
+	$effect.pre(() => {
+		console.log('pre')
+		if (p === 'a') p = null;
+	})
+</script>
+
+{#if p || !p}
+	{#if p}
+		<Component {p} />
+	{/if}
+{/if}
+
+<button onclick={() => p = 'a'}>a</button>