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

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: preserve each items that are needed by pending batches

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<EachOutroGroup>} */ (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<Effect> | 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<Batch, Set<any>>} */
+	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);
 			}
 		}

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<any, EachItem>;
+	/** a batch -> keys lookup of all keys that are still needed */
+	pending: Map<Batch, Set<any>>;
 	/** all outro groups that this item is a part of */
 	outrogroups: Set<EachOutroGroup> | null;
 	/** `{:else}` effect */

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,
+			`
+				<button>add</button>
+				<button>shift</button>
+				<p>1</p>
+			`
+		);
+
+		shift.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>add</button>
+				<button>shift</button>
+				<p>1</p>
+				<p>2</p>
+			`
+		);
+
+		shift.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>add</button>
+				<button>shift</button>
+				<p>1</p>
+				<p>2</p>
+				<p>3</p>
+			`
+		);
+
+		shift.click();
+		await tick();
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>add</button>
+				<button>shift</button>
+				<p>1</p>
+				<p>2</p>
+				<p>3</p>
+				<p>4</p>
+			`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-each-preserve-pending/main.svelte

@@ -0,0 +1,29 @@
+<script>
+	let values = $state([1]);
+
+	const queue = [];
+
+	function push(v) {
+		if (v === 1) return v;
+
+		const p = Promise.withResolvers();
+		queue.push(() => p.resolve(v));
+		return p.promise;
+	}
+
+	function shift() {
+		const fn = queue.shift();
+		if (fn) fn();
+	}
+
+	function addValue() {
+		values = [...values, values.length + 1];
+	}
+</script>
+
+<button onclick={addValue}>add</button>
+<button onclick={shift}>shift</button>
+
+{#each values as v}
+	<p>{await push(v)}</p>
+{/each}