Merge branch 'async-each-preserve-pending' into each-block-pending

.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}