From 36ccef80bf7abd9896ce7572d12d5b5cddd22f1d Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Thu, 9 Oct 2025 01:29:15 -0700 Subject: [PATCH 01/12] test: guard effect regression samples --- .../samples/guard-else-effect/_config.js | 22 ++++++++++++++++ .../samples/guard-else-effect/main.svelte | 25 +++++++++++++++++++ .../samples/guard-if-nested/_config.js | 13 ++++++++++ .../samples/guard-if-nested/main.svelte | 19 ++++++++++++++ 4 files changed, 79 insertions(+) create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-if-nested/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js new file mode 100644 index 000000000000..c8a8e1078e07 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js @@ -0,0 +1,22 @@ +import { expect, vi } from 'vitest'; +import { test } from '../../test'; +import { flushSync } from 'svelte'; + +const trackBranch = vi.fn(); + +export default test({ + mode: ['client'], + props: { trackBranch: trackBranch }, + async test({ target }) { + const button = target.querySelector('button'); + + flushSync(() => button?.click()); + flushSync(() => button?.click()); + flushSync(() => button?.click()); + flushSync(() => button?.click()); + + expect(trackBranch).toHaveBeenCalledWith('one'); + expect(trackBranch).toHaveBeenCalledWith('two'); + expect(trackBranch).not.toHaveBeenCalledWith('else'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte new file mode 100644 index 000000000000..3f75004decab --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte @@ -0,0 +1,25 @@ + + + + + + +{#if v === "one"} +
if1 matched! {trackBranch('one')}
+{:else if v === "two"} +
if2 matched! {trackBranch('two')}
+{:else} +
nothing matched {trackBranch('else')}
+{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/guard-if-nested/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-if-nested/_config.js new file mode 100644 index 000000000000..881c1545ee9f --- /dev/null +++ b/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'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte new file mode 100644 index 000000000000..c0528680c029 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte @@ -0,0 +1,19 @@ + + + {#if centerRow?.nested} + {#if centerRow?.nested?.optional != undefined && centerRow.nested.optional > 0} + op: {centerRow.nested.optional}
+ {:else} + req: {centerRow.nested.required}
+ {/if} + {/if} + + From 7b029d51e21379f36ad4a24052e121c874fff06c Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Fri, 17 Oct 2025 16:10:57 -0700 Subject: [PATCH 02/12] fix: skip destroyed eager effects and execute in order of depth --- .../src/internal/client/reactivity/batch.js | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index fd2a6d9f5dcf..8bc081989389 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -601,15 +601,32 @@ 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 old_values.clear(); + /** @type {Array<{ effect: Effect; depth: number }>} */ + const effects_with_depth = []; 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; + + let depth = 0; + let ancestor = e.parent; + while (ancestor !== null) { + depth++; + ancestor = ancestor.parent; + } + + effects_with_depth.push({ effect: e, depth }); } - eager_block_effects = []; + effects_with_depth.sort((a, b) => a.depth - b.depth); + + for (const { effect } of effects_with_depth) { + update_effect(effect); + } } + + eager_block_effects = []; } } From 140ee11f0b14496f1c14c2a05ec18b67d3cfd15f Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Sat, 11 Oct 2025 02:16:07 -0700 Subject: [PATCH 03/12] add changeset Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com> --- .changeset/witty-seas-learn.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/witty-seas-learn.md diff --git a/.changeset/witty-seas-learn.md b/.changeset/witty-seas-learn.md new file mode 100644 index 000000000000..aa94c7c35f36 --- /dev/null +++ b/.changeset/witty-seas-learn.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure guards (eg. if, each, key) run before their contents From 73867b953299110489dcd5e26aabba6326681694 Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Sun, 19 Oct 2025 15:03:07 -0700 Subject: [PATCH 04/12] use bucket sort for better perf --- .../src/internal/client/reactivity/batch.js | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 8bc081989389..9ce2204dd36f 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -603,8 +603,10 @@ function flush_queued_effects(effects) { if (eager_block_effects?.length > 0) { old_values.clear(); - /** @type {Array<{ effect: Effect; depth: number }>} */ - const effects_with_depth = []; + let max_depth = 0; + /** @type {Effect[][]} */ + const depth_buckets = []; + for (const e of eager_block_effects) { // Skip eager effects that have already been unmounted if ((e.f & (DESTROYED | INERT)) !== 0) continue; @@ -616,13 +618,17 @@ function flush_queued_effects(effects) { ancestor = ancestor.parent; } - effects_with_depth.push({ effect: e, depth }); + if (depth > max_depth) max_depth = depth; + (depth_buckets[depth] ??= []).push(e); } - effects_with_depth.sort((a, b) => a.depth - b.depth); - - for (const { effect } of effects_with_depth) { - update_effect(effect); + for (let depth = 0; depth <= max_depth; depth++) { + const effects = depth_buckets[depth]; + if (effects) { + for (const e of effects) { + update_effect(e); + } + } } } From e50d2b9a550e7608b6fabfd322e53fa459049af0 Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Sun, 19 Oct 2025 15:32:18 -0700 Subject: [PATCH 05/12] simplify --- packages/svelte/src/internal/client/reactivity/batch.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 9ce2204dd36f..c78d2b4b3ef9 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -603,7 +603,6 @@ function flush_queued_effects(effects) { if (eager_block_effects?.length > 0) { old_values.clear(); - let max_depth = 0; /** @type {Effect[][]} */ const depth_buckets = []; @@ -618,12 +617,10 @@ function flush_queued_effects(effects) { ancestor = ancestor.parent; } - if (depth > max_depth) max_depth = depth; (depth_buckets[depth] ??= []).push(e); } - for (let depth = 0; depth <= max_depth; depth++) { - const effects = depth_buckets[depth]; + for (const effects of depth_buckets) { if (effects) { for (const e of effects) { update_effect(e); From cd350960bb99a966add4c6b23eea6441d66212f6 Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Sun, 19 Oct 2025 17:25:17 -0700 Subject: [PATCH 06/12] instead of sorting, skip eager effects with eager ancestors in same flush --- .../src/internal/client/reactivity/batch.js | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index c78d2b4b3ef9..d72dc13f6547 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -603,33 +603,38 @@ function flush_queued_effects(effects) { if (eager_block_effects?.length > 0) { old_values.clear(); - /** @type {Effect[][]} */ - const depth_buckets = []; + /** @type {Effect[]} */ + const filtered_effects = []; for (const e of eager_block_effects) { // Skip eager effects that have already been unmounted if ((e.f & (DESTROYED | INERT)) !== 0) continue; - let depth = 0; + // Skip effects that have related parent effects in the current flush, + // as the inner ones will be run if its parent triggers it (eg. in a guard). + let skip = false; let ancestor = e.parent; - while (ancestor !== null) { - depth++; + while (!skip && ancestor !== null) { + if (eager_block_effects.includes(ancestor)) { + skip = true; + break; + } ancestor = ancestor.parent; } + if (skip) { + unlink_effect(e); + continue; + } - (depth_buckets[depth] ??= []).push(e); + filtered_effects.push(e); } - for (const effects of depth_buckets) { - if (effects) { - for (const e of effects) { - update_effect(e); - } - } + for (const e of filtered_effects) { + update_effect(e); } - } - eager_block_effects = []; + eager_block_effects = []; + } } } From c67e58b65c500171ab2128a3ba66712ff5a3baa4 Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Mon, 20 Oct 2025 01:26:01 -0700 Subject: [PATCH 07/12] don't unlink skipped eager effects this was causing some $derived.by's to not run at all... --- packages/svelte/src/internal/client/reactivity/batch.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index d72dc13f6547..17830416b7c3 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -622,7 +622,6 @@ function flush_queued_effects(effects) { ancestor = ancestor.parent; } if (skip) { - unlink_effect(e); continue; } From 749a05614bd93322ab64915b41f16b06551e8e8c Mon Sep 17 00:00:00 2001 From: David Roizenman Date: Mon, 20 Oct 2025 02:30:49 -0700 Subject: [PATCH 08/12] use set for less costly parent lookup --- .../svelte/src/internal/client/reactivity/batch.js | 10 +++++----- .../svelte/src/internal/client/reactivity/sources.js | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 17830416b7c3..6b7b115a61b6 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -560,7 +560,7 @@ function infinite_loop_guard() { } } -/** @type {Effect[] | null} */ +/** @type {Set | null} */ export let eager_block_effects = null; /** @@ -577,7 +577,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); @@ -600,7 +600,7 @@ 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) { + if (eager_block_effects?.size > 0) { old_values.clear(); /** @type {Effect[]} */ @@ -615,7 +615,7 @@ function flush_queued_effects(effects) { let skip = false; let ancestor = e.parent; while (!skip && ancestor !== null) { - if (eager_block_effects.includes(ancestor)) { + if (eager_block_effects.has(ancestor)) { skip = true; break; } @@ -632,7 +632,7 @@ function flush_queued_effects(effects) { update_effect(e); } - eager_block_effects = []; + eager_block_effects.clear(); } } } diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index cd0c28016dc5..c5dcff9cfbdd 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/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)); } } From bed73acc2b14b5587aa9d720f7ce3dc23992f786 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 20 Oct 2025 23:18:30 +0200 Subject: [PATCH 09/12] tidy --- .../src/internal/client/reactivity/batch.js | 11 +++-------- .../samples/guard-else-effect/main.svelte | 7 +------ .../samples/guard-if-nested/main.svelte | 15 +++++++-------- 3 files changed, 11 insertions(+), 22 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 2a7badadcb29..dd647b544d54 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -607,24 +607,19 @@ function flush_queued_effects(effects) { /** @type {Effect[]} */ const filtered_effects = []; - for (const e of eager_block_effects) { + outer: for (const e of eager_block_effects) { // Skip eager effects that have already been unmounted if ((e.f & (DESTROYED | INERT)) !== 0) continue; // Skip effects that have related parent effects in the current flush, // as the inner ones will be run if its parent triggers it (eg. in a guard). - let skip = false; let ancestor = e.parent; - while (!skip && ancestor !== null) { + while (ancestor !== null) { if (eager_block_effects.has(ancestor)) { - skip = true; - break; + continue outer; } ancestor = ancestor.parent; } - if (skip) { - continue; - } filtered_effects.push(e); } diff --git a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte index 3f75004decab..62520ca197ef 100644 --- a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte @@ -1,21 +1,16 @@ - - {#if v === "one"}
if1 matched! {trackBranch('one')}
{:else if v === "two"} diff --git a/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte index c0528680c029..4514bd114e74 100644 --- a/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/guard-if-nested/main.svelte @@ -1,6 +1,5 @@ - {#if centerRow?.nested} - {#if centerRow?.nested?.optional != undefined && centerRow.nested.optional > 0} - op: {centerRow.nested.optional}
- {:else} - req: {centerRow.nested.required}
- {/if} +{#if centerRow?.nested} + {#if centerRow?.nested?.optional != undefined && centerRow.nested.optional > 0} + op: {centerRow.nested.optional}
+ {:else} + req: {centerRow.nested.required}
{/if} +{/if} From 3dac3e718b661463273f15ff5ab1e2fa90c473de Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 21 Oct 2025 00:05:45 +0200 Subject: [PATCH 10/12] fix --- .../src/internal/client/reactivity/batch.js | 23 ++++++++++--------- .../guard-nested-if-pre/Component.svelte | 6 +++++ .../samples/guard-nested-if-pre/_config.js | 13 +++++++++++ .../samples/guard-nested-if-pre/main.svelte | 18 +++++++++++++++ 4 files changed, 49 insertions(+), 11 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/Component.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/main.svelte diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index dd647b544d54..6c60c4fdee56 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -604,28 +604,29 @@ function flush_queued_effects(effects) { if (eager_block_effects?.size > 0) { old_values.clear(); - /** @type {Effect[]} */ - const filtered_effects = []; - - outer: for (const e of eager_block_effects) { + for (const e of eager_block_effects) { // Skip eager effects that have already been unmounted if ((e.f & (DESTROYED | INERT)) !== 0) continue; - // Skip effects that have related parent effects in the current flush, - // as the inner ones will be run if its parent triggers it (eg. in a guard). + // Run effects in order from ancestor to descendant, else we could run into nullpointers + /** @type {Effect[]} */ + const ordered_effects = []; let ancestor = e.parent; while (ancestor !== null) { if (eager_block_effects.has(ancestor)) { - continue outer; + eager_block_effects.delete(ancestor); + ordered_effects.unshift(ancestor); } ancestor = ancestor.parent; } - filtered_effects.push(e); - } + ordered_effects.push(e); - for (const e of filtered_effects) { - update_effect(e); + for (const e of ordered_effects) { + // Skip eager effects that have already been unmounted + if ((e.f & (DESTROYED | INERT)) !== 0) continue; + update_effect(e); + } } eager_block_effects.clear(); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/Component.svelte b/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/Component.svelte new file mode 100644 index 000000000000..b7322e75309f --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/Component.svelte @@ -0,0 +1,6 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/_config.js new file mode 100644 index 000000000000..9706855fb436 --- /dev/null +++ b/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']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/main.svelte new file mode 100644 index 000000000000..4ebb13eca396 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/guard-nested-if-pre/main.svelte @@ -0,0 +1,18 @@ + + +{#if p || !p} + {#if p} + + {/if} +{/if} + + From 739b51140cf9847f6ebcc811c402936c6a124ebe Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 21 Oct 2025 00:08:54 +0200 Subject: [PATCH 11/12] tweak test --- .../samples/guard-else-effect/_config.js | 22 +++++++++---------- .../samples/guard-else-effect/main.svelte | 8 +++---- 2 files changed, 13 insertions(+), 17 deletions(-) diff --git a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js index c8a8e1078e07..4e8eec8b1670 100644 --- a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/_config.js @@ -1,22 +1,20 @@ -import { expect, vi } from 'vitest'; import { test } from '../../test'; import { flushSync } from 'svelte'; -const trackBranch = vi.fn(); - export default test({ mode: ['client'], - props: { trackBranch: trackBranch }, - async test({ target }) { + async test({ target, assert, logs }) { const button = target.querySelector('button'); - flushSync(() => button?.click()); - flushSync(() => button?.click()); - flushSync(() => button?.click()); - flushSync(() => button?.click()); + button?.click(); + flushSync(); + button?.click(); + flushSync(); + button?.click(); + flushSync(); + button?.click(); + flushSync(); - expect(trackBranch).toHaveBeenCalledWith('one'); - expect(trackBranch).toHaveBeenCalledWith('two'); - expect(trackBranch).not.toHaveBeenCalledWith('else'); + assert.deepEqual(logs, ['two', 'one', 'two', 'one', 'two']); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte index 62520ca197ef..91fd0442bdef 100644 --- a/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/guard-else-effect/main.svelte @@ -1,6 +1,4 @@