From 47ea6a53b9358fa98128e273e527657fb3174b21 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Wed, 5 Nov 2025 15:49:09 +0100 Subject: [PATCH 1/2] fix: delete from batch_values on updates This fixes a bug where a derived would still show its old value even after it was indirectly updated again within the same batch. This can for example happen by reading a derived on an effect, then writing to a source in that same effect that makes the derived update, and then read the derived value in a sibling effect - it still shows the old value without the fix. The fix is to _delete_ the value from batch_values, as it's now the newest value across all batches. In order to not prevent breakage on other batches we have to leave the status of deriveds as-is, i.e. within is_dirty and update_derived we cannot update its status. That might be a bit more inefficient as you now have to traverse the graph for each `get` of that derived (it's a bit like they are all disconnected) but we can always optimize that later if need be. Another advantage of this fix is that we can get rid of duplicate logic we had to add about unmarking and reconnecting deriveds, because that logic was only needed for the case where deriveds are read after they are updated, which now no longer hits that if-branch --- .../src/internal/client/reactivity/batch.js | 5 +-- .../internal/client/reactivity/deriveds.js | 14 +++++++- .../svelte/src/internal/client/runtime.js | 36 ++++--------------- .../_config.js | 6 ++-- 4 files changed, 25 insertions(+), 36 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 57aa185a31db..9d1d776188e3 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -286,11 +286,12 @@ export class Batch { this.previous.set(source, value); } - // Don't save errors in `batch_values`, or they won't be thrown in `runtime.js#get` if ((source.f & ERROR_VALUE) === 0) { this.current.set(source, source.v); - batch_values?.set(source, source.v); } + + // The value is now the newest known value for this source, therefore remove it from batch_values + batch_values?.delete(source); } activate() { diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index d7a093a1dafe..8ead947b5535 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -365,8 +365,20 @@ export function update_derived(derived) { return; } + // During time traveling we don't want to reset the status so that + // traversal of the graph in the other batches still happens if (batch_values !== null) { - batch_values.set(derived, derived.v); + // Delete the value as the current one is now the latest. + // Deleting instead of updating handles the case where a derived + // is subsequently indirectly updated in the same batch — without + // deleting here we would incorrectly get the old value from `batch_values` + // instead of recomputing it. The one drawback is that it's now a bit + // more inefficient to get the value of that derived again in the same batch, + // as it has to check is_dirty all the way up the graph all the time. + // TODO if that turns out to be a performance problem, we could try + // to save the current status of the derived in a map and restore it + // before leaving the batch. + batch_values.delete(derived); } else { var status = (derived.f & CONNECTED) === 0 ? MAYBE_DIRTY : CLEAN; set_signal_status(derived, status); diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 5f59b7aeacd3..74797f3b56fe 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -179,7 +179,12 @@ export function is_dirty(reaction) { } } - if ((flags & CONNECTED) !== 0) { + if ( + (flags & CONNECTED) !== 0 && + // During time traveling we don't want to reset the status so that + // traversal of the graph in the other batches still happens + batch_values === null + ) { set_signal_status(reaction, CLEAN); } } @@ -611,17 +616,7 @@ export function get(signal) { } else if (is_derived) { derived = /** @type {Derived} */ (signal); - var should_reconnect = is_updating_effect && effect_tracking() && (derived.f & CONNECTED) === 0; - if (batch_values?.has(derived)) { - // This happens as part of is_dirty normally, but we return early - // here so we need to do it separately - remove_marked_flag(derived); - - if (should_reconnect) { - reconnect(derived); - } - return batch_values.get(derived); } @@ -629,7 +624,7 @@ export function get(signal) { update_derived(derived); } - if (should_reconnect) { + if (is_updating_effect && effect_tracking() && (derived.f & CONNECTED) === 0) { reconnect(derived); } } else if (batch_values?.has(signal)) { @@ -662,23 +657,6 @@ function reconnect(derived) { } } -/** - * Removes the WAS_MARKED flag from the derived and its dependencies - * @param {Value} derived - */ -function remove_marked_flag(derived) { - // We cannot stop at the first non-marked derived because batch_values can - // cause "holes" of unmarked deriveds in an otherwise marked graph - if ((derived.f & DERIVED) === 0) return; - - derived.f &= ~WAS_MARKED; - - // Only deriveds with dependencies can be marked - for (const dep of /** @type {Value[]} */ (/** @type {Derived} */ (derived).deps)) { - remove_marked_flag(dep); - } -} - /** @param {Derived} derived */ function depends_on_old_values(derived) { if (derived.v === UNINITIALIZED) return true; // we don't know, so assume the worst diff --git a/packages/svelte/tests/runtime-runes/samples/async-derived-in-multiple-effects/_config.js b/packages/svelte/tests/runtime-runes/samples/async-derived-in-multiple-effects/_config.js index 600e7f096b0a..15bb42074f73 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-derived-in-multiple-effects/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-derived-in-multiple-effects/_config.js @@ -7,12 +7,10 @@ export default test({ button?.click(); await tick(); - // TODO this is wrong: it should be [5] - assert.deepEqual(logs, [4]); + assert.deepEqual(logs, [5]); button?.click(); await tick(); - // TODO this is wrong: it should be [5, 7] - assert.deepEqual(logs, [4, 7]); + assert.deepEqual(logs, [5, 7]); } }); From 6ca62452635b6c64b92e20d4bc22a713bbc03b1e Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 5 Nov 2025 13:32:13 -0500 Subject: [PATCH 2/2] keep derived cache, but clear it in mark_reactions (#17116) --- .../src/internal/client/reactivity/batch.js | 5 ++--- .../src/internal/client/reactivity/deriveds.js | 18 ++++++------------ .../src/internal/client/reactivity/sources.js | 9 +++++++-- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 9d1d776188e3..57aa185a31db 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -286,12 +286,11 @@ export class Batch { this.previous.set(source, value); } + // Don't save errors in `batch_values`, or they won't be thrown in `runtime.js#get` if ((source.f & ERROR_VALUE) === 0) { this.current.set(source, source.v); + batch_values?.set(source, source.v); } - - // The value is now the newest known value for this source, therefore remove it from batch_values - batch_values?.delete(source); } activate() { diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 8ead947b5535..7e6f3c6f604f 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -26,7 +26,7 @@ import { import { equals, safe_equals } from './equality.js'; import * as e from '../errors.js'; import * as w from '../warnings.js'; -import { async_effect, destroy_effect, teardown } from './effects.js'; +import { async_effect, destroy_effect, effect_tracking, teardown } from './effects.js'; import { eager_effects, internal_set, set_eager_effects, source } from './sources.js'; import { get_stack } from '../dev/tracing.js'; import { async_mode_flag, tracing_mode_flag } from '../../flags/index.js'; @@ -368,17 +368,11 @@ export function update_derived(derived) { // During time traveling we don't want to reset the status so that // traversal of the graph in the other batches still happens if (batch_values !== null) { - // Delete the value as the current one is now the latest. - // Deleting instead of updating handles the case where a derived - // is subsequently indirectly updated in the same batch — without - // deleting here we would incorrectly get the old value from `batch_values` - // instead of recomputing it. The one drawback is that it's now a bit - // more inefficient to get the value of that derived again in the same batch, - // as it has to check is_dirty all the way up the graph all the time. - // TODO if that turns out to be a performance problem, we could try - // to save the current status of the derived in a map and restore it - // before leaving the batch. - batch_values.delete(derived); + // only cache the value if we're in a tracking context, otherwise we won't + // clear the cache in `mark_reactions` when dependencies are updated + if (effect_tracking()) { + batch_values.set(derived, derived.v); + } } else { var status = (derived.f & CONNECTED) === 0 ? MAYBE_DIRTY : CLEAN; set_signal_status(derived, status); diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 0b5ad33bfcb1..8ae406b57b30 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -34,7 +34,7 @@ import * as e from '../errors.js'; import { legacy_mode_flag, tracing_mode_flag } from '../../flags/index.js'; import { get_stack, tag_proxy } from '../dev/tracing.js'; import { component_context, is_runes } from '../context.js'; -import { Batch, eager_block_effects, schedule_effect } from './batch.js'; +import { Batch, batch_values, eager_block_effects, schedule_effect } from './batch.js'; import { proxy } from '../proxy.js'; import { execute_derived } from './deriveds.js'; @@ -334,12 +334,17 @@ function mark_reactions(signal, status) { } if ((flags & DERIVED) !== 0) { + var derived = /** @type {Derived} */ (reaction); + + batch_values?.delete(derived); + if ((flags & WAS_MARKED) === 0) { // Only connected deriveds can be reliably unmarked right away if (flags & CONNECTED) { reaction.f |= WAS_MARKED; } - mark_reactions(/** @type {Derived} */ (reaction), MAYBE_DIRTY); + + mark_reactions(derived, MAYBE_DIRTY); } } else if (not_dirty) { if ((flags & BLOCK_EFFECT) !== 0) {