From 91d215664f0db97eb31ab8d5b1007157d716aab7 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 21 Sep 2026 00:10:27 +0200 Subject: [PATCH] resolve some todos --- .../src/internal/client/reactivity/batch.js | 65 ++++++------ .../src/internal/client/reactivity/sources.js | 20 +++- .../svelte/src/internal/client/runtime.js | 98 ++++++++++--------- .../_config.js | 30 ++++++ .../main.svelte | 19 ++++ .../Child.svelte | 14 +++ .../_config.js | 33 +++++++ .../main.svelte | 27 +++++ 8 files changed, 226 insertions(+), 80 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/Child.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/main.svelte diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 5b4ff7ec4a..cae469ce22 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/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. - * @type {Set} + * avoid scheduling the same reaction multiple times when it reads more than one such value. + * @type {Set} */ 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) - * They keys of this map are identical to `this.#previous` + * 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` * @type {Map} */ current = new Map(); /** - * The values 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` - * @type {Map} + * The values and write versions of any signals (sources and deriveds) that are updated in this batch _before_ those updates took place. + * Tuple format: [value, write_version] + * They keys of this map are identical to `this.current` + * @type {Map} */ 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 && - (!is_latest_value || - batch.current.has(source) || - // Check derived's dependencies for outdated values. We only have to check one - // level because is_dirty etc will execute the top-most deriveds first, whose result - // the later deriveds can use to make a decision ("oh this derived's value is different to what I cached") - ((source.f & DERIVED) !== 0 && - /** @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) - ))) + batch.current.has(source) || + ((source.f & DERIVED) !== 0 && + /** @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; - while (batch) { + for (let batch = first_batch; batch !== null; batch = batch.next) { 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); } } } diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index a9571db89e..8230edc114 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/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 diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index d0d22ab912..563b0e692b 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/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? - // A dependency is "not latest" if `batch_values` overrides it with a value the batch itself - // did not write (i.e. it's another batch's value being hidden from us). - // TODO consolidate with similar logic in batch.capture() - var own_batch = previous_batch ?? current_batch; - let is_latest_value = true; - // Can be falsy inside flush_eager_effects - if (own_batch) { - is_latest_value = - !own_batch.is_fork && - (!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) - ); - })); - } + // Did the effect see the latest value of all its dependencies, or (partly) its batch's view + // of them, i.e. did `batch_values` hand it a value that differs from the real one? We only + // need to look one level deep: a derived that was itself computed from such a value was not + // written to the real world either, so it differs as well. + var own_batch = previous_batch ?? current_batch; // can be null inside flush_eager_effects + var is_latest_value = + own_batch === null || + (!own_batch.is_fork && + (effect.deps === null || + !effect.deps.some((d) => batch_values?.has(d) && batch_values.get(d) !== 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 batch = /** @type {Batch} */ (own_batch).next; - while (batch) { + var own = /** @type {Batch} */ (own_batch); + own.stale_effects.set(effect, write_version); + 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 && - (is_updating_effect || active_effect.f & ASYNC) && - // an effect can read several stale values in one run — only schedule the re-run once - !batch.stale_readers.has(active_effect) + reader !== null && + reactive && + // a reaction can read several stale values in one run — only schedule the re-run once + !batch.stale_readers.has(reader) ) { - const effect = active_effect; - batch.stale_readers.add(effect); + batch.stale_readers.add(reader); 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); - const b = Batch.ensure(); - set_signal_status(effect, DIRTY); - b.schedule(effect); + batch.stale_readers.delete(reader); + Batch.ensure(); + invalidate(reader); }); } else { queue_micro_task(() => { - batch.stale_readers.delete(effect); - set_signal_status(effect, DIRTY); - batch.schedule(effect); - batch.flush(); + batch.stale_readers.delete(reader); + const b = batch.activate(); + invalidate(reader); + b.flush(); }); } } } } - if ( - // TODO correct?! I thought the failure can only occur in case we see new values for the first time while flushing (render)effects, - // but it can also occur when resolving async deriveds after creating them for the first time, which can happen outside - // the effects flush phase. - (!first_time || !previous_batch) && - // (!first_time || current_batch?.is_fork || signal.v === UNINITIALIZED) && - batch_values?.has(signal) - ) { + // A reaction that reads a signal for the first time must see the latest value, rather than + // this batch's view, if that view could hide the write of an _earlier_ batch — the user's + // program made that write before this batch's writes, so hiding it could e.g. crash a newly + // created branch (see `async-state-read-new-dependency`). Earlier batches' writes are hidden + // only while flushing a committing batch (`previous_batch` is set, see `apply(true)`) and in + // eager batches (which hide every other batch). Everywhere else `batch_values` only hides + // _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); } diff --git a/packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/_config.js b/packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/_config.js new file mode 100644 index 0000000000..2a54e2bbf6 --- /dev/null +++ b/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 = ` + + + +`; + +// 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}

1

`); + + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

1

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/main.svelte new file mode 100644 index 0000000000..42d43ad005 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-eager-read-new-dependency/main.svelte @@ -0,0 +1,19 @@ + + + + + + +{await wait(value)} + +{#if $state.eager(show)} +

{value.x}

+{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/Child.svelte b/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/Child.svelte new file mode 100644 index 0000000000..ea6a8ec7b2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/Child.svelte @@ -0,0 +1,14 @@ + + +

{sx}

+

{await slow(s)}

diff --git a/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/_config.js b/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/_config.js new file mode 100644 index 0000000000..5296a8add9 --- /dev/null +++ b/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']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/main.svelte new file mode 100644 index 0000000000..9421500071 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-hidden-write-version-no-rerun/main.svelte @@ -0,0 +1,27 @@ + + + + + + + + + {#snippet pending()}loading{/snippet} + +