From ab75873a57cf9f2fff1a430fc9f8901407ed645f Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Sun, 20 Sep 2026 21:55:19 +0200 Subject: [PATCH] more fixes --- .../src/internal/client/reactivity/batch.js | 30 +++++++---- .../svelte/src/internal/client/runtime.js | 50 +++++++++++-------- .../_config.js | 21 ++++++++ .../main.svelte | 17 +++++++ .../Child.svelte | 8 +++ .../_config.js | 43 ++++++++++++++++ .../main.svelte | 25 ++++++++++ .../_config.js | 40 +++++++++++++++ .../main.svelte | 22 ++++++++ .../Child.svelte | 8 +++ .../_config.js | 44 ++++++++++++++++ .../main.svelte | 30 +++++++++++ 12 files changed, 307 insertions(+), 31 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/Child.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/Child.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/main.svelte diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index fd7b0c876a..043b3a3f09 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -130,6 +130,14 @@ export class Batch { /** @type {Map} */ stale_effects = new Map(); + /** + * Effects 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} + */ + stale_readers = new Set(); + /** @type {Set} */ seen_effects = new Set(); @@ -160,14 +168,6 @@ export class Batch { */ previous = new Map(); - /** - * TODO run this on fork commit, put in all the effects we have decided we need to rerun, - * it's a map so that we can delete entries if later batches have runs in it; though - * how do we know that this no longer counts for batch 3 but still for batch 2? - * @type {Map void>} - */ - on_fork_commit = new Map(); - /** * 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 @@ -772,6 +772,10 @@ export class Batch { if (d) deferred.promise.then(d.resolve).catch(d.reject); } + for (const b of batch.dependent) { + if (b !== this) this.dependent.add(b); + } + for (const c of batch.#commit_callbacks) { this.oncommit(() => c(batch)); } @@ -1880,6 +1884,7 @@ export function fork(fn) { // TODO we need to ensure that the version bumps happen "in order", e.g. in case of source1->derived2 we need to bump S last // } + var changed = source.v !== content[0]; source.v = content[0]; if (!content[1]) { @@ -1889,6 +1894,13 @@ export function fork(fn) { // batch.mark(source, ...) TODO re-maybe-dirty- everything? // dirty those effects the fork did not see yet, e.g. because a later batch created new branches batch.mark(source, MAYBE_DIRTY, true); + } else if (changed) { + // A derived that was evaluated inside the fork: bump its version too, so that reactions + // which read the (then still old) real value _after_ the fork evaluated it — and which are + // therefore not in `stale_effects` — see a newer dependency version and re-run. + // We deliberately use a fresh version rather than the fork-time `content[2]`, because the + // real world may have run reactions since then that would otherwise outrank it. + content[2] = source.wv = increment_write_version(); } } @@ -1924,7 +1936,7 @@ export function fork(fn) { batch.flush(); // Other forks might need to rerun now with the updated state. - // TODO reuse batch.capture() logic here (maybe we can just call it?) + // TODO reuse batch.capture() logic here (delete values from fork and possibly discard, depend on other batches etc) (maybe we can just call it?) let next_batch = batch.next; while (next_batch) { for (const [source, [, is_derived]] of batch.current) { diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 03d0a8c3f9..9b967fd4d8 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -544,21 +544,15 @@ export function update_effect(effect) { if (is_latest_value) { effect.wv = write_version; } else { - // console.log('setting', effect.wv, effect, 'to', write_version, is_latest_value); - // effect.wv = write_version; - // set_signal_status(effect, MAYBE_DIRTY); - // debugger; - if (!is_latest_value) { - /** @type {Batch} */ (own_batch).stale_effects.set(effect, write_version); - var batch = /** @type {Batch} */ (own_batch).next; - while (batch) { - batch.maybe_dirty_effects.add(effect); - batch = batch.next; - } + // 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) { + batch.maybe_dirty_effects.add(effect); + batch = batch.next; } - - // TODO add to maybe_dirty_effects in all subsequent batches here, - // removing need for other cross-batch rerun mechanisms / remove need for adding blocks to maybe_dirty? } // In DEV, increment versions of any sources that were written to during the effect, @@ -646,12 +640,17 @@ export function get(signal) { // rather than updating `new_deps`, which creates GC cost if (new_deps === null && deps !== null && deps[skipped_deps] === signal) { skipped_deps++; - } else if (new_deps === null) { - new_deps = [signal]; - first_time = true; } else { - new_deps.push(signal); - first_time = true; + if (new_deps === null) { + new_deps = [signal]; + } else { + new_deps.push(signal); + } + + // Only a signal that wasn't a dependency of this reaction before counts as new — + // reading existing dependencies in a different order must not (it would make + // the reaction see the latest value instead of its batch's view, see below) + first_time = deps === null || !includes.call(deps, signal); } } } else { @@ -793,19 +792,26 @@ export function get(signal) { if (batch) { if (!current.is_eager) batch.dependent.add(current); // TODO do we only need this for async/block effects? - if (active_effect && (is_updating_effect || active_effect.f & ASYNC)) { + 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) + ) { const effect = active_effect; - // TODO can overfire when two stale reads within one effect, because no "already scheduled this" logic. - // TODO how to know "ok we already did this now" + 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); const b = Batch.ensure(); set_signal_status(effect, DIRTY); b.schedule(effect); }); } else { queue_micro_task(() => { + batch.stale_readers.delete(effect); set_signal_status(effect, DIRTY); batch.schedule(effect); batch.flush(); diff --git a/packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/_config.js b/packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/_config.js new file mode 100644 index 0000000000..7fe22437f6 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/_config.js @@ -0,0 +1,21 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +const buttons = ``; + +export default test({ + async test({ assert, target }) { + const [fork, show, commit] = target.querySelectorAll('button'); + + fork.click(); + await tick(); + + show.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

0

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

2

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/main.svelte new file mode 100644 index 0000000000..9b425a216d --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-fork-commit-derived-new-reader/main.svelte @@ -0,0 +1,17 @@ + + + + + + +{#if d2 === 2}two{/if} +{#if show} +

{d2}

+{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/Child.svelte b/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/Child.svelte new file mode 100644 index 0000000000..029e8d613e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/Child.svelte @@ -0,0 +1,8 @@ + + +

{await slow(a)}

+

{await slow(b)}

+

{await fast(`${c}:${untrack(() => `${a}${b}`)}`)}

diff --git a/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/_config.js b/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/_config.js new file mode 100644 index 0000000000..633a3c2b27 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/_config.js @@ -0,0 +1,43 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +const buttons = ` + + + + + +`; + +// B3 reads values held by both B1 and B2 and therefore depends on both. When B3 merges +// into B2 (the nearest earlier related batch), B2 must inherit B3's dependency on B1, +// otherwise B2+B3 commit before B1 and leak B1's uncommitted value into the DOM +export default test({ + async test({ assert, target }) { + const [a, b, c, resolve, resolve_last] = target.querySelectorAll('button'); + + resolve.click(); + await tick(); + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

0

0

0:00

`); + + a.click(); // B1: slow(a=1) pending + await tick(); + b.click(); // B2: slow(b=1) pending + await tick(); + c.click(); // B3: reads a=1 (held by B1) and b=1 (held by B2) -> depends on both, merges into B2 + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

0

0

0:00

`); + + // B2 finishes first: it must still wait for B1 + resolve_last.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

0

0

0:00

`); + + // B1 finishes: everything commits together + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

1

1

1:11

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/main.svelte new file mode 100644 index 0000000000..43371710a4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-merge-transitive-dependency/main.svelte @@ -0,0 +1,25 @@ + + + + + + + + + + {#snippet pending()}loading{/snippet} + + diff --git a/packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/_config.js b/packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/_config.js new file mode 100644 index 0000000000..3f32f32027 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/_config.js @@ -0,0 +1,40 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +const buttons = ` + + + + +`; + +// A reaction that reads its existing dependencies in a different order than last time +// is not a "new reader" of those dependencies — it must keep seeing its own batch's view, +// not the latest (uncommitted) value of a later pending batch +export default test({ + async test({ assert, target }) { + const [flip, a, , resolve] = target.querySelectorAll('button'); + + resolve.click(); + await tick(); + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

a0b0

true

a0

`); + + flip.click(); // B1: cond = false, slow(cond) pending + await tick(); + a.click(); // B2: a = 'a1', slow(a) pending + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

a0b0

true

a0

`); + + // B1 commits while B2 is still pending. The flipped branch reads `b` then `a` (reordered + // deps) — `a` must still resolve to B1's view (a0), not B2's uncommitted a1 + resolve.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `${buttons}

b0a0

false

a0

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

b0a1

false

a1

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/main.svelte new file mode 100644 index 0000000000..c7ec3ccd52 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-reordered-deps-not-new-reader/main.svelte @@ -0,0 +1,22 @@ + + + + + + + + + {#snippet pending()}loading{/snippet} +

{cond ? a + b : b + a}

+

{await slow(cond)}

+

{await slow(a)}

+ diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/Child.svelte b/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/Child.svelte new file mode 100644 index 0000000000..5addda5ba6 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/Child.svelte @@ -0,0 +1,8 @@ + + +

{sa}

+

{await slow(b + c)}

+

{await track(sa, b, c)}

diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/_config.js b/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/_config.js new file mode 100644 index 0000000000..7aa495b0b8 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/_config.js @@ -0,0 +1,44 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +const buttons = ``; + +// An effect that runs in an earlier batch and reads _several_ values that a later batch +// holds newer versions of must only be re-run once in that later batch, not once per value +export default test({ + async test({ assert, target, instance }) { + const [a, bc, resolve] = target.querySelectorAll('button'); + + resolve.click(); + await tick(); + resolve.click(); + await tick(); + resolve.click(); + await tick(); + assert.deepEqual(instance.get_calls(), ['0/0/0']); + assert.htmlEqual(target.innerHTML, `${buttons}

0

0

0/0/0

`); + + // B1: a++ -> `sa` pending + a.click(); + await tick(); + assert.deepEqual(instance.get_calls(), ['0/0/0']); + + // B2: b++, c++ -> `slow(b + c)` pending, `track` re-runs with B2's view + bc.click(); + await tick(); + assert.deepEqual(instance.get_calls(), ['0/0/0', '0/1/1']); + + // resolve `sa` in B1: `track` re-runs in B1 (reading B1's stale view of b/c) and, + // because it read two values held by B2, exactly once more in B2 + resolve.click(); + await tick(); + assert.deepEqual(instance.get_calls(), ['0/0/0', '0/1/1', '1/0/0', '1/1/1']); + + for (let i = 0; i < 6; i++) { + resolve.click(); + await tick(); + } + assert.deepEqual(instance.get_calls(), ['0/0/0', '0/1/1', '1/0/0', '1/1/1']); + assert.htmlEqual(target.innerHTML, `${buttons}

1

2

1/1/1

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/main.svelte new file mode 100644 index 0000000000..507edd83ce --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-read-multiple-no-double-run/main.svelte @@ -0,0 +1,30 @@ + + + + + + + + {#snippet pending()}loading{/snippet} + +