diff --git a/.changeset/easy-singers-retire.md b/.changeset/easy-singers-retire.md new file mode 100644 index 0000000000..4420286e13 --- /dev/null +++ b/.changeset/easy-singers-retire.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: don't rebase just-created batches diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 7adf3be00c..4239cda04b 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -342,6 +342,14 @@ export class Batch { this.#deferred?.resolve(); } + // Order matters here - we need to commit and THEN continue flushing new batches, not the other way around, + // else we could start flushing a new batch and then, if it has pending work, rebase it right afterwards, which is wrong. + // In sync mode flushSync can cause #commit to wrongfully think that there needs to be a rebase, so we only do it in async mode + // TODO fix the underlying cause, otherwise this will likely regress when non-async mode is removed + if (async_mode_flag && !batches.has(this)) { + this.#commit(); + } + var next_batch = /** @type {Batch | null} */ (/** @type {unknown} */ (current_batch)); // Edge case: During traversal new branches might create effects that run immediately and set state, @@ -363,12 +371,6 @@ export class Batch { next_batch.#process(); } - - // In sync mode flushSync can cause #commit to wrongfully think that there needs to be a rebase, so we only do it in async mode - // TODO fix the underlying cause, otherwise this will likely regress when non-async mode is removed - if (async_mode_flag && !batches.has(this)) { - this.#commit(); - } } /** @@ -575,19 +577,23 @@ export class Batch { checked = new Map(); var current_unequal = [...batch.current.keys()].filter((c) => - this.current.has(c) ? /** @type {[any, boolean]} */ (this.current.get(c))[0] !== c : true + this.current.has(c) + ? /** @type {[any, boolean]} */ (this.current.get(c))[0] !== c.v + : true ); - 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); + 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); + } } } } diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 5af51449ad..4ae49fecba 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -43,7 +43,7 @@ import { get_error } from '../../shared/dev.js'; import { async_mode_flag, tracing_mode_flag } from '../../flags/index.js'; import { component_context } from '../context.js'; import { UNINITIALIZED } from '../../../constants.js'; -import { batch_values, current_batch } from './batch.js'; +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'; @@ -399,7 +399,14 @@ export function update_derived(derived) { // change, `derived.equals` may incorrectly return `true` if (!current_batch?.is_fork || derived.deps === null) { if (current_batch !== null) { + // We also write to previous_batch because if it exists, it is a sign that we're + // currently in the process of flushing effects. These updates to deriveds may belong + // to the previous batch, not the new one (which can already exist if an earlier + // effect wrote to a source). This can cause bugs when running batch.#commit() later, + // but not adding it to current_batch can, too, so we add it to both. + // See https://github.com/sveltejs/svelte/pull/18117 for more details. current_batch.capture(derived, value, true); + previous_batch?.capture(derived, value, true); } else { derived.v = value; } diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/_config.js b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/_config.js new file mode 100644 index 0000000000..fb6f3388c9 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/_config.js @@ -0,0 +1,27 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(), +// rescheduling an effect on the new batch that shouldn't run. +export default test({ + async test({ assert, target, logs }) { + await tick(); + const [increment, resolve] = target.querySelectorAll('button'); + + increment.click(); + await tick(); + assert.deepEqual(logs, []); + + // This resolve + // - shouldn't result in the derived execution capturing the new derived value on the new batch, but on the previous batch which is currently flushing + // - shouldn't result in #commit() rebasing the new batch + resolve.click(); + await tick(); + assert.deepEqual(logs, [2]); + + // As a result, this resolve shouldn't result in another execution of the effect depending on the derived + resolve.click(); + await tick(); + assert.deepEqual(logs, [2]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/main.svelte new file mode 100644 index 0000000000..af470363bf --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-1/main.svelte @@ -0,0 +1,32 @@ + + + + + +{#if count} + + + {(() => { + $effect(() => { + count_mirror = count; + }) + })()} + + {(() => { + $effect(() => { + console.log(double); + }) + })()} +{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/_config.js b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/_config.js new file mode 100644 index 0000000000..d8a86f77da --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/_config.js @@ -0,0 +1,25 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(), +// rescheduling an effect on the new batch that shouldn't run. +export default test({ + async test({ assert, target, logs }) { + await tick(); + const [increment, resolve] = target.querySelectorAll('button'); + assert.deepEqual(logs, ['delay 0']); + + increment.click(); + await tick(); + assert.deepEqual(logs, ['delay 0', 'delay 2']); + + // This resolve should trigger the async effect only once + resolve.click(); + await tick(); + assert.deepEqual(logs, ['delay 0', 'delay 2', 'effect run', 'delay 4']); + + resolve.click(); + await tick(); + assert.deepEqual(logs, ['delay 0', 'delay 2', 'effect run', 'delay 4']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/main.svelte new file mode 100644 index 0000000000..fc90ae2ba4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-2/main.svelte @@ -0,0 +1,29 @@ + + + + +{await delay(a + b + c)} diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/_config.js b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/_config.js new file mode 100644 index 0000000000..b430e408c7 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/_config.js @@ -0,0 +1,31 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(), +// rescheduling an effect on the new batch that shouldn't run. +export default test({ + async test({ assert, target, logs }) { + await tick(); + const [increment, shift, pop] = target.querySelectorAll('button'); + + increment.click(); + await tick(); + assert.deepEqual(logs, []); + + // Resolve the blocking await which shouldn't result in the derived execution capturing + // the new derived value on the new batch, but on the previous batch which is currently flushing + pop.click(); + await tick(); + assert.deepEqual(logs, [2]); + + // Resolve the non-blocking await which shouldn't result in #commit() rebasing the new batch + shift.click(); + await tick(); + assert.deepEqual(logs, [2]); + + // Resolve the new batch's await + shift.click(); + await tick(); + assert.deepEqual(logs, [2]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/main.svelte new file mode 100644 index 0000000000..9dec14cd13 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-3/main.svelte @@ -0,0 +1,37 @@ + + + + + + +{#if count} + + {await delay(count)} + {#snippet pending()}loading{/snippet} + + + + {(() => { + $effect(() => { + count_mirror = count; + }) + })()} + + {(() => { + $effect(() => { + console.log(double); + }) + })()} +{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/_config.js b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/_config.js new file mode 100644 index 0000000000..804c1f53bb --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/_config.js @@ -0,0 +1,58 @@ +import { tick } from 'svelte'; +import { test } from '../../test'; + +// Tests that a newly created batch during an effect flush isn't rebased right away by the previous batch.#commit(), +// rescheduling an effect on the new batch that shouldn't run. +export default test({ + async test({ assert, target, logs }) { + await tick(); + const [increment, unrelated, resolve] = target.querySelectorAll('button'); + + increment.click(); + await tick(); + assert.deepEqual(logs, []); + + // This resolve + // - shouldn't result in the derived execution capturing the new derived value on the new batch, but on the previous batch which is currently flushing + // - shouldn't result in #commit() rebasing the new batch + resolve.click(); + await tick(); + assert.deepEqual(logs, [2]); + assert.htmlEqual( + target.innerHTML, + ` + + + + ` + ); + + // This resolve + // - shouldn't result in the derived execution capturing the new derived value on the new batch, but on the previous batch which is currently flushing + // - shouldn't result in #commit() rebasing the new batch + unrelated.click(); + await tick(); + assert.deepEqual(logs, [2]); + assert.htmlEqual( + target.innerHTML, + ` + + + + ` + ); + + // As a result, this resolve shouldn't result in another execution of the effect depending on the derived + resolve.click(); + await tick(); + assert.deepEqual(logs, [2]); + assert.htmlEqual( + target.innerHTML, + ` + + + + ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/main.svelte b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/main.svelte new file mode 100644 index 0000000000..fdc2447e3e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/async-dont-rebase-new-batch-4/main.svelte @@ -0,0 +1,38 @@ + + + + + + +{#if count} + + + {(() => { + $effect(() => { + count_mirror = count; + untrack(() => count_mirror_d); // execute derived; should associate value with the right batch + }) + })()} + + {(() => { + $effect(() => { + console.log(double); + }) + })()} +{/if} \ No newline at end of file