diff --git a/.changeset/stupid-chefs-rescue.md b/.changeset/stupid-chefs-rescue.md new file mode 100644 index 0000000000..d21389e97a --- /dev/null +++ b/.changeset/stupid-chefs-rescue.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: avoid traversing clean roots diff --git a/packages/svelte/src/internal/client/dom/blocks/boundary.js b/packages/svelte/src/internal/client/dom/blocks/boundary.js index 8046f1e222..b440bb3ba4 100644 --- a/packages/svelte/src/internal/client/dom/blocks/boundary.js +++ b/packages/svelte/src/internal/client/dom/blocks/boundary.js @@ -271,21 +271,9 @@ export class Boundary { #resolve(batch) { this.is_pending = false; - // any effects that were previously deferred should be rescheduled — - // after the next traversal (which will happen immediately, due to the - // same update that brought us here) the effects will be flushed - for (const e of this.#dirty_effects) { - set_signal_status(e, DIRTY); - batch.schedule(e); - } - - for (const e of this.#maybe_dirty_effects) { - set_signal_status(e, MAYBE_DIRTY); - batch.schedule(e); - } - - this.#dirty_effects.clear(); - this.#maybe_dirty_effects.clear(); + // any effects that were previously deferred should be transferred + // to the batch, which will flush in the next microtask + batch.transfer_effects(this.#dirty_effects, this.#maybe_dirty_effects); } /** diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index ebaed93e9c..3a4872b0b6 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -38,6 +38,8 @@ import { defer_effect } from './utils.js'; import { UNINITIALIZED } from '../../../constants.js'; import { set_signal_status } from './status.js'; import { legacy_is_updating_store } from './store.js'; +import { invariant } from '../../shared/dev.js'; +import { log_effect_tree } from '../dev/debug.js'; /** @type {Set} */ const batches = new Set(); @@ -204,9 +206,25 @@ export class Batch { #process() { if (flush_count++ > 1000) { + batches.delete(this); infinite_loop_guard(); } + // we only reschedule previously-deferred effects if we expect + // to be able to run them after processing the batch + if (!this.#is_deferred()) { + for (const e of this.#dirty_effects) { + this.#maybe_dirty_effects.delete(e); + set_signal_status(e, DIRTY); + this.schedule(e); + } + + for (const e of this.#maybe_dirty_effects) { + set_signal_status(e, MAYBE_DIRTY); + this.schedule(e); + } + } + const roots = this.#roots; this.#roots = []; @@ -396,21 +414,6 @@ export class Batch { is_processing = true; current_batch = this; - // we only reschedule previously-deferred effects if we expect - // to be able to run them after processing the batch - if (!this.#is_deferred()) { - for (const e of this.#dirty_effects) { - this.#maybe_dirty_effects.delete(e); - set_signal_status(e, DIRTY); - this.schedule(e); - } - - for (const e of this.#maybe_dirty_effects) { - set_signal_status(e, MAYBE_DIRTY); - this.schedule(e); - } - } - this.#process(); } finally { flush_count = 0; @@ -470,6 +473,10 @@ export class Batch { // Re-run async/block effects that depend on distinct values changed in both batches var others = [...batch.current.keys()].filter((s) => !this.current.has(s)); if (others.length > 0) { + if (DEV) { + invariant(batch.#roots.length === 0, 'Batch has scheduled roots'); + } + batch.activate(); /** @type {Set} */ @@ -482,6 +489,7 @@ export class Batch { mark_effects(source, others, marked, checked); } + // Only apply and traverse when we know we triggered async work with marking the effects if (batch.#roots.length > 0) { batch.apply(); @@ -489,7 +497,7 @@ export class Batch { batch.#traverse(root, [], []); } - // TODO do we need to do anything with the dummy effect arrays? + batch.#roots = []; } batch.deactivate(); @@ -523,6 +531,23 @@ export class Batch { }); } + /** + * @param {Set} dirty_effects + * @param {Set} maybe_dirty_effects + */ + transfer_effects(dirty_effects, maybe_dirty_effects) { + for (const e of dirty_effects) { + this.#dirty_effects.add(e); + } + + for (const e of maybe_dirty_effects) { + this.#maybe_dirty_effects.add(e); + } + + dirty_effects.clear(); + maybe_dirty_effects.clear(); + } + /** @param {(batch: Batch) => void} fn */ oncommit(fn) { this.#commit_callbacks.add(fn); diff --git a/packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte new file mode 100644 index 0000000000..b39ca3dfd4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/Component.svelte @@ -0,0 +1,11 @@ + \ No newline at end of file diff --git a/packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js new file mode 100644 index 0000000000..083608e9dc --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/_config.js @@ -0,0 +1,16 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + // This test causes two batches to be scheduled such that the same root is traversed multiple times, + // some of the time while it was already marked clean by a previous batch processing. It tests + // that the app stays reactive after, i.e. that the root is not improperly marked as unclean. + await tick(); + const [button] = target.querySelectorAll('button'); + + button.click(); + await tick(); + assert.htmlEqual(target.innerHTML, `

hello

`); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte new file mode 100644 index 0000000000..d6c09803f9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-pending-effect/main.svelte @@ -0,0 +1,19 @@ + + + + + + + + {#snippet pending()} + + {/snippet} + + +{#if condition} +

hello

+{/if}