diff --git a/.changeset/beige-beans-arrive.md b/.changeset/beige-beans-arrive.md new file mode 100644 index 0000000000..1a5a36000c --- /dev/null +++ b/.changeset/beige-beans-arrive.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: reschedule new effects from other branches diff --git a/.changeset/mean-windows-march.md b/.changeset/mean-windows-march.md new file mode 100644 index 0000000000..620a140d36 --- /dev/null +++ b/.changeset/mean-windows-march.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: avoid persisting stale async values diff --git a/packages/svelte/src/internal/client/dom/blocks/branches.js b/packages/svelte/src/internal/client/dom/blocks/branches.js index 33c34f58bb..8127eb3651 100644 --- a/packages/svelte/src/internal/client/dom/blocks/branches.js +++ b/packages/svelte/src/internal/client/dom/blocks/branches.js @@ -109,13 +109,15 @@ export class BranchManager { } for (const [b, k] of this.#batches) { - this.#batches.delete(b); - - if (b === batch) { + if (b.id > batch.id) { // keep values for newer batches - break; + continue; } + this.#batches.delete(b); + + if (b === batch) continue; + const offscreen = this.#offscreen.get(k); if (offscreen) { diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 8f14cb4437..07d3fe49c7 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -92,6 +92,21 @@ let uid = 1; export class Batch { id = uid++; + /** + * Effects this batch has seen already or that were created before it (and there it is able to know about them). + * Used to filter out #new_effects of other batches during #commit. + */ + #seen_effects = new Set(); + + constructor() { + // This batch doesn't care about new batches before it was created; it knows about these already + for (const batch of batches) { + for (const e of batch.#new_effects) { + this.#seen_effects.add(e); + } + } + } + /** True as soon as `#process()` was called */ #started = false; @@ -206,24 +221,20 @@ export class Batch { #is_blocked() { for (const batch of this.#blockers) { - for (const effect of batch.#blocking_pending.keys()) { + outer: for (const effect of batch.#blocking_pending.keys()) { if (this.unblocked.has(effect)) continue; - var skipped = false; var e = effect; while (e.parent !== null) { if (this.#skipped_branches.has(e)) { - skipped = true; - break; + continue outer; } e = e.parent; } - if (!skipped) { - return true; - } + return true; } } @@ -415,6 +426,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.#seen_effects.add(effect); // This effect was definitely touched by this batch now if ((flags & BLOCK_EFFECT) !== 0) this.#maybe_dirty_effects.add(effect); update_effect(effect); } @@ -521,7 +533,9 @@ export class Batch { * @param {Effect} effect */ register_created_effect(effect) { - this.#new_effects.push(effect); + if ((effect.f & (EAGER_EFFECT | BRANCH_EFFECT)) === 0) { + this.#new_effects.push(effect); + } } #commit() { @@ -534,6 +548,44 @@ export class Batch { /** @type {Source[]} */ var sources = []; + /** @type {Map} */ + var checked = new Map(); + + var scheduled = false; + + var current_unequal = [...batch.current] + .filter(([k, c]) => { + const current = this.current.get(k); + // unequal if value is different and it's not a derived in both + return current ? current[0] !== c[0] && (!current[1] || !c[1]) : true; + }) + .map(([k]) => k); + + if (current_unequal.length > 0) { + // New effects created in this batch another batch doesn't know about yet + // need to be checked against the sources that are changing in this batch, + // as they might need to be rescheduled now. If we don't do this, we can + // end up with stale values in the UI. + for (const effect of this.#new_effects) { + if ( + !batch.#seen_effects.has(effect) && + (effect.f & (INERT | EAGER_EFFECT)) === 0 && + depends_on(effect, current_unequal, checked) + ) { + if ((effect.f & (ASYNC | BLOCK_EFFECT)) !== 0) { + scheduled = true; + set_signal_status(effect, DIRTY); + batch.schedule(effect); + } else { + // TODO this isn't quite right, maybe the source is different + // but the effect is only reading it indirectly through a derived + // that didn't change, so it doesn't need to re-run. + batch.#dirty_effects.add(effect); + } + // TODO skipped branches needs to be taken into account here? + } + } + } for (const [source, [value, is_derived]] of this.current) { if (batch.current.has(source)) { @@ -562,8 +614,8 @@ export class Batch { // this batch is now obsolete and can be discarded batch.discard(); } - } else if (sources.length > 0) { - if (DEV) { + } else if (sources.length > 0 || scheduled) { + if (DEV && !scheduled) { invariant(batch.#roots.length === 0, 'Batch has scheduled roots'); } @@ -585,36 +637,12 @@ export class Batch { /** @type {Set} */ var marked = new Set(); - /** @type {Map} */ - var checked = new Map(); + checked = new Map(); for (var source of sources) { mark_effects(source, others, marked, checked); } - checked = new Map(); - var current_unequal = [...batch.current.keys()].filter((c) => - this.current.has(c) - ? /** @type {[any, boolean]} */ (this.current.get(c))[0] !== c.v - : true - ); - - if (current_unequal.length > 0) { - for (const effect of this.#new_effects) { - if ( - (effect.f & (DESTROYED | INERT | EAGER_EFFECT)) === 0 && - depends_on(effect, current_unequal, checked) - ) { - if ((effect.f & (ASYNC | BLOCK_EFFECT)) !== 0) { - set_signal_status(effect, DIRTY); - batch.schedule(effect); - } else { - batch.#dirty_effects.add(effect); - } - } - } - } - // Only apply and traverse when we know we triggered async work with marking the effects if (batch.#roots.length > 0) { batch.apply(); @@ -1024,6 +1052,9 @@ function mark_effects(value, sources, marked, checked) { (flags & DIRTY) === 0 && depends_on(reaction, sources, checked) ) { + // TODO this isn't quite right, maybe the source is different + // but the effect is only reading it indirectly through a derived + // that didn't change, so it doesn't need to re-run. set_signal_status(reaction, DIRTY); schedule_effect(/** @type {Effect} */ (reaction)); } diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 794da4f2b0..3281ad1fba 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -47,6 +47,7 @@ import { batch_values, current_batch, previous_batch } from './batch.js'; import { increment_pending, unset_context } from './async.js'; import { deferred, includes, noop } from '../../shared/utils.js'; import { set_signal_status, update_derived_status } from './status.js'; +import { queue_micro_task } from '../dom/task.js'; /** * This allows us to track 'reactivity loss' that occurs when signals @@ -125,7 +126,7 @@ export function async_derived(fn, label, location) { // only suspend in async deriveds created on initialisation var should_suspend = !active_reaction; - /** @type {Map>>} */ + /** @type {Map> & {temp?: true}>} */ var deferreds = new Map(); async_effect(() => { @@ -135,7 +136,7 @@ export function async_derived(fn, label, location) { reactivity_loss_tracker = { effect, effect_deps: new Set(), warned: false }; } - /** @type {ReturnType>} */ + /** @type {ReturnType> & {temp?: true}} */ var d = deferred(); promise = d.promise; @@ -226,17 +227,28 @@ export function async_derived(fn, label, location) { signal.f ^= ERROR_VALUE; } + const v = signal.v; internal_set(signal, value); + if (d.temp) signal.v = v; // All prior async derived runs are now stale for (const [b, d] of deferreds) { if (b.id < batch.id) { - // Don't delete + resolve directly, instead only do that once - // the current batch commits. This way we avoid tearing when + // Don't resolve directly, instead only do that once the + // current batch commits. This way we avoid tearing when // `b` is rendering through the early resolve while `batch` is // still pending. batch.unblocked.add(effect); + if (parent !== null) { + // Terrible hack: this way we unblock ourselves from `flatten` + // which can block us on initial run + batch.unblocked.add(parent); + } batch.oncommit(() => d.resolve(value)); + // We don't want the value to be written to the underlying source because + // else it will be considered the latest value even thought it's outdated. + // This can happen if the earlier derived resolves before this batch commits. + d.temp = true; } } diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js new file mode 100644 index 0000000000..fd0fc0de48 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/_config.js @@ -0,0 +1,20 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + const [increment, pop] = target.querySelectorAll('button'); + + increment.click(); + await tick(); + increment.click(); + await tick(); + pop.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ' 2 2 1'); // showing nothing here yet would also be ok + + pop.click(); + await tick(); + assert.htmlEqual(target.innerHTML, ' 2 2 1'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte new file mode 100644 index 0000000000..42b7206b56 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-stale-derived-8/main.svelte @@ -0,0 +1,23 @@ + + + + + + +{#if count > 0} + {await push(count)} {count} {other} +{/if}