Merge b88b4e8ef8 into 2344b4052e

.changeset/tame-ears-invite.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: ensure obsolete batches are removed and its necessary dom changes committed

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

@@ -10,7 +10,6 @@ import {
 	INERT,
 	RENDER_EFFECT,
 	ROOT_EFFECT,
-	USER_EFFECT,
 	MAYBE_DIRTY
 } from '#client/constants';
 import { async_mode_flag } from '../../flags/index.js';
@@ -96,9 +95,9 @@ export class Batch {
 	/**
 	 * When the batch is committed (and the DOM is updated), we need to remove old branches
 	 * and append new ones by calling the functions added inside (if/each/key/etc) blocks
-	 * @type {Set<() => void>}
+	 * @type {Map<Effect, () => void>}
 	 */
-	#callbacks = new Set();
+	#callbacks = new Map();
 
 	/**
 	 * The number of async effects that are currently in flight
@@ -112,12 +111,6 @@ export class Batch {
 	 */
 	#deferred = null;
 
-	/**
-	 * True if an async effect inside this batch resolved and
-	 * its parent branch was already deleted
-	 */
-	#neutered = false;
-
 	/**
 	 * Async effects (created inside `async_derived`) encountered during processing.
 	 * These run after the rest of the batch has updated, since they should
@@ -184,6 +177,14 @@ export class Batch {
 		/** @type {Map<Source, { v: unknown, wv: number }> | null} */
 		var current_values = null;
 
+		/**
+		 * A batch is superseded if all of its sources are also in the current batch.
+		 * If the current batch commits, we don't need the old batch anymore.
+		 * This also prevents memory leaks since the old batch will never be committed.
+		 * @type {Batch[]}
+		 */
+		var superseded_batches = [];
+
 		// if there are multiple batches, we are 'time travelling' —
 		// we need to undo the changes belonging to any batch
 		// other than the current one
@@ -196,15 +197,25 @@ export class Batch {
 				source.v = current;
 			}
 
+			let is_prior_batch = true;
+
 			for (const batch of batches) {
-				if (batch === this) continue;
+				if (batch === this) {
+					is_prior_batch = false;
+					continue;
+				}
+
+				let superseded = is_prior_batch;
 
 				for (const [source, previous] of batch.#previous) {
 					if (!current_values.has(source)) {
+						superseded = false;
 						current_values.set(source, { v: source.v, wv: source.wv });
 						source.v = previous;
 					}
 				}
+
+				if (superseded) superseded_batches.push(batch);
 			}
 		}
 
@@ -215,6 +226,24 @@ export class Batch {
 		// if we didn't start any new async work, and no async work
 		// is outstanding from a previous flush, commit
 		if (this.#async_effects.length === 0 && this.#pending === 0) {
+			if (superseded_batches.length > 0) {
+				const own = [...this.#callbacks.keys()];
+				// A superseded batch could have callbacks for e.g. destroying if blocks
+				// that are not part of the current batch because it already happened in the prior one,
+				// and the corresponding block effect therefore returning early because nothing was changed from its
+				// point of view, therefore not adding a callback to the current batch, so we gotta call them here.
+				// We do it from newest to oldest to ensure the correct callback is applied.
+				for (const batch of superseded_batches.reverse()) {
+					for (const [effect, cb] of batch.#callbacks) {
+						if (!own.includes(effect)) {
+							cb();
+							own.push(effect);
+						}
+					}
+					batch.remove();
+				}
+			}
+
 			this.#commit();
 
 			var render_effects = this.#render_effects;
@@ -372,8 +401,17 @@ export class Batch {
 		}
 	}
 
-	neuter() {
+	remove() {
-		this.#neutered = true;
+		// Cleanup to
+		// - prevent memory leaks which could happen if a batch is tied to a never-ending promise
+		// - prevent effects from rerunning for outdated-and-now-no-longer-pending batches
+		this.#callbacks.clear();
+		this.#maybe_dirty_effects =
+			this.#dirty_effects =
+			this.#boundary_async_effects =
+			this.#async_effects =
+				[];
+		batches.delete(this);
 	}
 
 	flush() {
@@ -400,10 +438,8 @@ export class Batch {
 	 * Append and remove branches to/from the DOM
 	 */
 	#commit() {
-		if (!this.#neutered) {
+		for (const fn of this.#callbacks.values()) {
-			for (const fn of this.#callbacks) {
+			fn();
-				fn();
-			}
 		}
 
 		this.#callbacks.clear();
@@ -436,9 +472,11 @@ export class Batch {
 		}
 	}
 
-	/** @param {() => void} fn */
+	/**
+	 * @param {() => void} fn
+	 */
 	add_callback(fn) {
-		this.#callbacks.add(fn);
+		this.#callbacks.set(/** @type {Effect} */ (active_effect), fn);
 	}
 
 	settled() {

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

@@ -188,12 +188,6 @@ export function async_derived(fn, location) {
 		};
 
 		promise.then(handler, (e) => handler(null, e || 'unknown'));
-
-		if (batch) {
-			return () => {
-				queueMicrotask(() => batch.neuter());
-			};
-		}
 	});
 
 	if (DEV) {

packages/svelte/tests/runtime-runes/samples/async-redirect-2/_config.js

@@ -0,0 +1,36 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	async test({ assert, logs, target }) {
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<h1>a</h1>
+				<button>a</button>
+				<button>b</button>
+				<button>c</button>
+				<p>a</p>
+			`
+		);
+
+		const [a, b] = target.querySelectorAll('button');
+
+		b.click();
+		await tick();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<h1>c</h1>
+				<button>a</button>
+				<button>b</button>
+				<button>c</button>
+				<p>c</p>
+				<p>b or c</p>
+			`
+		);
+
+		assert.deepEqual(logs, ['route a', 'route c']);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-redirect-2/main.svelte

@@ -0,0 +1,41 @@
+<script lang=ts>
+	let route = $state('a');
+
+	function goto(r) {
+		return Promise.resolve().then(async () => {
+			route = r;
+			await Promise.resolve();
+		});
+	}
+
+	$effect(() => {
+		console.log('route ' + route);
+	});
+</script>
+
+<h1>{route}</h1>
+<button onclick={() => route = 'a'}>a</button>
+<button onclick={() => route = 'b'}>b</button>
+<button onclick={() => route = 'c'}>c</button>
+
+<svelte:boundary>
+	{#if route === 'a'}
+		<p>a</p>
+	{/if}
+
+	{#if route === 'b'} 
+		{await goto('c')}
+	{/if}
+
+	{#if route === 'c'}
+		<p>c</p>
+	{/if}
+
+	{#if route === 'b' || route === 'c'}
+		<p>b or c</p>
+	{/if}
+
+	{#snippet pending()}
+		<p>pending...</p>
+	{/snippet}
+</svelte:boundary>