diff --git a/.changeset/beige-sloths-cry.md b/.changeset/beige-sloths-cry.md new file mode 100644 index 000000000000..3d3362819017 --- /dev/null +++ b/.changeset/beige-sloths-cry.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: performance regression affecting deriveds with no dependencies (#17342) diff --git a/.changeset/free-phones-exist.md b/.changeset/free-phones-exist.md new file mode 100644 index 000000000000..a9f0c824eca6 --- /dev/null +++ b/.changeset/free-phones-exist.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: reconnect deriveds inside branch effects diff --git a/packages/svelte/src/internal/client/reactivity/batch.js b/packages/svelte/src/internal/client/reactivity/batch.js index 6f941c7ff231..63667aea1288 100644 --- a/packages/svelte/src/internal/client/reactivity/batch.js +++ b/packages/svelte/src/internal/client/reactivity/batch.js @@ -28,9 +28,9 @@ import { is_dirty, is_updating_effect, set_is_updating_effect, - set_signal_status, update_effect } from '../runtime.js'; +import { set_signal_status } from './status.js'; import * as e from '../errors.js'; import { flush_tasks, queue_micro_task } from '../dom/task.js'; import { DEV } from 'esm-env'; diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index b2d5d59ae198..a44eba34dbbd 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -17,13 +17,13 @@ import { import { active_reaction, active_effect, - set_signal_status, update_reaction, increment_write_version, set_active_effect, push_reaction_value, is_destroying_effect } from '../runtime.js'; +import { set_signal_status, update_derived_status } from './status.js'; import { equals, safe_equals } from './equality.js'; import * as e from '../errors.js'; import * as w from '../warnings.js'; @@ -360,7 +360,10 @@ export function update_derived(derived) { // the underlying value will be updated when the fork is committed. // otherwise, the next time we get here after a 'real world' state // change, `derived.equals` may incorrectly return `true` - if (!current_batch?.is_fork) { + // + // deriveds with no deps should always update `derived.v` + // since they will never change and need the value after fork commits + if (!current_batch?.is_fork || derived.deps === null) { derived.v = value; } @@ -381,8 +384,9 @@ export function update_derived(derived) { if (effect_tracking() || current_batch?.is_fork) { batch_values.set(derived, value); } - } else { - var status = (derived.f & CONNECTED) === 0 ? MAYBE_DIRTY : CLEAN; - set_signal_status(derived, status); + } + + if (batch_values === null || derived.deps === null) { + update_derived_status(derived); } } diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 717fc3500665..6d3bb1487bdf 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -1,4 +1,5 @@ /** @import { ComponentContext, ComponentContextLegacy, Derived, Effect, TemplateNode, TransitionManager } from '#client' */ +import { set_signal_status } from './status.js'; import { is_dirty, active_effect, @@ -9,7 +10,6 @@ import { remove_reactions, set_active_reaction, set_is_destroying_effect, - set_signal_status, untrack, untracking } from '../runtime.js'; diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 3f8d6cb09f15..1094360cb294 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -6,7 +6,6 @@ import { untracked_writes, get, set_untracked_writes, - set_signal_status, untrack, increment_write_version, update_effect, @@ -18,6 +17,7 @@ import { set_is_updating_effect, is_updating_effect } from '../runtime.js'; +import { set_signal_status, update_derived_status } from './status.js'; import { equals, safe_equals } from './equality.js'; import { CLEAN, @@ -218,12 +218,14 @@ export function internal_set(source, value) { } if ((source.f & DERIVED) !== 0) { + const derived = /** @type {Derived} */ (source); + // if we are assigning to a dirty derived we set it to clean/maybe dirty but we also eagerly execute it to track the dependencies if ((source.f & DIRTY) !== 0) { - execute_derived(/** @type {Derived} */ (source)); + execute_derived(derived); } - set_signal_status(source, (source.f & CONNECTED) !== 0 ? CLEAN : MAYBE_DIRTY); + update_derived_status(derived); } source.wv = increment_write_version(); diff --git a/packages/svelte/src/internal/client/reactivity/status.js b/packages/svelte/src/internal/client/reactivity/status.js new file mode 100644 index 000000000000..024285e73a2c --- /dev/null +++ b/packages/svelte/src/internal/client/reactivity/status.js @@ -0,0 +1,25 @@ +/** @import { Derived, Signal } from '#client' */ +import { CLEAN, CONNECTED, DIRTY, MAYBE_DIRTY } from '#client/constants'; + +const STATUS_MASK = ~(DIRTY | MAYBE_DIRTY | CLEAN); + +/** + * @param {Signal} signal + * @param {number} status + */ +export function set_signal_status(signal, status) { + signal.f = (signal.f & STATUS_MASK) | status; +} + +/** + * Set a derived's status to CLEAN or MAYBE_DIRTY based on its connection state. + * @param {Derived} derived + */ +export function update_derived_status(derived) { + // Only mark as MAYBE_DIRTY if disconnected and has dependencies. + if ((derived.f & CONNECTED) !== 0 || derived.deps === null) { + set_signal_status(derived, CLEAN); + } else { + set_signal_status(derived, MAYBE_DIRTY); + } +} diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 64c8409b8ffb..44d9c77367dc 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -28,7 +28,6 @@ import { old_values } from './reactivity/sources.js'; import { destroy_derived_effects, execute_derived, - current_async_effect, recent_async_deriveds, update_derived } from './reactivity/deriveds.js'; @@ -56,6 +55,7 @@ import { handle_error } from './error-handling.js'; import { UNINITIALIZED } from '../../constants.js'; import { captured_signals } from './legacy.js'; import { without_reactive_context } from './dom/elements/bindings/shared.js'; +import { set_signal_status, update_derived_status } from './reactivity/status.js'; export let is_updating_effect = false; @@ -186,12 +186,12 @@ export function is_dirty(reaction) { } if ( - (flags & CONNECTED) !== 0 && + (flags & DERIVED) !== 0 && // During time traveling we don't want to reset the status so that // traversal of the graph in the other batches still happens - batch_values === null + (batch_values === null || reaction.deps === null) ) { - set_signal_status(reaction, CLEAN); + update_derived_status(/** @type {Derived} */ (reaction)); } } @@ -629,7 +629,14 @@ export function get(signal) { update_derived(derived); } - if (is_updating_effect && effect_tracking() && (derived.f & CONNECTED) === 0) { + if ( + (derived.f & CONNECTED) === 0 && + ((is_updating_effect && + (effect_tracking() || + (active_effect !== null && (active_effect.f & BRANCH_EFFECT) !== 0))) || + // evaluating connected parent derived, so reconnect child deriveds too + (active_reaction !== null && (active_reaction.f & CONNECTED) !== 0)) + ) { reconnect(derived); } } @@ -718,17 +725,6 @@ export function untrack(fn) { } } -const STATUS_MASK = ~(DIRTY | MAYBE_DIRTY | CLEAN); - -/** - * @param {Signal} signal - * @param {number} status - * @returns {void} - */ -export function set_signal_status(signal, status) { - signal.f = (signal.f & STATUS_MASK) | status; -} - /** * @param {Record} obj * @param {Array} keys diff --git a/packages/svelte/src/legacy/legacy-client.js b/packages/svelte/src/legacy/legacy-client.js index d4a053d1aab2..ec355eb4d50d 100644 --- a/packages/svelte/src/legacy/legacy-client.js +++ b/packages/svelte/src/legacy/legacy-client.js @@ -3,7 +3,8 @@ import { DIRTY, LEGACY_PROPS, MAYBE_DIRTY } from '../internal/client/constants.j import { user_pre_effect } from '../internal/client/reactivity/effects.js'; import { mutable_source, set } from '../internal/client/reactivity/sources.js'; import { hydrate, mount, unmount } from '../internal/client/render.js'; -import { active_effect, get, set_signal_status } from '../internal/client/runtime.js'; +import { active_effect, get } from '../internal/client/runtime.js'; +import { set_signal_status } from '../internal/client/reactivity/status.js'; import { flushSync } from '../internal/client/reactivity/batch.js'; import { define_property, is_array } from '../internal/shared/utils.js'; import * as e from '../internal/client/errors.js'; diff --git a/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/Child.svelte b/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/Child.svelte new file mode 100644 index 000000000000..fdb0e5fc7b5e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/Child.svelte @@ -0,0 +1,10 @@ + + + +

{y.foo}

diff --git a/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/_config.js b/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/_config.js new file mode 100644 index 000000000000..6da74a4e99dd --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/_config.js @@ -0,0 +1,24 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + skip_no_async: true, + async test({ assert, target }) { + const forkButton = target.querySelector('button'); + + flushSync(() => { + forkButton?.click(); + }); + + const [, clickButton] = target.querySelectorAll('button'); + const p = target.querySelector('p'); + + assert.equal(p?.textContent, '0'); + + flushSync(() => { + clickButton?.click(); + }); + + assert.equal(p?.textContent, '1'); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/main.svelte b/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/main.svelte new file mode 100644 index 000000000000..2b94475d5008 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fork-derived-class-instance/main.svelte @@ -0,0 +1,17 @@ + + + + +{#if x} + +{/if} diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 23c4bb42f99b..51aa508e9fa5 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -1,3 +1,4 @@ +/** @import { Reaction } from '#client' */ import { describe, assert, it } from 'vitest'; import { flushSync } from '../../src/index-client'; import * as $ from '../../src/internal/client/runtime'; @@ -15,9 +16,10 @@ import { proxy } from '../../src/internal/client/proxy'; import { derived } from '../../src/internal/client/reactivity/deriveds'; import { snapshot } from '../../src/internal/shared/clone.js'; import { SvelteSet } from '../../src/reactivity/set'; -import { DESTROYED } from '../../src/internal/client/constants'; +import { CLEAN, CONNECTED, DESTROYED, MAYBE_DIRTY } from '../../src/internal/client/constants'; import { noop } from 'svelte/internal/client'; import { disable_async_mode_flag, enable_async_mode_flag } from '../../src/internal/flags'; +import { branch } from '../../src/internal/client/reactivity/effects'; /** * @param runes runes mode @@ -1493,4 +1495,159 @@ describe('signals', () => { assert.deepEqual(log, ['inner destroyed', 'inner destroyed']); }; }); + + // reproduces conditions leading to https://github.com/sveltejs/kit/issues/15059 + test('derived read inside branch effect should be reconnected even when effect_tracking is false', () => { + let count = state(0); + let d: Derived | null = null; + + render_effect(() => { + branch(() => { + if (!d) { + d = derived(() => $.get(count) * 2); + } + + $.get(d); + }); + }); + + return () => { + flushSync(); + + const isConnected = (d!.f & CONNECTED) !== 0; + assert.ok( + isConnected, + 'derived should be CONNECTED after being read inside branch during effect update' + ); + + const countReactions = count.reactions || []; + assert.ok(countReactions.includes(d!), 'derived should be in source reactions'); + }; + }); + + // Test that deriveds with no dependencies are always CLEAN + test('deriveds with no deps should be CLEAN and not re-evaluate', () => { + let evalCount = 0; + let d: Derived | null = null; + + render_effect(() => { + branch(() => { + if (!d) { + d = derived(() => { + evalCount++; + return 42; + }); + } + + $.get(d); + }); + }); + + return () => { + flushSync(); + + const initialEvalCount = evalCount; + assert.equal(initialEvalCount, 1, 'derived should evaluate once initially'); + + for (let i = 0; i < 100; i++) { + $.get(d!); + } + + assert.equal( + evalCount, + initialEvalCount, + 'derived with no deps should not re-evaluate on subsequent reads' + ); + + const isClean = (d!.f & CLEAN) !== 0; + assert.ok(isClean, 'derived with no deps should be CLEAN'); + }; + }); + + // reproduction of https://github.com/sveltejs/svelte/issues/17352 + test('nested deriveds with parent-child dependencies stay reactive', () => { + const log: any[] = []; + + return () => { + const expanded_ids = new SvelteSet(); + const nodes = proxy([ + { + id: 'folder', + children: [{ id: 'file' }] + }, + { id: 'other' } + ]); + + let items_derived: Derived; + let visible_items_derived: Derived; + + const destroy = effect_root(() => { + items_derived = derived(function create_items( + list: any[] = nodes, + parent?: any, + result: any[] = [] + ) { + for (const node of list) { + const expanded_d = derived(() => expanded_ids.has(node.id)); + const visible_d = derived(() => + parent === undefined ? true : $.get(parent.expanded_d) && $.get(parent.visible_d) + ); + + const item = { + node, + expanded_d, + visible_d, + get expanded() { + return $.get(expanded_d); + }, + get visible() { + return $.get(visible_d); + } + }; + result.push(item); + + if (node.children) { + create_items(node.children, item, result); + } + } + return result; + }); + + visible_items_derived = derived(() => $.get(items_derived).filter((item) => item.visible)); + + render_effect(() => { + log.push($.get(visible_items_derived).length); + }); + }); + + flushSync(); + assert.deepEqual(log, [2], 'initial: folder and other visible'); + + expanded_ids.add('folder'); + flushSync(); + assert.deepEqual(log, [2, 3], 'after expand: folder, file, other visible'); + + expanded_ids.delete('folder'); + flushSync(); + assert.deepEqual(log, [2, 3, 2], 'after collapse'); + + nodes.splice(1, 1); // Remove 'other' + + const snapshot = $.get(visible_items_derived!); + assert.equal(snapshot.length, 1, 'after delete: only folder visible'); + + flushSync(); + assert.deepEqual(log, [2, 3, 2, 1], 'effect ran after delete'); + + expanded_ids.add('folder'); + flushSync(); + assert.deepEqual( + log, + [2, 3, 2, 1, 2], + 'after expand post-delete: folder and file should be visible' + ); + + destroy(); + }; + }); });