From 9285e7b1d3133f2ed8fd33aed6639a00d75762cf Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Sat, 28 Dec 2024 21:52:41 +0000 Subject: [PATCH 1/9] fix: ensure unowned deriveds correctly get re-linked to the graph --- .changeset/chatty-dolphins-complain.md | 5 +++++ packages/svelte/src/internal/client/runtime.js | 12 ++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 .changeset/chatty-dolphins-complain.md diff --git a/.changeset/chatty-dolphins-complain.md b/.changeset/chatty-dolphins-complain.md new file mode 100644 index 000000000000..b2c6f2b971e2 --- /dev/null +++ b/.changeset/chatty-dolphins-complain.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure unowned deriveds correctly get re-linked to the graph diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 4a90a219712f..98aba8a2f0f4 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -205,10 +205,12 @@ export function check_dirtiness(reaction) { reaction.f ^= DISCONNECTED; } + var dirty = false; + for (i = 0; i < dependencies.length; i++) { var dependency = dependencies[i]; - if (check_dirtiness(/** @type {Derived} */ (dependency))) { + if (!dirty && check_dirtiness(/** @type {Derived} */ (dependency))) { update_derived(/** @type {Derived} */ (dependency)); } @@ -225,9 +227,15 @@ export function check_dirtiness(reaction) { } if (dependency.version > reaction.version) { - return true; + // We can't just return here as we might have other dependencies that are unowned + // ad need to be linked to the reaction again + dirty = true; } } + + if (dirty) { + return true; + } } // Unowned signals should never be marked as clean unless they From 9c5e708726318bec05d892b553aa3d70a4624af5 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Sat, 28 Dec 2024 21:57:37 +0000 Subject: [PATCH 2/9] fix: ensure unowned deriveds correctly get re-linked to the graph --- .../svelte/src/internal/client/runtime.js | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 98aba8a2f0f4..f25cd592374f 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -205,15 +205,17 @@ export function check_dirtiness(reaction) { reaction.f ^= DISCONNECTED; } - var dirty = false; + var unowned_dirty = false; for (i = 0; i < dependencies.length; i++) { var dependency = dependencies[i]; - if (!dirty && check_dirtiness(/** @type {Derived} */ (dependency))) { + if (!unowned_dirty && check_dirtiness(/** @type {Derived} */ (dependency))) { update_derived(/** @type {Derived} */ (dependency)); } + var version_mismatch = dependency.version > reaction.version; + // If we are working with an unowned signal as part of an effect (due to !skip_reaction) // and the version hasn't changed, we still need to check that this reaction // is linked to the dependency source – otherwise future updates will not be caught. @@ -224,16 +226,15 @@ export function check_dirtiness(reaction) { !dependency?.reactions?.includes(reaction) ) { (dependency.reactions ??= []).push(reaction); - } - - if (dependency.version > reaction.version) { - // We can't just return here as we might have other dependencies that are unowned - // ad need to be linked to the reaction again - dirty = true; + if (version_mismatch) { + unowned_dirty = true; + } + } else if (version_mismatch) { + return true } } - if (dirty) { + if (unowned_dirty) { return true; } } From 1e929223c22793a910936b4da5d1cb56becc9dc4 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Sat, 28 Dec 2024 21:58:22 +0000 Subject: [PATCH 3/9] fix: ensure unowned deriveds correctly get re-linked to the graph --- packages/svelte/src/internal/client/runtime.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index f25cd592374f..f46a3ece5108 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -219,18 +219,16 @@ export function check_dirtiness(reaction) { // If we are working with an unowned signal as part of an effect (due to !skip_reaction) // and the version hasn't changed, we still need to check that this reaction // is linked to the dependency source – otherwise future updates will not be caught. - if ( - is_unowned && - active_effect !== null && - !skip_reaction && - !dependency?.reactions?.includes(reaction) - ) { - (dependency.reactions ??= []).push(reaction); + if (is_unowned && active_effect !== null && !skip_reaction) { + if (!dependency?.reactions?.includes(reaction)) { + (dependency.reactions ??= []).push(reaction); + } + if (version_mismatch) { unowned_dirty = true; } } else if (version_mismatch) { - return true + return true; } } From 7ec02948027891da8fc901c0683463e1559f5326 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 2 Jan 2025 14:27:30 +0000 Subject: [PATCH 4/9] add test --- packages/svelte/tests/signals/test.ts | 33 +++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 6796655cc8d2..6d1be168d73c 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -781,4 +781,37 @@ describe('signals', () => { assert.equal($.get(count), 0n); }; }); + + test('unowned deriveds correctly re-attach to their source', () => { + const log: any[] = []; + + return () => { + const a = state(0); + const b = state(0); + const c = derived(() => { + $.get(a); + return $.get(b) + }); + + $.get(c); + + set(a, 1); + + const destroy = effect_root(() => { + render_effect(() => { + log.push($.get(c)); + }); + }); + + assert.deepEqual(log, [0]); + + set(b, 1); + + flushSync(); + + assert.deepEqual(log, [0, 1]); + + destroy(); + }; + }); }); From 3634549541f012a3a80b232cbfaa39f2a2c21f1c Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 2 Jan 2025 14:31:45 +0000 Subject: [PATCH 5/9] add test --- packages/svelte/tests/signals/test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 6d1be168d73c..0a22e302864e 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -790,7 +790,7 @@ describe('signals', () => { const b = state(0); const c = derived(() => { $.get(a); - return $.get(b) + return $.get(b); }); $.get(c); From ca2751b8d6579afee691d510379c0d71d85423a9 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 2 Jan 2025 16:32:19 +0000 Subject: [PATCH 6/9] cleaner apporach --- .../svelte/src/internal/client/runtime.js | 38 +++++++------------ 1 file changed, 14 insertions(+), 24 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index f46a3ece5108..630f7a93ef03 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -196,45 +196,35 @@ export function check_dirtiness(reaction) { if (dependencies !== null) { var i; + var dependency; + var depedency_needs_linking = + (flags & DISCONNECTED) !== 0 || (is_unowned && active_effect !== null && !skip_reaction); - if ((flags & DISCONNECTED) !== 0) { + // If we are working with a disconnected or an unowned signal as part of an effect (due to !skip_reaction) + // then we need to re-connect the reaction to the dependency + if (depedency_needs_linking) { for (i = 0; i < dependencies.length; i++) { - (dependencies[i].reactions ??= []).push(reaction); + dependency = dependencies[i]; + + if (!dependency?.reactions?.includes(reaction)) { + (dependency.reactions ??= []).push(reaction); + } } reaction.f ^= DISCONNECTED; } - var unowned_dirty = false; - for (i = 0; i < dependencies.length; i++) { - var dependency = dependencies[i]; + dependency = dependencies[i]; - if (!unowned_dirty && check_dirtiness(/** @type {Derived} */ (dependency))) { + if (check_dirtiness(/** @type {Derived} */ (dependency))) { update_derived(/** @type {Derived} */ (dependency)); } - var version_mismatch = dependency.version > reaction.version; - - // If we are working with an unowned signal as part of an effect (due to !skip_reaction) - // and the version hasn't changed, we still need to check that this reaction - // is linked to the dependency source – otherwise future updates will not be caught. - if (is_unowned && active_effect !== null && !skip_reaction) { - if (!dependency?.reactions?.includes(reaction)) { - (dependency.reactions ??= []).push(reaction); - } - - if (version_mismatch) { - unowned_dirty = true; - } - } else if (version_mismatch) { + if (dependency.version > reaction.version) { return true; } } - - if (unowned_dirty) { - return true; - } } // Unowned signals should never be marked as clean unless they From 8bedfc7b069ada294da5f14e774f99855d23cd64 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 2 Jan 2025 16:33:18 +0000 Subject: [PATCH 7/9] cleaner apporach --- packages/svelte/src/internal/client/runtime.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 630f7a93ef03..1fff95e1a2c5 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -197,8 +197,9 @@ export function check_dirtiness(reaction) { if (dependencies !== null) { var i; var dependency; + var is_disconnected = (flags & DISCONNECTED) !== 0; var depedency_needs_linking = - (flags & DISCONNECTED) !== 0 || (is_unowned && active_effect !== null && !skip_reaction); + is_disconnected || (is_unowned && active_effect !== null && !skip_reaction); // If we are working with a disconnected or an unowned signal as part of an effect (due to !skip_reaction) // then we need to re-connect the reaction to the dependency @@ -211,7 +212,9 @@ export function check_dirtiness(reaction) { } } - reaction.f ^= DISCONNECTED; + if (is_disconnected) { + reaction.f ^= DISCONNECTED; + } } for (i = 0; i < dependencies.length; i++) { From 3ec6bc6d9d76398791e875016ad0e4b6ab44be0b Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 2 Jan 2025 16:34:29 +0000 Subject: [PATCH 8/9] cleaner apporach --- packages/svelte/src/internal/client/runtime.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 1fff95e1a2c5..fa90977f2968 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -198,12 +198,11 @@ export function check_dirtiness(reaction) { var i; var dependency; var is_disconnected = (flags & DISCONNECTED) !== 0; - var depedency_needs_linking = - is_disconnected || (is_unowned && active_effect !== null && !skip_reaction); + var is_unowned_connected = is_unowned && active_effect !== null && !skip_reaction; - // If we are working with a disconnected or an unowned signal as part of an effect (due to !skip_reaction) + // If we are working with a disconnected or an unowned signal that is now connected (due to an active effect) // then we need to re-connect the reaction to the dependency - if (depedency_needs_linking) { + if (is_disconnected || is_unowned_connected) { for (i = 0; i < dependencies.length; i++) { dependency = dependencies[i]; From 3f8c7dd732b5ef6c48eee3b930c7100ff8114cde Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Thu, 2 Jan 2025 16:35:38 +0000 Subject: [PATCH 9/9] cleaner apporach --- packages/svelte/src/internal/client/runtime.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index fa90977f2968..83d01caabe08 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -199,11 +199,12 @@ export function check_dirtiness(reaction) { var dependency; var is_disconnected = (flags & DISCONNECTED) !== 0; var is_unowned_connected = is_unowned && active_effect !== null && !skip_reaction; + var length = dependencies.length; // If we are working with a disconnected or an unowned signal that is now connected (due to an active effect) // then we need to re-connect the reaction to the dependency if (is_disconnected || is_unowned_connected) { - for (i = 0; i < dependencies.length; i++) { + for (i = 0; i < length; i++) { dependency = dependencies[i]; if (!dependency?.reactions?.includes(reaction)) { @@ -216,7 +217,7 @@ export function check_dirtiness(reaction) { } } - for (i = 0; i < dependencies.length; i++) { + for (i = 0; i < length; i++) { dependency = dependencies[i]; if (check_dirtiness(/** @type {Derived} */ (dependency))) {