resolve some todos

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

@@ -130,10 +130,10 @@ export class Batch {
 	stale_effects = new Map();
 
 	/**
-	 * Effects that, while running in an earlier batch, read a value that this batch holds
+	 * Reactions that, while running in an earlier batch, read a value that this batch holds
 	 * a newer version of, and that are therefore scheduled to re-run in this batch. Used to
-	 * avoid scheduling the same effect multiple times when it reads more than one such value.
+	 * avoid scheduling the same reaction multiple times when it reads more than one such value.
-	 * @type {Set<Effect>}
+	 * @type {Set<Reaction>}
 	 */
 	stale_readers = new Set();
 
@@ -154,16 +154,17 @@ export class Batch {
 
 	/**
 	 * The current values of any signals that are updated in this batch.
-	 * Tuple format: [value, is_derived] (note: is_derived is false for deriveds, too, if they were overridden via assignment)
+	 * Tuple format: [value, is_derived, write_version] (note: is_derived is false for deriveds, too, if they were overridden via assignment)
-	 * They keys of this map are identical to `this.#previous`
+	 * They keys of this map are identical to `this.previous`
 	 * @type {Map<Value, [any, boolean, number]>}
 	 */
 	current = new Map();
 
 	/**
-	 * The values of any signals (sources and deriveds) that are updated in this batch _before_ those updates took place.
+	 * The values and write versions of any signals (sources and deriveds) that are updated in this batch _before_ those updates took place.
-	 * They keys of this map are identical to `this.#current`
+	 * Tuple format: [value, write_version]
-	 * @type {Map<Value, any>}
+	 * They keys of this map are identical to `this.current`
+	 * @type {Map<Value, [any, number]>}
 	 */
 	previous = new Map();
 
@@ -698,7 +699,7 @@ export class Batch {
 	#merge(batch) {
 		for (const [source, value] of batch.current) {
 			if (!this.previous.has(source) && batch.previous.has(source)) {
-				this.previous.set(source, batch.previous.get(source));
+				this.previous.set(source, /** @type {[any, number]} */ (batch.previous.get(source)));
 			}
 
 			this.current.set(source, value);
@@ -767,7 +768,7 @@ export class Batch {
 	 */
 	capture(source, value, is_derived = false) {
 		if (source.v !== UNINITIALIZED && !this.previous.has(source)) {
-			this.previous.set(source, source.v);
+			this.previous.set(source, [source.v, source.wv]);
 		}
 
 		const wv = increment_write_version();
@@ -779,27 +780,31 @@ export class Batch {
 			wv_values?.set(source, wv);
 		}
 
-		let batch = this.next;
+		// The value becomes the real one unless this is a fork or a later batch wrote to the source
+		// as well. For a derived, the same goes if a later batch wrote to one of its dependencies:
+		// the derived value then belongs to that batch's world, not ours. (This is deliberately not
+		// the same check as in `update_effect`: a later batch's write is visible through `batch_values`,
+		// so comparing what was read against the real value could not attribute the value to the right
+		// batch, see `async-dont-rebase-new-batch-4`.) We only need to look one level deep: `is_dirty`
+		// evaluates the top-most deriveds first, so a dependency derived that was itself not the latest
+		// value was not written to the real world, and differs from our value for it.
 		let is_latest_value = !this.is_fork;
-		while (batch) {
+
+		for (let batch = this.next; batch !== null && is_latest_value; batch = batch.next) {
+			if (batch.is_fork) continue;
+
 			if (
-				!batch.is_fork &&
+				batch.current.has(source) ||
-				(!is_latest_value ||
+				((source.f & DERIVED) !== 0 &&
-					batch.current.has(source) ||
+					/** @type {Derived} */ (source).deps?.some(
-					// Check derived's dependencies for outdated values. We only have to check one
+						(d) =>
-					// level because is_dirty etc will execute the top-most deriveds first, whose result
+							/** @type {Batch} */ (batch).current.has(d) ||
-					// the later deriveds can use to make a decision ("oh this derived's value is different to what I cached")
+							(this.current.has(d) &&
-					((source.f & DERIVED) !== 0 &&
+								/** @type {[any, boolean, number]} */ (this.current.get(d))[0] !== d.v)
-						/** @type {Derived} */ (source).deps?.some(
+					))
-							(d) =>
-								/** @type {Batch} */ (batch).current.has(d) ||
-								(this.current.has(d) &&
-									/** @type {[any, boolean, number]} */ (this.current.get(d))[0] !== d.v)
-						)))
 			) {
 				is_latest_value = false;
 			}
-			batch = batch.next;
 		}
 
 		if (is_latest_value) {
@@ -807,8 +812,7 @@ export class Batch {
 			source.wv = wv;
 		}
 
-		batch = first_batch;
+		for (let batch = first_batch; batch !== null; batch = batch.next) {
-		while (batch) {
 			if (batch.id < this.id && batch.current.has(source)) {
 				this.dependent.add(batch);
 			}
@@ -816,7 +820,6 @@ export class Batch {
 			if (batch.is_fork && is_latest_value) {
 				this.notify_fork(batch, source, is_derived, value);
 			}
-			batch = batch.next;
 		}
 	}
 
@@ -1048,10 +1051,10 @@ export class Batch {
 			}
 
 			if (batch.id > this.id || include_earlier || this.is_eager) {
-				for (const [source, value] of batch.previous) {
+				for (const [source, [value, wv]] of batch.previous) {
 					if (!batch_values.has(source)) {
 						batch_values.set(source, value);
-						// TODO I think we need previous_wv in batch.previous
+						wv_values.set(source, wv);
 					}
 				}
 			}

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

@@ -1,4 +1,4 @@
-/** @import { Derived, Effect, Source, Value } from '#client' */
+/** @import { Derived, Effect, Reaction, Source, Value } from '#client' */
 import { DEV } from 'esm-env';
 import {
 	active_reaction,
@@ -350,6 +350,24 @@ export function increment(source) {
 	set(source, source.v + 1);
 }
 
+/**
+ * Make `reaction` re-run in the current batch. For a derived this means dirtying
+ * its reactions, as if the derived's value had changed.
+ * @param {Reaction} reaction
+ */
+export function invalidate(reaction) {
+	set_signal_status(reaction, DIRTY);
+
+	if ((reaction.f & DERIVED) !== 0) {
+		seen = null;
+		count_deps = 0;
+		mark_reactions(/** @type {Derived} */ (reaction), DIRTY, null);
+		seen = null;
+	} else {
+		schedule_effect(/** @type {Effect} */ (reaction));
+	}
+}
+
 /**
  * @param {Value} signal
  * @param {number} status should be DIRTY or MAYBE_DIRTY

packages/svelte/src/internal/client/runtime.js

@@ -25,7 +25,7 @@ import {
 	REACTION_RAN,
 	ASYNC
 } from './constants.js';
-import { old_values } from './reactivity/sources.js';
+import { invalidate, old_values } from './reactivity/sources.js';
 import {
 	reactivity_loss_tracker,
 	execute_derived,
@@ -493,26 +493,16 @@ export function update_effect(effect) {
 		var teardown = update_reaction(effect);
 		effect.teardown = typeof teardown === 'function' ? teardown : null;
 
-		// Did the effect see the latest value of all its dependencies, or (some of) its batch's view?
+		// Did the effect see the latest value of all its dependencies, or (partly) its batch's view
-		// A dependency is "not latest" if `batch_values` overrides it with a value the batch itself
+		// of them, i.e. did `batch_values` hand it a value that differs from the real one? We only
-		// did not write (i.e. it's another batch's value being hidden from us).
+		// need to look one level deep: a derived that was itself computed from such a value was not
-		// TODO consolidate with similar logic in batch.capture()
+		// written to the real world either, so it differs as well.
-		var own_batch = previous_batch ?? current_batch;
+		var own_batch = previous_batch ?? current_batch; // can be null inside flush_eager_effects
-		let is_latest_value = true;
+		var is_latest_value =
-		// Can be falsy inside flush_eager_effects
+			own_batch === null ||
-		if (own_batch) {
+			(!own_batch.is_fork &&
-			is_latest_value =
+				(effect.deps === null ||
-				!own_batch.is_fork &&
+					!effect.deps.some((d) => batch_values?.has(d) && batch_values.get(d) !== d.v)));
-				(!effect.deps?.length ||
-					!effect.deps.some((d) => {
-						return (
-							batch_values &&
-							batch_values.has(d) &&
-							(!own_batch?.current.has(d) ||
-								/** @type {any} */ (own_batch.current.get(d))[0] !== d.v)
-						);
-					}));
-		}
 
 		if (is_latest_value) {
 			effect.wv = write_version;
@@ -520,11 +510,10 @@ export function update_effect(effect) {
 			// The effect ran with values that are not the latest ones (it saw its own batch's view).
 			// Don't update its write version — instead remember it so that the batch can bring it
 			// up to date on commit, and tell all subsequent batches that it may need to re-run in their view.
-			/** @type {Batch} */ (own_batch).stale_effects.set(effect, write_version);
+			var own = /** @type {Batch} */ (own_batch);
-			var batch = /** @type {Batch} */ (own_batch).next;
+			own.stale_effects.set(effect, write_version);
-			while (batch) {
+			for (var batch = own.next; batch !== null; batch = batch.next) {
 				batch.maybe_dirty_effects.add(effect);
-				batch = batch.next;
 			}
 		}
 
@@ -764,44 +753,57 @@ export function get(signal) {
 		const batch = stale_sources?.get(signal);
 		if (batch) {
 			if (!current.is_eager) batch.dependent.add(current);
-			// TODO do we only need this for async/block effects?
+
+			// The reaction that read the stale value has to re-run in `batch`'s world. If we're inside
+			// a derived, that's the derived (whose reactions get dirtied): `active_effect` is only the
+			// derived's parent then, not the effect on whose behalf the derived is evaluated (which may
+			// even happen in `is_dirty`, i.e. outside of any effect update)
+			const in_derived = active_reaction !== null && (active_reaction.f & DERIVED) !== 0;
+			const reader = in_derived ? active_reaction : active_effect;
+
+			var reactive =
+				in_derived ||
+				is_updating_effect ||
+				(active_effect !== null && (active_effect.f & ASYNC) !== 0);
+
 			if (
-				active_effect &&
+				reader !== null &&
-				(is_updating_effect || active_effect.f & ASYNC) &&
+				reactive &&
-				// an effect can read several stale values in one run — only schedule the re-run once
+				// a reaction can read several stale values in one run — only schedule the re-run once
-				!batch.stale_readers.has(active_effect)
+				!batch.stale_readers.has(reader)
 			) {
-				const effect = active_effect;
+				batch.stale_readers.add(reader);
-				batch.stale_readers.add(effect);
 
 				if (current.is_eager) {
 					// TODO only do this if we can see that the batch doesn't have this already scheduled in (maybe)dirty effects.
 					batch.oncommit(() => {
-						batch.stale_readers.delete(effect);
+						batch.stale_readers.delete(reader);
-						const b = Batch.ensure();
+						Batch.ensure();
-						set_signal_status(effect, DIRTY);
+						invalidate(reader);
-						b.schedule(effect);
 					});
 				} else {
 					queue_micro_task(() => {
-						batch.stale_readers.delete(effect);
+						batch.stale_readers.delete(reader);
-						set_signal_status(effect, DIRTY);
+						const b = batch.activate();
-						batch.schedule(effect);
+						invalidate(reader);
-						batch.flush();
+						b.flush();
 					});
 				}
 			}
 		}
 	}
 
-	if (
+	// A reaction that reads a signal for the first time must see the latest value, rather than
-		// TODO correct?! I thought the failure can only occur in case we see new values for the first time while flushing (render)effects,
+	// this batch's view, if that view could hide the write of an _earlier_ batch — the user's
-		// but it can also occur when resolving async deriveds after creating them for the first time, which can happen outside
+	// program made that write before this batch's writes, so hiding it could e.g. crash a newly
-		// the effects flush phase.
+	// created branch (see `async-state-read-new-dependency`). Earlier batches' writes are hidden
-		(!first_time || !previous_batch) &&
+	// only while flushing a committing batch (`previous_batch` is set, see `apply(true)`) and in
-		// (!first_time || current_batch?.is_fork || signal.v === UNINITIALIZED) &&
+	// eager batches (which hide every other batch). Everywhere else `batch_values` only hides
-		batch_values?.has(signal)
+	// _later_ batches' writes, which is correct even for new readers: that's the state the
-	) {
+	// program was in when this batch's writes happened.
+	var see_latest = first_time && (previous_batch !== null || current_batch?.is_eager);
+
+	if (!see_latest && batch_values?.has(signal)) {
 		return batch_values.get(signal);
 	}
 

packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/_config.js

@@ -0,0 +1,30 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+const buttons = `
+	<button>update</button>
+	<button>show</button>
+	<button>resolve</button>
+`;
+
+// Same as `async-state-read-new-dependency`, but the branch is selected by an eager
+// expression: the eager batch hides every other batch's writes, so a newly created
+// reader inside it must still see the latest value of `value` rather than `undefined`
+export default test({
+	mode: ['client'],
+	async test({ assert, target }) {
+		await tick();
+		const [update, show, resolve] = target.querySelectorAll('button');
+
+		update.click(); // pending batch writes `value`
+		await tick();
+
+		show.click(); // eager flush creates the branch, which reads `value` for the first time
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>1</p>`);
+
+		resolve.click();
+		await tick();
+		assert.htmlEqual(target.innerHTML, `${buttons}<p>1</p>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/main.svelte

@@ -0,0 +1,19 @@
+<script>
+	let value = $state();
+	let show = $state(false);
+	const deferred = Promise.withResolvers();
+
+	function wait(value) {
+		return value === undefined ? '' : deferred.promise;
+	}
+</script>
+
+<button onclick={() => (value = { x: 1 })}>update</button>
+<button onclick={() => (show = true)}>show</button>
+<button onclick={() => deferred.resolve('')}>resolve</button>
+
+{await wait(value)}
+
+{#if $state.eager(show)}
+	<p>{value.x}</p>
+{/if}

packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/Child.svelte

@@ -0,0 +1,14 @@
+<script>
+	let { x, s, slow, log } = $props();
+
+	// async result of x; `d` only flips when it crosses 0, so incrementing x doesn't change it
+	let sx = $derived(await slow(x));
+	let d = $derived(sx > 0);
+
+	$effect(() => {
+		log(`${s}/${d}`);
+	});
+</script>
+
+<p>{sx}</p>
+<p>{await slow(s)}</p>

packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/_config.js

@@ -0,0 +1,33 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+// When a later batch's write is hidden from an earlier batch, its write version must be
+// hidden as well — otherwise reactions of that source look dirty to the earlier batch
+// and re-run even though they see the old (unchanged) value
+export default test({
+	async test({ assert, target, instance }) {
+		const [x, s, resolve] = target.querySelectorAll('button');
+
+		resolve.click();
+		await tick();
+		resolve.click();
+		await tick();
+		assert.deepEqual(instance.get_logs(), ['0/true']);
+
+		x.click(); // B1: slow(x = 2) pending; `d` stays true
+		await tick();
+		s.click(); // B2: slow(s = 1) pending; the $effect is dirtied by `s` and deferred in B2
+		await tick();
+		assert.deepEqual(instance.get_logs(), ['0/true']);
+
+		// B1 commits while B2 is pending. In B1's world nothing the effect depends on changed
+		// (`s` is hidden and still 0, `d` is still true), so it must not run
+		resolve.click();
+		await tick();
+		assert.deepEqual(instance.get_logs(), ['0/true']);
+
+		resolve.click();
+		await tick();
+		assert.deepEqual(instance.get_logs(), ['0/true', '1/true']);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/main.svelte

@@ -0,0 +1,27 @@
+<script>
+	import Child from './Child.svelte';
+	let x = $state(1);
+	let s = $state(0);
+	let logs = [];
+
+	const resolvers = [];
+	function slow(v) {
+		return new Promise((r) => resolvers.push(() => r(v)));
+	}
+	function log(v) {
+		logs.push(v);
+	}
+	export function get_logs() {
+		return logs;
+	}
+</script>
+
+<button onclick={() => x++}>x</button>
+<button onclick={() => s++}>s</button>
+<button onclick={() => resolvers.shift()?.()}>resolve</button>
+<button onclick={() => resolvers.pop()?.()}>resolve last</button>
+
+<svelte:boundary>
+	{#snippet pending()}loading{/snippet}
+	<Child {x} {s} {slow} {log} />
+</svelte:boundary>