diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index cf0d82668e..52f5ba3f5f 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -117,6 +117,9 @@ export class Batch { linked = true; + /** @type {Set} */ + effects_ran = new Set(); + /** @type {Batch | null} */ #prev = null; @@ -144,6 +147,14 @@ 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 @@ -530,6 +541,7 @@ export class Batch { } else if (async_mode_flag && (flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0) { render_effects.push(effect); } else if (is_dirty(effect)) { + this.effects_ran.add(effect); if ((flags & BLOCK_EFFECT) !== 0) this.#maybe_dirty_effects.add(effect); update_effect(effect); } @@ -582,6 +594,53 @@ export class Batch { return null; } + /** + * Mark all reactive trees leading to block/async effects that (indirectly) depend on `value` + * @param {Value} value + * @param {number} status + * @param {boolean} not_yet - whether to mark effects that have not yet run, as opposed to those that have already run + */ + mark(value, status, not_yet = false) { + var reactions = value.reactions; + if (reactions === null) return false; + // skip if value is derived and is neither dirty nor maybe dirty. transitive + // deriveds (a derived depending on another derived) are only MAYBE_DIRTY, so + // we must continue traversing them to reach the effects that depend on them + // if ((value.f & DERIVED) !== 0 && (value.f & (DIRTY | MAYBE_DIRTY)) === 0) { + // return; + // } + + let marked = false; + + for (const reaction of reactions) { + var flags = reaction.f; + + if ((flags & DERIVED) !== 0) { + if (this.mark(/** @type {Derived} */ (reaction), MAYBE_DIRTY, not_yet)) { + set_signal_status(/** @type {Derived} */ (reaction), status); + marked = true; + } + } else { + var effect = /** @type {Effect} */ (reaction); + + if ( + not_yet + ? !this.effects_ran.has(effect) && + !this.#dirty_effects.has(effect) && + !this.#maybe_dirty_effects.has(effect) + : flags & (ASYNC | BLOCK_EFFECT) && this.effects_ran.has(effect) + ) { + this.#maybe_dirty_effects.delete(effect); + set_signal_status(effect, status); + this.schedule(effect); + marked = true; + } + } + } + + return marked; + } + /** * @param {Batch} batch */ @@ -591,6 +650,9 @@ export class Batch { this.previous.set(source, batch.previous.get(source)); } + if (this.current.get(source)?.[0] !== value[0]) { + // this.mark(source, DIRTY); + } this.current.set(source, value); } @@ -599,6 +661,31 @@ export class Batch { if (d) deferred.promise.then(d.resolve).catch(d.reject); } + for (const c of batch.#commit_callbacks) { + this.oncommit(() => c(batch)); + } + + for (const c of batch.#discard_callbacks) { + this.ondiscard(() => c(batch)); + } + + for (const [s, v] of batch.#skipped_branches) { + this.#skipped_branches.set(s, v); + this.#unskipped_branches.delete(s); + } + + for (const s of batch.#unskipped_branches) { + const v = this.#skipped_branches.get(s); + // TODO i do wonder at this point if it's less code / easier / more robust to do what mark() below does + // instead and just rerun all the block effects. Though it will certainly overrun some blocks, potentially + // with bad consequences for e.g. each blocks (could generate a new array etc etc). + if (v) { + v.d = v.d.filter((e) => !batch.async_deriveds.has(e)); + v.m = v.m.filter((e) => !batch.async_deriveds.has(e)); + } + this.unskip_effect(s); + } + // Clear them or else those that are still pending might get rejected on discard (after merged-into batch is done). // This can happen when batch Y merged into X and Y has a pending boundary and therefore still-pending async deriveds inside. batch.async_deriveds.clear(); @@ -633,7 +720,9 @@ export class Batch { var effect = /** @type {Effect} */ (reaction); // TODO this overfires e.g. for async-state-new-branch-fork-5 where it reruns - // the Child async effects with "world" after commit+first resolve. + // the Child async effects with "world" after commit+first resolve; generally + // overfires everywhere where the merged effect has async deriveds that are not + // in the earlier batch, which it definitely should not rerun. if (flags & (ASYNC | BLOCK_EFFECT) && !this.async_deriveds.has(effect)) { this.#maybe_dirty_effects.delete(effect); set_signal_status(effect, DIRTY); @@ -643,9 +732,9 @@ export class Batch { } }; - for (const source of this.current.keys()) { - mark(source); - } + // for (const source of this.current.keys()) { + // mark(source); + // } this.oncommit(() => batch.discard()); batch.#unlink(); @@ -690,18 +779,41 @@ export class Batch { batch = batch.#prev; } - if (!this.is_fork) { - let is_latest_value = true; - batch = this.#next; - while (batch) { - if (batch.current.has(source)) { - is_latest_value = false; - break; + let is_latest_value = true; + batch = this.#next; + while (batch) { + if (source.f & ASYNC) { + const b = batch; + const run = () => { + if (b.mark(source, DIRTY)) { + b.flush(); + } + }; + if (this.is_fork) { + // this.on_fork_commit.set({}, run); // TODO + } else { + queue_micro_task(run); } - batch = batch.#next; } - if (is_latest_value) source.v = value; + if (batch.current.has(source)) { + is_latest_value = false; + } + batch = batch.#next; } + if (is_latest_value && !this.is_fork) source.v = value; + + // if (!this.is_fork) { + // let is_latest_value = true; + // batch = this.#next; + // while (batch) { + // if (batch.current.has(source)) { + // is_latest_value = false; + // break; + // } + // batch = batch.#next; + // } + // if (is_latest_value) source.v = value; + // } } activate() { @@ -1505,6 +1617,8 @@ export function fork(fn) { for (var [source, [value]] of batch.current) { source.v = value; source.wv = increment_write_version(); + // dirty those effects the fork did not see yet, e.g. because a later batch created new branches + batch.mark(source, DIRTY, true); // TODO probably better to only DIRTY on first non-seen derived } // trigger any `$state.eager(...)` expressions with the new state. @@ -1524,6 +1638,18 @@ export function fork(fn) { flush_eager_effects(); }); + let next_batch = batch.next; + while (next_batch) { + for (const [source, [, is_derived]] of batch.current) { + if (!is_derived && !next_batch.current.has(source)) { + if (next_batch.mark(source, DIRTY)) { + next_batch.flush(); + } + } + } + next_batch = next_batch.next; + } + // let next_batch = batch.next; // while (next_batch) { // for (const [effect] of batch.async_deriveds) { @@ -1541,6 +1667,10 @@ export function fork(fn) { // next_batch = next_batch.next; // } + // for (const run of batch.on_fork_commit.values()) { + // run(); + // } + batch.flush(); await settled; }, diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index a5f9231234..5c737191d5 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -181,21 +181,21 @@ export function async_derived(fn, label, location) { var batch = /** @type {Batch} */ (current_batch); - let next_batch = batch.next; - while (next_batch) { - if (next_batch.async_deriveds.has(effect)) { - next_batch.dependent.add(batch); - if (!next_batch.is_fork) { - set_signal_status(effect, DIRTY); // TODO ideally we can find out if we really need to rerun or if all dependencies' values are equal - // TODO same for block effects; ideally one mechanism for both - next_batch.schedule(effect); - const b = next_batch; - queue_micro_task(() => b.flush()); - } - break; // TODO break correct? Don't we need to do the rerun for all of them? - } - next_batch = next_batch.next; - } + // let next_batch = batch.next; + // while (next_batch) { + // if (next_batch.async_deriveds.has(effect)) { + // next_batch.dependent.add(batch); + // if (!next_batch.is_fork) { + // set_signal_status(effect, DIRTY); // TODO ideally we can find out if we really need to rerun or if all dependencies' values are equal + // // TODO same for block effects; ideally one mechanism for both + // next_batch.schedule(effect); + // const b = next_batch; + // queue_micro_task(() => b.flush()); + // } + // break; // TODO break correct? Don't we need to do the rerun for all of them? + // } + // next_batch = next_batch.next; + // } let prev = batch.prev; while (prev) { @@ -283,11 +283,9 @@ export function async_derived(fn, label, location) { } }); - if (DEV) { - // add a flag that lets this be printed as a derived - // when using `$inspect.trace()` - signal.f |= ASYNC; - } + // Besides prod-logic this also helps in DEV to let this be printed + // as a derived when using `$inspect.trace()` + signal.f |= ASYNC; return new Promise((fulfil) => { /** @param {Promise} p */ diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index e69a639a94..3a577c5f17 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -370,7 +370,9 @@ export function legacy_pre_effect_reset() { * @returns {Effect} */ export function async_effect(fn) { - return create_effect(ASYNC | EFFECT_PRESERVED, fn); + const effect = create_effect(ASYNC | EFFECT_PRESERVED, fn); + current_batch?.effects_ran.add(effect); + return effect; } /** @@ -417,6 +419,7 @@ export function block(fn, flags = 0) { if (DEV) { effect.dev_stack = dev_stack; } + current_batch?.effects_ran.add(effect); return effect; } diff --git a/packages/svelte/tests/runtime-runes/samples/async-merge-no-unnecessary-rerun/_config.js b/packages/svelte/tests/runtime-runes/samples/async-merge-no-unnecessary-rerun/_config.js new file mode 100644 index 0000000000..c4ffabc31a --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-merge-no-unnecessary-rerun/_config.js @@ -0,0 +1,38 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target, logs }) { + await tick(); + + const [a, b, pop] = target.querySelectorAll('button'); + + a.click(); + await tick(); + assert.deepEqual(logs, ['a+b 1']); + logs.length = 0; + + b.click(); + await tick(); + assert.deepEqual(logs, ['a+b 2', 'b 1']); + logs.length = 0; + + pop.click(); + await tick(); + assert.deepEqual(logs, []); + + pop.click(); + await tick(); + // At this point the two batches are merged, and no async effects should be rerun + assert.deepEqual(logs, []); + assert.htmlEqual( + target.innerHTML, + ` + + + + 1 + 1 = 2 | 1 + ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-merge-no-unnecessary-rerun/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-merge-no-unnecessary-rerun/main.svelte new file mode 100644 index 0000000000..71656ba78c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-merge-no-unnecessary-rerun/main.svelte @@ -0,0 +1,18 @@ + + + + + + +{a} + {b} = {await push('a+b', a + b)} | {await push('b', b)} diff --git a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-2/_config.js b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-2/_config.js index f4aff83aad..0f422d1797 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-2/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-2/_config.js @@ -2,7 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, // TODO works on https://github.com/sveltejs/svelte/pull/17971 async test({ assert, target }) { await tick(); const [a_b, a_c, b_d, shift, pop] = target.querySelectorAll('button'); diff --git a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-3/_config.js b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-3/_config.js index c9e7513b22..868ae1f54f 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-3/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-3/_config.js @@ -2,7 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, // TODO works on https://github.com/sveltejs/svelte/pull/17971 async test({ assert, target }) { await tick(); const [a_b, a_c, b_d, shift, pop] = target.querySelectorAll('button'); @@ -63,12 +62,12 @@ export default test({ ` ); - pop.click(); // second b resolved, still pending: [b, a] + pop.click(); // second b resolved, still pending: [b, a] but b is now obsolete await tick(); assert.htmlEqual( target.innerHTML, ` - a 0 | b 0 | c 0 | d 0 + a 1 | b 2 | c 0 | d 1 @@ -79,18 +78,6 @@ export default test({ shift.click(); // first b resolved, first + last batch settled, still pending: [a] await tick(); - assert.htmlEqual( - target.innerHTML, - ` - a 1 | b 2 | c 0 | d 1 - - - - - - ` - ); - shift.click(); // all resolved await tick(); assert.htmlEqual( diff --git a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-4/_config.js b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-4/_config.js index 472a3ebcaf..fff23f68ef 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-4/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-4/_config.js @@ -2,7 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, // TODO works on https://github.com/sveltejs/svelte/pull/17971 async test({ assert, target }) { await tick(); const [a_b, a_c, b_d, shift, pop] = target.querySelectorAll('button'); @@ -63,12 +62,12 @@ export default test({ ` ); - pop.click(); // second b resolved, still pending: [b, a] + pop.click(); // second b resolved, still pending: [b, a] with b obsolete so can render some already await tick(); assert.htmlEqual( target.innerHTML, ` - a 0 | b 0 | c 0 | d 0 + a 1 | b 2 | c 0 | d 1 @@ -78,22 +77,6 @@ export default test({ ); pop.click(); // second a resolved, first a/b now obsolete - // TODO would be nice to show final result here already, right now it doesn't because - // we have no handle on the already resolved first a anymore - await tick(); - assert.htmlEqual( - target.innerHTML, - ` - a 0 | b 0 | c 0 | d 0 - - - - - - ` - ); - - shift.click(); // queue empty await tick(); assert.htmlEqual( target.innerHTML, diff --git a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-7/_config.js b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-7/_config.js index b1bf53a2fc..c0b119803b 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-7/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-7/_config.js @@ -32,7 +32,7 @@ export default test({ ` ); - shift.click(); // schedules second step of first batch and schedules rerun of second batch + shift.click(); // schedules second step of first batch and schedules rerun of second batch which can use updated values await tick(); assert.htmlEqual( target.innerHTML, @@ -45,36 +45,8 @@ export default test({ ` ); - // how it's on main - pop.click(); await tick(); - assert.htmlEqual( - target.innerHTML, - ` - a 1 | b 2 | c 0 | d 2 - - - - - ` - ); - - shift.click(); // obsolete second batch promise (already rejected) - await tick(); - assert.htmlEqual( - target.innerHTML, - ` - a 1 | b 2 | c 0 | d 2 - - - - - ` - ); - - shift.click(); // first batch resolves - await tick(); assert.htmlEqual( target.innerHTML, ` @@ -85,45 +57,5 @@ export default test({ ` ); - - // how it's on https://github.com/sveltejs/svelte/pull/17971 - // pop.click(); // second batch resolves but knows it needs to wait on first batch - // await tick(); - // assert.htmlEqual( - // target.innerHTML, - // ` - // a 0 | b 0 | c 0 | d 0 - // - // - // - // - // ` - // ); - - // shift.click(); // obsolete second batch promise (already rejected) - // await tick(); - // assert.htmlEqual( - // target.innerHTML, - // ` - // a 0 | b 0 | c 0 | d 0 - // - // - // - // - // ` - // ); - - // shift.click(); // first batch resolves, with it second can now resolve as well - // await tick(); - // assert.htmlEqual( - // target.innerHTML, - // ` - // a 1 | b 2 | c 1 | d 3 - // - // - // - // - // ` - // ); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-fork-3/_config.js b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-fork-3/_config.js index 61360d8dc9..fd7bc40eed 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-fork-3/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-overlap-multiple-fork-3/_config.js @@ -2,7 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, async test({ assert, target }) { await tick(); const [a_b_fork, a_c, b_d, shift, pop, commit] = target.querySelectorAll('button'); diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-derived-2/_config.js b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-2/_config.js index 8276c5be41..2349936245 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-stale-derived-2/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-2/_config.js @@ -2,8 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, // TODO this one is tricky - async test({ assert, target }) { const [increment, a, b] = target.querySelectorAll('button'); @@ -31,27 +29,27 @@ export default test({ a.click(); await tick(); - assert.htmlEqual( target.innerHTML, ` -

a: 0

+

a: 2

` ); b.click(); await tick(); - + b.click(); + await tick(); assert.htmlEqual( target.innerHTML, ` -

b: 0

b: 3

a + + + + 0 0 bye + ` + ); + + pop.click(); + await tick(); + // The later batch finishes first but should not commit; it overlaps with the earlier batch through a block. + // If we were to commit this batch we would briefly see an if block's truthy branch, which is wrong considering + // the "timeline applied in order" principle. + assert.htmlEqual( + target.innerHTML, + ` + + + + + 0 0 bye + ` + ); + + pop.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + ` + + + + + 1 1 bye + ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-state-block-overlap/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-state-block-overlap/main.svelte new file mode 100644 index 0000000000..1486b4b7ff --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-state-block-overlap/main.svelte @@ -0,0 +1,25 @@ + + + + + + + +{await push(a)} +{await push(b)} + +{#if a < b} + hello +{:else} + bye +{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-1/_config.js b/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-1/_config.js index 737169cb91..e2e5c4ab15 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-1/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-1/_config.js @@ -2,7 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, // TODO works on https://github.com/sveltejs/svelte/pull/17971 async test({ assert, target }) { const [x, y, shift, pop, commit] = target.querySelectorAll('button'); diff --git a/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-4/_config.js b/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-4/_config.js index 6a6399a19e..a20f8c0ba5 100644 --- a/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-4/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/async-state-new-branch-fork-4/_config.js @@ -2,7 +2,6 @@ import { tick } from 'svelte'; import { test } from '../../test'; export default test({ - skip: true, // TODO works on https://github.com/sveltejs/svelte/pull/17971 async test({ assert, target }) { const [x, y, resolve, commit] = target.querySelectorAll('button');