From 7717ba01b42625f2022b7034063a25d8507560d7 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 2 Mar 2026 15:46:22 -0500 Subject: [PATCH] fix: preserve each items that are needed by pending batches (#17819) This fixes a longstanding TODO with each blocks: currently, if any effects aren't used in the current batch at the moment of reconciliation, they are destroyed. Subsequent batches therefore end up recreating them. This is wasteful at the best of times, but if the effect contains any async work, that work has to be restarted. This PR fixes it by preserving any effects that correspond to the keys of pending batches. It _does_ mean that we need to iterate over each `keys` map for each pending batch in which an each block re-ran, but that is a rare scenario. This feels preferable to the alternative approaches. --- .changeset/spicy-teeth-tan.md | 5 ++ .../src/internal/client/dom/blocks/each.js | 64 ++++++++++++++---- .../svelte/src/internal/client/types.d.ts | 3 + .../async-each-preserve-pending/_config.js | 65 +++++++++++++++++++ .../async-each-preserve-pending/main.svelte | 29 +++++++++ 5 files changed, 153 insertions(+), 13 deletions(-) create mode 100644 .changeset/spicy-teeth-tan.md create mode 100644 packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/main.svelte diff --git a/.changeset/spicy-teeth-tan.md b/.changeset/spicy-teeth-tan.md new file mode 100644 index 0000000000..4497392a9f --- /dev/null +++ b/.changeset/spicy-teeth-tan.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: preserve each items that are needed by pending batches diff --git a/packages/svelte/src/internal/client/dom/blocks/each.js b/packages/svelte/src/internal/client/dom/blocks/each.js index 7ae02d073c..cac980aa9b 100644 --- a/packages/svelte/src/internal/client/dom/blocks/each.js +++ b/packages/svelte/src/internal/client/dom/blocks/each.js @@ -29,6 +29,7 @@ import { block, branch, destroy_effect, + move_effect, pause_effect, resume_effect } from '../../reactivity/effects.js'; @@ -83,7 +84,7 @@ function pause_effects(state, to_destroy, controlled_anchor) { if (group.pending.size === 0) { var groups = /** @type {Set} */ (state.outrogroups); - destroy_effects(array_from(group.done)); + destroy_effects(state, array_from(group.done)); groups.delete(group); if (groups.size === 0) { @@ -114,7 +115,7 @@ function pause_effects(state, to_destroy, controlled_anchor) { state.items.clear(); } - destroy_effects(to_destroy, !fast_path); + destroy_effects(state, to_destroy, !fast_path); } else { group = { pending: new Set(to_destroy), @@ -126,14 +127,36 @@ function pause_effects(state, to_destroy, controlled_anchor) { } /** + * @param {EachState} state * @param {Effect[]} to_destroy * @param {boolean} remove_dom */ -function destroy_effects(to_destroy, remove_dom = true) { - // TODO only destroy effects if no pending batch needs them. otherwise, - // just re-add the `EFFECT_OFFSCREEN` flag +function destroy_effects(state, to_destroy, remove_dom = true) { + /** @type {Set | undefined} */ + var preserved_effects; + + // The loop-in-a-loop isn't ideal, but we should only hit this in relatively rare cases + if (state.pending.size > 0) { + preserved_effects = new Set(); + + for (const keys of state.pending.values()) { + for (const key of keys) { + preserved_effects.add(/** @type {EachItem} */ (state.items.get(key)).e); + } + } + } + for (var i = 0; i < to_destroy.length; i++) { - destroy_effect(to_destroy[i], remove_dom); + var e = to_destroy[i]; + + if (preserved_effects?.has(e)) { + e.f |= EFFECT_OFFSCREEN; + + const fragment = document.createDocumentFragment(); + move_effect(e, fragment); + } else { + destroy_effect(to_destroy[i], remove_dom); + } } } @@ -185,9 +208,17 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f /** @type {V[]} */ var array; + /** @type {Map>} */ + var pending = new Map(); + var first_run = true; - function commit() { + /** + * @param {Batch} batch + */ + function commit(batch) { + state.pending.delete(batch); + state.fallback = fallback; reconcile(state, array, anchor, flags, get_key); @@ -210,6 +241,13 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f } } + /** + * @param {Batch} batch + */ + function discard(batch) { + state.pending.delete(batch); + } + var effect = block(() => { array = /** @type {V[]} */ (get(each_array)); var length = array.length; @@ -314,6 +352,8 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f } if (!first_run) { + pending.set(batch, keys); + if (defer) { for (const [key, item] of items) { if (!keys.has(key)) { @@ -322,11 +362,9 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f } batch.oncommit(commit); - batch.ondiscard(() => { - // TODO presumably we need to do something here? - }); + batch.ondiscard(discard); } else { - commit(); + commit(batch); } } @@ -345,7 +383,7 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f }); /** @type {EachState} */ - var state = { effect, flags, items, outrogroups: null, fallback }; + var state = { effect, flags, items, pending, outrogroups: null, fallback }; first_run = false; @@ -544,7 +582,7 @@ function reconcile(state, array, anchor, flags, get_key) { if (state.outrogroups !== null) { for (const group of state.outrogroups) { if (group.pending.size === 0) { - destroy_effects(array_from(group.done)); + destroy_effects(state, array_from(group.done)); state.outrogroups?.delete(group); } } diff --git a/packages/svelte/src/internal/client/types.d.ts b/packages/svelte/src/internal/client/types.d.ts index 443c21010e..f4fc81170d 100644 --- a/packages/svelte/src/internal/client/types.d.ts +++ b/packages/svelte/src/internal/client/types.d.ts @@ -1,5 +1,6 @@ import type { Store } from '#shared'; import { STATE_SYMBOL } from './constants.js'; +import type { Batch } from './reactivity/batch.js'; import type { Effect, Source, Value } from './reactivity/types.js'; declare global { @@ -84,6 +85,8 @@ export type EachState = { flags: number; /** a key -> item lookup */ items: Map; + /** a batch -> keys lookup of all keys that are still needed */ + pending: Map>; /** all outro groups that this item is a part of */ outrogroups: Set | null; /** `{:else}` effect */ diff --git a/packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/_config.js b/packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/_config.js new file mode 100644 index 0000000000..c4efa873bf --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/_config.js @@ -0,0 +1,65 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + await tick(); + + const [add, shift] = target.querySelectorAll('button'); + + add.click(); + await tick(); + add.click(); + await tick(); + add.click(); + await tick(); + + assert.htmlEqual( + target.innerHTML, + ` + + +

1

+ ` + ); + + shift.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + +

1

+

2

+ ` + ); + + shift.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + +

1

+

2

+

3

+ ` + ); + + shift.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + +

1

+

2

+

3

+

4

+ ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/main.svelte new file mode 100644 index 0000000000..55d894d240 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/main.svelte @@ -0,0 +1,29 @@ + + + + + +{#each values as v} +

{await push(v)}

+{/each}