diff --git a/.changeset/angry-ideas-listen.md b/.changeset/angry-ideas-listen.md new file mode 100644 index 0000000000..cb2bf00d1c --- /dev/null +++ b/.changeset/angry-ideas-listen.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +chore: proactively defer effects in pending boundary diff --git a/.changeset/volatile-each-key.md b/.changeset/volatile-each-key.md new file mode 100644 index 0000000000..674bce9bec --- /dev/null +++ b/.changeset/volatile-each-key.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: detect and error on non-idempotent each block keys in dev mode diff --git a/documentation/docs/98-reference/.generated/client-errors.md b/documentation/docs/98-reference/.generated/client-errors.md index 8601a728a7..7fccac5808 100644 --- a/documentation/docs/98-reference/.generated/client-errors.md +++ b/documentation/docs/98-reference/.generated/client-errors.md @@ -62,6 +62,14 @@ Keyed each block has duplicate key at indexes %a% and %b% Keyed each block has duplicate key `%value%` at indexes %a% and %b% ``` +### each_key_volatile + +``` +Keyed each block has key that is not idempotent — the key for item at index %index% was `%a%` but is now `%b%`. Keys must be the same each time for a given item +``` + +The key expression in a keyed each block must return the same value when called multiple times for the same item. Using expressions like `[item.a, item.b]` creates a new array each time, which will never be equal to itself. Instead, use a primitive value or create a stable key like `item.a + '-' + item.b`. + ### effect_in_teardown ``` diff --git a/packages/svelte/messages/client-errors/errors.md b/packages/svelte/messages/client-errors/errors.md index bedf6db0a5..3f20cb989d 100644 --- a/packages/svelte/messages/client-errors/errors.md +++ b/packages/svelte/messages/client-errors/errors.md @@ -42,6 +42,12 @@ See the [migration guide](/docs/svelte/v5-migration-guide#Components-are-no-long > Keyed each block has duplicate key `%value%` at indexes %a% and %b% +## each_key_volatile + +> Keyed each block has key that is not idempotent — the key for item at index %index% was `%a%` but is now `%b%`. Keys must be the same each time for a given item + +The key expression in a keyed each block must return the same value when called multiple times for the same item. Using expressions like `[item.a, item.b]` creates a new array each time, which will never be equal to itself. Instead, use a primitive value or create a stable key like `item.a + '-' + item.b`. + ## effect_in_teardown > `%rune%` cannot be used inside an effect cleanup function diff --git a/packages/svelte/src/internal/client/dom/blocks/async.js b/packages/svelte/src/internal/client/dom/blocks/async.js index e8c9cf0643..43af3d8dd3 100644 --- a/packages/svelte/src/internal/client/dom/blocks/async.js +++ b/packages/svelte/src/internal/client/dom/blocks/async.js @@ -1,6 +1,5 @@ /** @import { Blocker, TemplateNode, Value } from '#client' */ -import { flatten } from '../../reactivity/async.js'; -import { Batch, current_batch } from '../../reactivity/batch.js'; +import { flatten, increment_pending } from '../../reactivity/async.js'; import { get } from '../../runtime.js'; import { hydrate_next, @@ -10,7 +9,6 @@ import { set_hydrating, skip_nodes } from '../hydration.js'; -import { get_boundary } from './boundary.js'; /** * @param {TemplateNode} node @@ -44,12 +42,7 @@ export function async(node, blockers = [], expressions = [], fn) { return; } - var boundary = get_boundary(); - var batch = /** @type {Batch} */ (current_batch); - var blocking = boundary.is_rendered(); - - boundary.update_pending_count(1); - batch.increment(blocking); + const decrement_pending = increment_pending(); if (was_hydrating) { var previous_hydrate_node = hydrate_node; @@ -72,8 +65,7 @@ export function async(node, blockers = [], expressions = [], fn) { set_hydrating(false); } - boundary.update_pending_count(-1); - batch.decrement(blocking); + decrement_pending(); } }); } diff --git a/packages/svelte/src/internal/client/dom/blocks/boundary.js b/packages/svelte/src/internal/client/dom/blocks/boundary.js index 419e8dc201..0101b033eb 100644 --- a/packages/svelte/src/internal/client/dom/blocks/boundary.js +++ b/packages/svelte/src/internal/client/dom/blocks/boundary.js @@ -1,7 +1,6 @@ /** @import { Effect, Source, TemplateNode, } from '#client' */ import { BOUNDARY_EFFECT, - COMMENT_NODE, DIRTY, EFFECT_PRESERVED, EFFECT_TRANSPARENT, @@ -206,7 +205,7 @@ export class Boundary { this.#pending_effect = null; }); - this.is_pending = false; + this.#resolve(); } }); } @@ -228,13 +227,33 @@ export class Boundary { const pending = /** @type {(anchor: Node) => void} */ (this.#props.pending); this.#pending_effect = branch(() => pending(this.#anchor)); } else { - this.is_pending = false; + this.#resolve(); } } catch (error) { this.error(error); } } + #resolve() { + 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); + schedule_effect(e); + } + + for (const e of this.#maybe_dirty_effects) { + set_signal_status(e, MAYBE_DIRTY); + schedule_effect(e); + } + + this.#dirty_effects.clear(); + this.#maybe_dirty_effects.clear(); + } + /** * Defer an effect inside a pending boundary until the boundary resolves * @param {Effect} effect @@ -280,27 +299,6 @@ export class Boundary { } } - #reschedule_deferred_effects() { - this.is_pending = false; - - // any effects that were encountered and deferred during traversal - // 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); - schedule_effect(e); - } - - for (const e of this.#maybe_dirty_effects) { - set_signal_status(e, MAYBE_DIRTY); - schedule_effect(e); - } - - this.#dirty_effects.clear(); - this.#maybe_dirty_effects.clear(); - } - /** * Updates the pending count associated with the currently visible pending snippet, * if any, such that we can replace the snippet with content once work is done @@ -319,7 +317,7 @@ export class Boundary { this.#pending_count += d; if (this.#pending_count === 0) { - this.#reschedule_deferred_effects(); + this.#resolve(); if (this.#pending_effect) { pause_effect(this.#pending_effect, () => { @@ -460,10 +458,6 @@ export class Boundary { } } -export function get_boundary() { - return /** @type {Boundary} */ (/** @type {Effect} */ (active_effect).b); -} - export function pending() { if (active_effect === null) { e.effect_pending_outside_reaction(); diff --git a/packages/svelte/src/internal/client/dom/blocks/each.js b/packages/svelte/src/internal/client/dom/blocks/each.js index 25f7cf91eb..7ae02d073c 100644 --- a/packages/svelte/src/internal/client/dom/blocks/each.js +++ b/packages/svelte/src/internal/client/dom/blocks/each.js @@ -250,6 +250,14 @@ export function each(node, flags, get_collection, get_key, render_fn, fallback_f var value = array[index]; var key = get_key(value, index); + if (DEV) { + // Check that the key function is idempotent (returns the same value when called twice) + var key_again = get_key(value, index); + if (key !== key_again) { + e.each_key_volatile(String(index), String(key), String(key_again)); + } + } + var item = first_run ? null : items.get(key); if (item) { diff --git a/packages/svelte/src/internal/client/errors.js b/packages/svelte/src/internal/client/errors.js index 34f1d85540..d60c2dd280 100644 --- a/packages/svelte/src/internal/client/errors.js +++ b/packages/svelte/src/internal/client/errors.js @@ -147,6 +147,25 @@ export function each_key_duplicate(a, b, value) { } } +/** + * Keyed each block has key that is not idempotent — the key for item at index %index% was `%a%` but is now `%b%`. Keys must be the same each time for a given item + * @param {string} index + * @param {string} a + * @param {string} b + * @returns {never} + */ +export function each_key_volatile(index, a, b) { + if (DEV) { + const error = new Error(`each_key_volatile\nKeyed each block has key that is not idempotent — the key for item at index ${index} was \`${a}\` but is now \`${b}\`. Keys must be the same each time for a given item\nhttps://svelte.dev/e/each_key_volatile`); + + error.name = 'Svelte error'; + + throw error; + } else { + throw new Error(`https://svelte.dev/e/each_key_volatile`); + } +} + /** * `%rune%` cannot be used inside an effect cleanup function * @param {string} rune diff --git a/packages/svelte/src/internal/client/reactivity/async.js b/packages/svelte/src/internal/client/reactivity/async.js index b6eba3bf8a..b3c5248179 100644 --- a/packages/svelte/src/internal/client/reactivity/async.js +++ b/packages/svelte/src/internal/client/reactivity/async.js @@ -8,7 +8,7 @@ import { set_component_context, set_dev_stack } from '../context.js'; -import { get_boundary } from '../dom/blocks/boundary.js'; +import { Boundary } from '../dom/blocks/boundary.js'; import { invoke_error_boundary } from '../error-handling.js'; import { active_effect, @@ -224,12 +224,7 @@ export function unset_context() { export function run(thunks) { const restore = capture(); - var boundary = get_boundary(); - var batch = /** @type {Batch} */ (current_batch); - var blocking = boundary.is_rendered(); - - boundary.update_pending_count(1); - batch.increment(blocking); + const decrement_pending = increment_pending(); var active = /** @type {Effect} */ (active_effect); @@ -286,10 +281,7 @@ export function run(thunks) { // wait one more tick, so that template effects are // guaranteed to run before `$effect(...)` .then(() => Promise.resolve()) - .finally(() => { - boundary.update_pending_count(-1); - batch.decrement(blocking); - }); + .finally(decrement_pending); return blockers; } @@ -300,3 +292,17 @@ export function run(thunks) { export function wait(blockers) { return Promise.all(blockers.map((b) => b.promise)); } + +export function increment_pending() { + var boundary = /** @type {Boundary} */ (/** @type {Effect} */ (active_effect).b); + var batch = /** @type {Batch} */ (current_batch); + var blocking = boundary.is_rendered(); + + boundary.update_pending_count(1); + batch.increment(blocking); + + return () => { + boundary.update_pending_count(-1); + batch.decrement(blocking); + }; +} diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index b382a4e3a5..297049fd6b 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -18,7 +18,8 @@ import { EAGER_EFFECT, HEAD_EFFECT, ERROR_VALUE, - MANAGED_EFFECT + MANAGED_EFFECT, + REACTION_RAN } from '#client/constants'; import { async_mode_flag } from '../../flags/index.js'; import { deferred, define_property, includes } from '../../shared/utils.js'; @@ -142,7 +143,7 @@ export class Batch { #decrement_queued = false; - is_deferred() { + #is_deferred() { return this.is_fork || this.#blocking_pending > 0; } @@ -202,7 +203,7 @@ export class Batch { // log_inconsistent_branches(root); } - if (this.is_deferred()) { + if (this.#is_deferred()) { this.#defer_effects(render_effects); this.#defer_effects(effects); @@ -246,9 +247,6 @@ export class Batch { var effect = root.first; - /** @type {Effect | null} */ - var pending_boundary = null; - while (effect !== null) { var flags = effect.f; var is_branch = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) !== 0; @@ -256,26 +254,9 @@ export class Batch { var skip = is_skippable_branch || (flags & INERT) !== 0 || this.#skipped_branches.has(effect); - // Inside a `` with a pending snippet, - // all effects are deferred until the boundary resolves - // (except block/async effects, which run immediately) - if ( - async_mode_flag && - pending_boundary === null && - (flags & BOUNDARY_EFFECT) !== 0 && - effect.b?.is_pending - ) { - pending_boundary = effect; - } - if (!skip && effect.fn !== null) { if (is_branch) { effect.f ^= CLEAN; - } else if ( - pending_boundary !== null && - (flags & (EFFECT | RENDER_EFFECT | MANAGED_EFFECT)) !== 0 - ) { - /** @type {Boundary} */ (pending_boundary.b).defer_effect(effect); } else if ((flags & EFFECT) !== 0) { effects.push(effect); } else if (async_mode_flag && (flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0) { @@ -294,10 +275,6 @@ export class Batch { } while (effect !== null) { - if (effect === pending_boundary) { - pending_boundary = null; - } - var next = effect.next; if (next !== null) { @@ -475,7 +452,7 @@ export class Batch { queue_micro_task(() => { this.#decrement_queued = false; - if (!this.is_deferred()) { + if (!this.#is_deferred()) { // we only reschedule previously-deferred effects if we expect // to be able to run them after processing the batch this.revive(); @@ -839,6 +816,19 @@ function depends_on(reaction, sources, checked) { export function schedule_effect(signal) { var effect = (last_scheduled_effect = signal); + var boundary = effect.b; + + // defer render effects inside a pending boundary + // TODO the `REACTION_RAN` check is only necessary because of legacy `$:` effects AFAICT — we can remove later + if ( + boundary?.is_pending && + (signal.f & (EFFECT | RENDER_EFFECT | MANAGED_EFFECT)) !== 0 && + (signal.f & REACTION_RAN) === 0 + ) { + boundary.defer_effect(signal); + return; + } + while (effect.parent !== null) { effect = effect.parent; var flags = effect.f; @@ -850,13 +840,18 @@ export function schedule_effect(signal) { is_flushing && effect === active_effect && (flags & BLOCK_EFFECT) !== 0 && - (flags & HEAD_EFFECT) === 0 + (flags & HEAD_EFFECT) === 0 && + (flags & REACTION_RAN) !== 0 ) { return; } if ((flags & (ROOT_EFFECT | BRANCH_EFFECT)) !== 0) { - if ((flags & CLEAN) === 0) return; + if ((flags & CLEAN) === 0) { + // branch is already dirty, bail + return; + } + effect.f ^= CLEAN; } } diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index d11854fc91..80da5528c8 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -40,7 +40,7 @@ import { Boundary } from '../dom/blocks/boundary.js'; import { component_context } from '../context.js'; import { UNINITIALIZED } from '../../../constants.js'; import { batch_values, current_batch } from './batch.js'; -import { unset_context } from './async.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'; @@ -111,8 +111,6 @@ export function async_derived(fn, label, location) { e.async_derived_orphan(); } - var boundary = /** @type {Boundary} */ (parent.b); - var promise = /** @type {Promise} */ (/** @type {unknown} */ (undefined)); var signal = source(/** @type {V} */ (UNINITIALIZED)); @@ -156,10 +154,7 @@ export function async_derived(fn, label, location) { var batch = /** @type {Batch} */ (current_batch); if (should_suspend) { - var blocking = boundary.is_rendered(); - - boundary.update_pending_count(1); - batch.increment(blocking); + var decrement_pending = increment_pending(); deferreds.get(batch)?.reject(STALE_REACTION); deferreds.delete(batch); // delete to ensure correct order in Map iteration below @@ -208,9 +203,8 @@ export function async_derived(fn, label, location) { } } - if (should_suspend) { - boundary.update_pending_count(-1); - batch.decrement(blocking); + if (decrement_pending) { + decrement_pending(); } }; diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 512c435a27..157587e218 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -40,8 +40,8 @@ import { DEV } from 'esm-env'; import { define_property } from '../../shared/utils.js'; import { get_next_sibling } from '../dom/operations.js'; import { component_context, dev_current_component_function, dev_stack } from '../context.js'; -import { Batch, current_batch, schedule_effect } from './batch.js'; -import { flatten } from './async.js'; +import { Batch, schedule_effect } from './batch.js'; +import { flatten, increment_pending } from './async.js'; import { without_reactive_context } from '../dom/elements/bindings/shared.js'; import { set_signal_status } from './status.js'; @@ -376,14 +376,16 @@ export function template_effect(fn, sync = [], async = [], blockers = []) { * @param {Blocker[]} blockers */ export function deferred_template_effect(fn, sync = [], async = [], blockers = []) { - var batch = /** @type {Batch} */ (current_batch); - var is_async = async.length > 0 || blockers.length > 0; - - if (is_async) batch.increment(true); + if (async.length > 0 || blockers.length > 0) { + var decrement_pending = increment_pending(); + } flatten(blockers, sync, async, (values) => { create_effect(EFFECT, () => fn(...values.map(get)), false); - if (is_async) batch.decrement(true); + + if (decrement_pending) { + decrement_pending(); + } }); } diff --git a/packages/svelte/tests/runtime-runes/samples/each-key-volatile/_config.js b/packages/svelte/tests/runtime-runes/samples/each-key-volatile/_config.js new file mode 100644 index 0000000000..87d4bf45c7 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/each-key-volatile/_config.js @@ -0,0 +1,11 @@ +import { test } from '../../test'; + +export default test({ + compileOptions: { + dev: true + }, + + mode: ['client'], + + error: 'each_key_volatile' +}); diff --git a/packages/svelte/tests/runtime-runes/samples/each-key-volatile/main.svelte b/packages/svelte/tests/runtime-runes/samples/each-key-volatile/main.svelte new file mode 100644 index 0000000000..689c257cfa --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/each-key-volatile/main.svelte @@ -0,0 +1,10 @@ + + +{#each things as thing ([thing.group, thing.id])} +

{thing.group}-{thing.id}

+{/each}