From f6a489e172a1634491109dc7c5a63ae548ecadfb Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 09:49:20 +0000 Subject: [PATCH 01/10] fix: improve derived connection to ownership graph --- .changeset/blue-rocks-play.md | 5 ++ .../internal/client/reactivity/deriveds.js | 2 +- .../svelte/src/internal/client/runtime.js | 17 ++-- packages/svelte/tests/signals/test.ts | 90 +++++++++++++++---- 4 files changed, 87 insertions(+), 27 deletions(-) create mode 100644 .changeset/blue-rocks-play.md diff --git a/.changeset/blue-rocks-play.md b/.changeset/blue-rocks-play.md new file mode 100644 index 000000000000..31e99911482c --- /dev/null +++ b/.changeset/blue-rocks-play.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: improve derived connection to ownership graph diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index e86963408ce7..f180a6999574 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -87,7 +87,7 @@ export function derived_safe_equal(fn) { * @param {Derived} derived * @returns {void} */ -function destroy_derived_children(derived) { +export function destroy_derived_children(derived) { var children = derived.children; if (children !== null) { diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 859925186f59..87d6b589d7fd 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -28,7 +28,12 @@ import { } from './constants.js'; import { flush_tasks } from './dom/task.js'; import { internal_set, set } from './reactivity/sources.js'; -import { destroy_derived, execute_derived, update_derived } from './reactivity/deriveds.js'; +import { + destroy_derived, + destroy_derived_children, + execute_derived, + update_derived +} from './reactivity/deriveds.js'; import * as e from './errors.js'; import { FILENAME } from '../../constants.js'; import { legacy_mode_flag, tracing_mode_flag } from '../flags/index.js'; @@ -517,6 +522,8 @@ function remove_reaction(signal, dependency) { if ((dependency.f & (UNOWNED | DISCONNECTED)) === 0) { dependency.f ^= DISCONNECTED; } + // Disconnect any reactions owned by this reaction + destroy_derived_children(/** @type {Derived} **/ (dependency)); remove_reactions(/** @type {Derived} **/ (dependency), 0); } } @@ -934,13 +941,7 @@ export function get(signal) { new_deps.push(signal); } } - } - - if ( - is_derived && - /** @type {Derived} */ (signal).deps === null && - (active_reaction === null || untracking || (active_reaction.f & DERIVED) !== 0) - ) { + } else if (is_derived && /** @type {Derived} */ (signal).deps === null) { var derived = /** @type {Derived} */ (signal); var parent = derived.parent; diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index aa1dbf31328c..687e715eb956 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -9,11 +9,12 @@ import { user_effect } from '../../src/internal/client/reactivity/effects'; import { state, set } from '../../src/internal/client/reactivity/sources'; -import type { Derived, Value } from '../../src/internal/client/types'; +import type { Derived, Effect, Value } from '../../src/internal/client/types'; 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'; /** * @param runes runes mode @@ -256,12 +257,16 @@ describe('signals', () => { const a = state(0); const b = state(0); - const c = derived(() => { - const a_2 = derived(() => $.get(a) + '!'); - const b_2 = derived(() => $.get(b) + '?'); - nested.push(a_2, b_2); + let c: any; + + const destroy = effect_root(() => { + c = derived(() => { + const a_2 = derived(() => $.get(a) + '!'); + const b_2 = derived(() => $.get(b) + '?'); + nested.push(a_2, b_2); - return { a: $.get(a_2), b: $.get(b_2) }; + return { a: $.get(a_2), b: $.get(b_2) }; + }); }); $.get(c); @@ -274,11 +279,14 @@ describe('signals', () => { $.get(c); - // Ensure we're not leaking dependencies - assert.deepEqual( - nested.slice(0, -2).map((s) => s.deps), - [null, null, null, null] - ); + // Ensure we're not leaking + assert.equal(a.reactions?.[0], nested.at(-2)); + assert.equal(b.reactions?.[0], nested.at(-1)); + + destroy(); + + assert.equal(a.reactions, null); + assert.equal(b.reactions, null); }; }); @@ -831,12 +839,54 @@ describe('signals', () => { }; }); + test('deriveds containing effects work correctly', () => { + return () => { + let a = render_effect(() => {}); + let b = state(0); + let c; + let effects: Effect[] = []; + + const destroy = effect_root(() => { + a = render_effect(() => { + c = derived(() => { + effects.push( + effect(() => { + $.get(b); + }) + ); + $.get(b); + }); + $.get(c); + }); + }); + + assert.equal(c!.children?.length, 1); + assert.equal(a.first, a.last); + + set(b, 1); + + flushSync(); + + assert.equal(c!.children?.length, 1); + assert.equal(a.first, a.last); + + destroy(); + + assert.equal(a.deriveds, null); + assert.equal(a.first, null); + + assert.equal(effects.length, 2); + assert.equal((effects[0].f & DESTROYED) !== 0, true); + assert.equal((effects[1].f & DESTROYED) !== 0, true); + }; + }); + test('deriveds containing effects work correctly when used with untrack', () => { return () => { let a = render_effect(() => {}); let b = state(0); let c; - let effects = []; + let effects: Effect[] = []; const destroy = effect_root(() => { a = render_effect(() => { @@ -854,20 +904,24 @@ describe('signals', () => { }); }); - assert.deepEqual(c!.children?.length, 1); - assert.deepEqual(a.first, a.last); + assert.equal(c!.children?.length, 1); + assert.equal(a.first, a.last); set(b, 1); flushSync(); - assert.deepEqual(c!.children?.length, 1); - assert.deepEqual(a.first, a.last); + assert.equal(c!.children?.length, 1); + assert.equal(a.first, a.last); destroy(); - assert.deepEqual(a.deriveds, null); - assert.deepEqual(a.first, null); + assert.equal(a.deriveds, null); + assert.equal(a.first, null); + + assert.equal(effects.length, 2); + assert.equal((effects[0].f & DESTROYED) !== 0, true); + assert.equal((effects[1].f & DESTROYED) !== 0, true); }; }); From 293bdfd721140724c60ddc3a7533fcdfe4423bdf Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 10:52:35 +0000 Subject: [PATCH 02/10] revised --- .../svelte/src/internal/client/runtime.js | 22 ++++--- packages/svelte/tests/signals/test.ts | 58 +++++++++++++++++++ 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 87d6b589d7fd..066532fbd5a8 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -414,7 +414,13 @@ export function update_reaction(reaction) { skipped_deps = 0; untracked_writes = null; active_reaction = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null; - skip_reaction = !is_flushing_effect && (flags & UNOWNED) !== 0; + // prettier-ignore + skip_reaction = + (flags & UNOWNED) !== 0 && + (!is_flushing_effect || + (/** @type {Derived} */ (reaction).parent !== null && + (/** @type {Derived} */ (reaction).parent.f & DERIVED) !== 0)); + derived_sources = null; set_component_context(reaction.ctx); untracking = false; @@ -946,14 +952,14 @@ export function get(signal) { var parent = derived.parent; if (parent !== null) { - // Attach the derived to the nearest parent effect or derived - if ((parent.f & DERIVED) !== 0) { - var parent_derived = /** @type {Derived} */ (parent); - - if (!parent_derived.children?.includes(derived)) { - (parent_derived.children ??= []).push(derived); - } + // If the derived is owned by another derived then mark it as unowned + // as the derived value might have been shared and thus we cannot determine + // a true + if ((parent.f & DERIVED) !== 0 && (parent.f & UNOWNED) === 0) { + debugger; + derived.f ^= UNOWNED; } else { + // Otherwise we can attach the derieved to the parent effect var parent_effect = /** @type {Effect} */ (parent); if (!parent_effect.deriveds?.includes(derived)) { diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 687e715eb956..d09a748bc00d 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -538,6 +538,64 @@ describe('signals', () => { }; }); + test('mixed nested deriveds correctly cleanup when no longer connected to graph #1', () => { + let a: Derived; + let b: Derived; + let s = state(0); + + const destroy = effect_root(() => { + render_effect(() => { + a = derived(() => { + b = derived(() => { + $.get(s); + }); + $.untrack(() => { + $.get(b); + }); + $.get(s); + }); + $.get(a); + }); + }); + + return () => { + flushSync(); + assert.equal(a?.deps?.length, 1); + assert.equal(s?.reactions?.length, 1); + destroy(); + assert.equal(s?.reactions, null); + }; + }); + + test('mixed nested deriveds correctly cleanup when no longer connected to graph #2', () => { + let a: Derived; + let b: Derived; + let s = state(0); + + const destroy = effect_root(() => { + render_effect(() => { + a = derived(() => { + b = derived(() => { + $.get(s); + }); + effect_root(() => { + $.get(b); + }); + $.get(s); + }); + $.get(a); + }); + }); + + return () => { + flushSync(); + assert.equal(a?.deps?.length, 1); + assert.equal(s?.reactions?.length, 1); + destroy(); + assert.equal(s?.reactions, null); + }; + }); + test('deriveds update upon reconnection #1', () => { let a = state(false); let b = state(false); From bcbba7e19ba07c9e32c48a63be3ff1ecf7216ebd Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 10:54:40 +0000 Subject: [PATCH 03/10] revised --- packages/svelte/src/internal/client/runtime.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 066532fbd5a8..613f01338951 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -953,10 +953,9 @@ export function get(signal) { if (parent !== null) { // If the derived is owned by another derived then mark it as unowned - // as the derived value might have been shared and thus we cannot determine - // a true + // as the derived value might have been referenced in a different context + // since and thus its parent might not be its true owner anymore if ((parent.f & DERIVED) !== 0 && (parent.f & UNOWNED) === 0) { - debugger; derived.f ^= UNOWNED; } else { // Otherwise we can attach the derieved to the parent effect From 91a23e06e48ae414819c12a26a5b476beb380667 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 11:12:08 +0000 Subject: [PATCH 04/10] revised --- .../svelte/src/internal/client/runtime.js | 6 ++- packages/svelte/tests/signals/test.ts | 44 ++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 613f01338951..a870e93c0eb9 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -418,8 +418,12 @@ export function update_reaction(reaction) { skip_reaction = (flags & UNOWNED) !== 0 && (!is_flushing_effect || + // If we were previously not in a reactive context and we're reading an unowned derived + // that was created inside another derived, then we don't fully know the real owner and thus + // we need to skip adding any reactions for this unowned + ((previous_reaction === null || previous_untracking) && (/** @type {Derived} */ (reaction).parent !== null && - (/** @type {Derived} */ (reaction).parent.f & DERIVED) !== 0)); + (/** @type {Derived} */ (reaction).parent.f & DERIVED) !== 0))); derived_sources = null; set_component_context(reaction.ctx); diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index d09a748bc00d..b7ab101bc3dc 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -69,7 +69,7 @@ describe('signals', () => { }; }); - test('multiple effects with state and derived in it#1', () => { + test('multiple effects with state and derived in it #1', () => { const log: string[] = []; let count = state(0); @@ -90,7 +90,7 @@ describe('signals', () => { }; }); - test('multiple effects with state and derived in it#2', () => { + test('multiple effects with state and derived in it #2', () => { const log: string[] = []; let count = state(0); @@ -596,6 +596,46 @@ describe('signals', () => { }; }); + test('mixed nested deriveds correctly cleanup when no longer connected to graph #3', () => { + let a: Derived; + let b: Derived; + let s = state(0); + let logs: any[] = []; + + const destroy = effect_root(() => { + render_effect(() => { + a = derived(() => { + b = derived(() => { + return $.get(s); + }); + effect_root(() => { + $.get(b); + }); + render_effect(() => { + debugger + logs.push($.get(b)); + }); + $.get(s); + }); + $.get(a); + }); + }); + + return () => { + flushSync(); + assert.equal(a?.deps?.length, 1); + assert.equal(s?.reactions?.length, 2); + + set(s, 1); + flushSync(); + + assert.deepEqual(logs, [0, 1]); + + destroy(); + assert.equal(s?.reactions, null); + }; + }); + test('deriveds update upon reconnection #1', () => { let a = state(false); let b = state(false); From 679929a5158407a5167ebf7abc7a3b71ac6559b8 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 11:16:17 +0000 Subject: [PATCH 05/10] revised --- packages/svelte/tests/signals/test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index b7ab101bc3dc..04cf2b4550bd 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -612,7 +612,6 @@ describe('signals', () => { $.get(b); }); render_effect(() => { - debugger logs.push($.get(b)); }); $.get(s); From 927f3d2acc4665afad036a5bfd9b7bd9e6a8c77f Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 12:15:41 +0000 Subject: [PATCH 06/10] feedback --- .../internal/client/reactivity/deriveds.js | 42 ++++++++----------- .../src/internal/client/reactivity/effects.js | 4 +- .../src/internal/client/reactivity/types.d.ts | 4 +- .../svelte/src/internal/client/runtime.js | 18 +++++--- packages/svelte/tests/signals/test.ts | 10 ++--- 5 files changed, 39 insertions(+), 39 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index f180a6999574..1f65ff38c351 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -35,8 +35,12 @@ import { component_context } from '../context.js'; /*#__NO_SIDE_EFFECTS__*/ export function derived(fn) { var flags = DERIVED | DIRTY; + var parent_derived = + active_reaction !== null && (active_reaction.f & DERIVED) !== 0 + ? /** @type {Derived} */ (active_reaction) + : null; - if (active_effect === null) { + if (active_effect === null || (parent_derived !== null && (parent_derived.f & UNOWNED) !== 0)) { flags |= UNOWNED; } else { // Since deriveds are evaluated lazily, any effects created inside them are @@ -44,16 +48,11 @@ export function derived(fn) { active_effect.f |= EFFECT_HAS_DERIVED; } - var parent_derived = - active_reaction !== null && (active_reaction.f & DERIVED) !== 0 - ? /** @type {Derived} */ (active_reaction) - : null; - /** @type {Derived} */ const signal = { - children: null, ctx: component_context, deps: null, + effects: null, equals, f: flags, fn, @@ -87,19 +86,14 @@ export function derived_safe_equal(fn) { * @param {Derived} derived * @returns {void} */ -export function destroy_derived_children(derived) { - var children = derived.children; - - if (children !== null) { - derived.children = null; - - for (var i = 0; i < children.length; i += 1) { - var child = children[i]; - if ((child.f & DERIVED) !== 0) { - destroy_derived(/** @type {Derived} */ (child)); - } else { - destroy_effect(/** @type {Effect} */ (child)); - } +export function destroy_derived_effects(derived) { + var effects = derived.effects; + + if (effects !== null) { + derived.effects = null; + + for (var i = 0; i < effects.length; i += 1) { + destroy_effect(/** @type {Effect} */ (effects[i])); } } } @@ -147,7 +141,7 @@ export function execute_derived(derived) { stack.push(derived); - destroy_derived_children(derived); + destroy_derived_effects(derived); value = update_reaction(derived); } finally { set_active_effect(prev_active_effect); @@ -156,7 +150,7 @@ export function execute_derived(derived) { } } else { try { - destroy_derived_children(derived); + destroy_derived_effects(derived); value = update_reaction(derived); } finally { set_active_effect(prev_active_effect); @@ -188,9 +182,9 @@ export function update_derived(derived) { * @returns {void} */ export function destroy_derived(derived) { - destroy_derived_children(derived); + destroy_derived_effects(derived); remove_reactions(derived, 0); set_signal_status(derived, DESTROYED); - derived.v = derived.children = derived.deps = derived.ctx = derived.reactions = null; + derived.v = derived.deps = derived.ctx = derived.reactions = null; } diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 8a7cffecd623..1ccc2b4b6173 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -53,7 +53,7 @@ export function validate_effect(rune) { e.effect_orphan(rune); } - if (active_reaction !== null && (active_reaction.f & UNOWNED) !== 0) { + if (active_reaction !== null && (active_reaction.f & UNOWNED) !== 0 && active_effect === null) { e.effect_in_unowned_derived(); } @@ -153,7 +153,7 @@ function create_effect(type, fn, sync, push = true) { // if we're in a derived, add the effect there too if (active_reaction !== null && (active_reaction.f & DERIVED) !== 0) { var derived = /** @type {Derived} */ (active_reaction); - (derived.children ??= []).push(effect); + (derived.effects ??= []).push(effect); } } diff --git a/packages/svelte/src/internal/client/reactivity/types.d.ts b/packages/svelte/src/internal/client/reactivity/types.d.ts index 3a76a3ff836c..d992f840b5b5 100644 --- a/packages/svelte/src/internal/client/reactivity/types.d.ts +++ b/packages/svelte/src/internal/client/reactivity/types.d.ts @@ -36,8 +36,8 @@ export interface Reaction extends Signal { export interface Derived extends Value, Reaction { /** The derived function */ fn: () => V; - /** Reactions created inside this signal */ - children: null | Reaction[]; + /** Effects created inside this signal */ + effects: null | Effect[]; /** Parent effect or derived */ parent: Effect | Derived | null; } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index a870e93c0eb9..e32ff60ca514 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -30,7 +30,7 @@ import { flush_tasks } from './dom/task.js'; import { internal_set, set } from './reactivity/sources.js'; import { destroy_derived, - destroy_derived_children, + destroy_derived_effects, execute_derived, update_derived } from './reactivity/deriveds.js'; @@ -419,11 +419,10 @@ export function update_reaction(reaction) { (flags & UNOWNED) !== 0 && (!is_flushing_effect || // If we were previously not in a reactive context and we're reading an unowned derived - // that was created inside another derived, then we don't fully know the real owner and thus + // that was created inside another reaction, then we don't fully know the real owner and thus // we need to skip adding any reactions for this unowned ((previous_reaction === null || previous_untracking) && - (/** @type {Derived} */ (reaction).parent !== null && - (/** @type {Derived} */ (reaction).parent.f & DERIVED) !== 0))); + /** @type {Derived} */ (reaction).parent !== null)); derived_sources = null; set_component_context(reaction.ctx); @@ -533,7 +532,7 @@ function remove_reaction(signal, dependency) { dependency.f ^= DISCONNECTED; } // Disconnect any reactions owned by this reaction - destroy_derived_children(/** @type {Derived} **/ (dependency)); + destroy_derived_effects(/** @type {Derived} **/ (dependency)); remove_reactions(/** @type {Derived} **/ (dependency), 0); } } @@ -951,11 +950,18 @@ export function get(signal) { new_deps.push(signal); } } - } else if (is_derived && /** @type {Derived} */ (signal).deps === null) { + } else if ( + is_derived && + /** @type {Derived} */ (signal).deps === null && + /** @type {Derived} */ (signal).effects === null + ) { var derived = /** @type {Derived} */ (signal); var parent = derived.parent; if (parent !== null) { + // if ((parent.f & UNOWNED) === 0) { + // derived.f ^= UNOWNED; + // } // If the derived is owned by another derived then mark it as unowned // as the derived value might have been referenced in a different context // since and thus its parent might not be its true owner anymore diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 04cf2b4550bd..b062b3d7ef35 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -928,7 +928,7 @@ describe('signals', () => { }); assert.deepEqual(a.deriveds?.length, 1); - assert.deepEqual(b?.children, null); + assert.deepEqual(b?.effects, null); destroy(); @@ -957,14 +957,14 @@ describe('signals', () => { }); }); - assert.equal(c!.children?.length, 1); + assert.equal(c!.effects?.length, 1); assert.equal(a.first, a.last); set(b, 1); flushSync(); - assert.equal(c!.children?.length, 1); + assert.equal(c!.effects?.length, 1); assert.equal(a.first, a.last); destroy(); @@ -1001,14 +1001,14 @@ describe('signals', () => { }); }); - assert.equal(c!.children?.length, 1); + assert.equal(c!.effects?.length, 1); assert.equal(a.first, a.last); set(b, 1); flushSync(); - assert.equal(c!.children?.length, 1); + assert.equal(c!.effects?.length, 1); assert.equal(a.first, a.last); destroy(); From 3b85e10ad26a5eed42164ba3f9c79f75bb9dfa01 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 12:22:19 +0000 Subject: [PATCH 07/10] feedback --- packages/svelte/src/internal/client/runtime.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index e32ff60ca514..1f8e4b79526a 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -959,9 +959,6 @@ export function get(signal) { var parent = derived.parent; if (parent !== null) { - // if ((parent.f & UNOWNED) === 0) { - // derived.f ^= UNOWNED; - // } // If the derived is owned by another derived then mark it as unowned // as the derived value might have been referenced in a different context // since and thus its parent might not be its true owner anymore From 8fdb80c2f1a7942bae7ff67c97ece7657648e36a Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 12:22:34 +0000 Subject: [PATCH 08/10] invasive change --- .../src/internal/client/reactivity/effects.js | 18 ------ .../src/internal/client/reactivity/types.d.ts | 2 - .../svelte/src/internal/client/runtime.js | 11 +--- packages/svelte/tests/signals/test.ts | 61 +------------------ 4 files changed, 2 insertions(+), 90 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 1ccc2b4b6173..d014ff793dea 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -99,7 +99,6 @@ function create_effect(type, fn, sync, push = true) { var effect = { ctx: component_context, deps: null, - deriveds: null, nodes_start: null, nodes_end: null, f: type | DIRTY, @@ -395,22 +394,6 @@ export function execute_effect_teardown(effect) { } } -/** - * @param {Effect} signal - * @returns {void} - */ -export function destroy_effect_deriveds(signal) { - var deriveds = signal.deriveds; - - if (deriveds !== null) { - signal.deriveds = null; - - for (var i = 0; i < deriveds.length; i += 1) { - destroy_derived(deriveds[i]); - } - } -} - /** * @param {Effect} signal * @param {boolean} remove_dom @@ -468,7 +451,6 @@ export function destroy_effect(effect, remove_dom = true) { } destroy_effect_children(effect, remove_dom && !removed); - destroy_effect_deriveds(effect); remove_reactions(effect, 0); set_signal_status(effect, DESTROYED); diff --git a/packages/svelte/src/internal/client/reactivity/types.d.ts b/packages/svelte/src/internal/client/reactivity/types.d.ts index d992f840b5b5..5ef0097649a4 100644 --- a/packages/svelte/src/internal/client/reactivity/types.d.ts +++ b/packages/svelte/src/internal/client/reactivity/types.d.ts @@ -51,8 +51,6 @@ export interface Effect extends Reaction { */ nodes_start: null | TemplateNode; nodes_end: null | TemplateNode; - /** Reactions created inside this signal */ - deriveds: null | Derived[]; /** The effect function */ fn: null | (() => void | (() => void)); /** The teardown function returned from the effect function */ diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 1f8e4b79526a..2c53ac8f7b84 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -4,7 +4,6 @@ import { define_property, get_descriptors, get_prototype_of, index_of } from '.. import { destroy_block_effect_children, destroy_effect_children, - destroy_effect_deriveds, execute_effect_teardown, unlink_effect } from './reactivity/effects.js'; @@ -580,7 +579,6 @@ export function update_effect(effect) { } else { destroy_effect_children(effect); } - destroy_effect_deriveds(effect); execute_effect_teardown(effect); var teardown = update_reaction(effect); @@ -962,15 +960,8 @@ export function get(signal) { // If the derived is owned by another derived then mark it as unowned // as the derived value might have been referenced in a different context // since and thus its parent might not be its true owner anymore - if ((parent.f & DERIVED) !== 0 && (parent.f & UNOWNED) === 0) { + if ((parent.f & UNOWNED) === 0) { derived.f ^= UNOWNED; - } else { - // Otherwise we can attach the derieved to the parent effect - var parent_effect = /** @type {Effect} */ (parent); - - if (!parent_effect.deriveds?.includes(derived)) { - (parent_effect.deriveds ??= []).push(derived); - } } } } diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index b062b3d7ef35..edb26fba15d9 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -279,10 +279,6 @@ describe('signals', () => { $.get(c); - // Ensure we're not leaking - assert.equal(a.reactions?.[0], nested.at(-2)); - assert.equal(b.reactions?.[0], nested.at(-1)); - destroy(); assert.equal(a.reactions, null); @@ -511,7 +507,7 @@ describe('signals', () => { set(inner, 2); $.get(a); }); - assert.deepEqual(log, ['inner', 2]); + assert.deepEqual(log, ['outer', 1, 'inner', 2]); destroy(); }; }); @@ -883,59 +879,6 @@ describe('signals', () => { }; }); - test('nested deriveds clean up the relationships when used with untrack', () => { - return () => { - let a = render_effect(() => {}); - - const destroy = effect_root(() => { - a = render_effect(() => { - $.untrack(() => { - const b = derived(() => { - const c = derived(() => {}); - $.untrack(() => { - $.get(c); - }); - }); - $.get(b); - }); - }); - }); - - assert.deepEqual(a.deriveds?.length, 1); - - destroy(); - - assert.deepEqual(a.deriveds, null); - }; - }); - - test('nested deriveds do not connect inside parent deriveds if unused', () => { - return () => { - let a = render_effect(() => {}); - let b: Derived | undefined; - - const destroy = effect_root(() => { - a = render_effect(() => { - $.untrack(() => { - b = derived(() => { - derived(() => {}); - derived(() => {}); - derived(() => {}); - }); - $.get(b); - }); - }); - }); - - assert.deepEqual(a.deriveds?.length, 1); - assert.deepEqual(b?.effects, null); - - destroy(); - - assert.deepEqual(a.deriveds, null); - }; - }); - test('deriveds containing effects work correctly', () => { return () => { let a = render_effect(() => {}); @@ -969,7 +912,6 @@ describe('signals', () => { destroy(); - assert.equal(a.deriveds, null); assert.equal(a.first, null); assert.equal(effects.length, 2); @@ -1013,7 +955,6 @@ describe('signals', () => { destroy(); - assert.equal(a.deriveds, null); assert.equal(a.first, null); assert.equal(effects.length, 2); From adad37b716424a207698953a99c1e2f0efc45cdd Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 12:43:31 +0000 Subject: [PATCH 09/10] fix bugs --- packages/svelte/src/internal/client/runtime.js | 3 +++ .../samples/derived-unowned-7/_config.js | 11 ++++++++++- packages/svelte/tests/signals/test.ts | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 2c53ac8f7b84..6dd253c0b535 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -216,6 +216,9 @@ export function check_dirtiness(reaction) { } if (dependency.wv > reaction.wv) { + if (is_unowned) { + reaction.wv = dependency.wv; + } return true; } } diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js b/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js index 53ca29f4c3f0..bbb24f5e1cef 100644 --- a/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js @@ -9,6 +9,15 @@ export default test({ btn1.click(); }); - assert.deepEqual(logs, ['computing C', 'computing B', 'a', 'foo', 'computing B', 'aaa', 'foo']); + assert.deepEqual(logs, [ + 'computing C', + 'computing B', + 'a', + 'foo', + 'computing B', + 'aaa', + 'computing C' + 'foo' + ]); } }); diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index edb26fba15d9..b9fee3b46a52 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -507,7 +507,7 @@ describe('signals', () => { set(inner, 2); $.get(a); }); - assert.deepEqual(log, ['outer', 1, 'inner', 2]); + assert.deepEqual(log, ['inner', 2]); destroy(); }; }); From 7f01dc3a8c23b746e51a1ae36927f88907c28faa Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 29 Jan 2025 13:09:28 +0000 Subject: [PATCH 10/10] fix other bug --- packages/svelte/src/internal/client/runtime.js | 3 --- .../samples/derived-unowned-7/_config.js | 11 +---------- packages/svelte/tests/signals/test.ts | 1 + 3 files changed, 2 insertions(+), 13 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 6dd253c0b535..2c53ac8f7b84 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -216,9 +216,6 @@ export function check_dirtiness(reaction) { } if (dependency.wv > reaction.wv) { - if (is_unowned) { - reaction.wv = dependency.wv; - } return true; } } diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js b/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js index bbb24f5e1cef..53ca29f4c3f0 100644 --- a/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-7/_config.js @@ -9,15 +9,6 @@ export default test({ btn1.click(); }); - assert.deepEqual(logs, [ - 'computing C', - 'computing B', - 'a', - 'foo', - 'computing B', - 'aaa', - 'computing C' - 'foo' - ]); + assert.deepEqual(logs, ['computing C', 'computing B', 'a', 'foo', 'computing B', 'aaa', 'foo']); } }); diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index b9fee3b46a52..2ce624a7772e 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -481,6 +481,7 @@ describe('signals', () => { effect(() => { log.push('inner', $.get(inner)); }); + return $.get(outer); }); }); });