From e3d69b537204720601e2a4d23147f64f399e09e4 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 12:14:17 +0000 Subject: [PATCH 01/13] fix: when an unowned derived is tracked again, remove unowned flag --- .changeset/nasty-pigs-lay.md | 5 +++++ .../svelte/src/internal/client/runtime.js | 20 ++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 .changeset/nasty-pigs-lay.md diff --git a/.changeset/nasty-pigs-lay.md b/.changeset/nasty-pigs-lay.md new file mode 100644 index 000000000000..b7252b70f77b --- /dev/null +++ b/.changeset/nasty-pigs-lay.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: when an unowned derived is tracked again, remove unowned flag diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 30f14b7356ce..65376ddf39bf 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -184,19 +184,29 @@ export function check_dirtiness(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 (is_disconnected || is_unowned_connected) { + var derived = /** @type {Derived} */ (reaction); + for (i = 0; i < length; i++) { dependency = dependencies[i]; // We always re-add all reactions (even duplicates) if the derived was // previously disconnected, however we don't if it was unowned as we // de-duplicate dependencies in that case - if (is_disconnected || !dependency?.reactions?.includes(reaction)) { - (dependency.reactions ??= []).push(reaction); + if (is_disconnected || !dependency?.reactions?.includes(derived)) { + (dependency.reactions ??= []).push(derived); } } if (is_disconnected) { - reaction.f ^= DISCONNECTED; + derived.f ^= DISCONNECTED; + } + var parent = derived.parent; + + if (is_unowned_connected && parent !== null && (parent.f & UNOWNED) === 0) { + // If the derived is owned by another derived then mark it as owned agaub + // as the derived value might have been referenced in a different context + // and now has a reacive context managing it + derived.f ^= UNOWNED; } } @@ -409,6 +419,10 @@ export function update_reaction(reaction) { (flags & UNOWNED) !== 0 && (!is_flushing_effect || previous_reaction === null || previous_untracking); + if (skip_reaction) { + // debugger + } + derived_sources = null; set_component_context(reaction.ctx); untracking = false; From ac75c1b512ad1c1d945a2612aa8b5f87f405e0d1 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 12:15:01 +0000 Subject: [PATCH 02/13] fix: when an unowned derived is tracked again, remove unowned flag --- packages/svelte/src/internal/client/runtime.js | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 65376ddf39bf..923d89b0a8f4 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -419,10 +419,6 @@ export function update_reaction(reaction) { (flags & UNOWNED) !== 0 && (!is_flushing_effect || previous_reaction === null || previous_untracking); - if (skip_reaction) { - // debugger - } - derived_sources = null; set_component_context(reaction.ctx); untracking = false; From 5205e7c69b79fc36a1f127f66f3471e924730c66 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 12:40:28 +0000 Subject: [PATCH 03/13] test case --- packages/svelte/tests/signals/test.ts | 147 ++++++++++++++++++++++---- 1 file changed, 124 insertions(+), 23 deletions(-) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 46209ede8189..4c65ea216491 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -224,20 +224,75 @@ describe('signals', () => { }; }); - test('effects correctly handle unowned derived values that do not change', () => { - const log: number[] = []; + test('https://perf.js.hyoo.ru/#!bench=9h2as6_u0mfnn #2', () => { + let res: number[] = []; - let count = state(0); - const read = () => { - const x = derived(() => ({ count: $.get(count) })); - return $.get(x); + const numbers = Array.from({ length: 2 }, (_, i) => i); + const fib = (n: number): number => (n < 2 ? 1 : fib(n - 1) + fib(n - 2)); + const hard = (n: number, l: string) => n + fib(16); + + const A = state(0); + const B = state(0); + + return () => { + const C = derived(() => ($.get(A) % 2) + ($.get(B) % 2)); + const D = derived(() => numbers.map((i) => i + ($.get(A) % 2) - ($.get(B) % 2))); + const E = derived(() => hard($.get(C) + $.get(A) + $.get(D)[0]!, 'E')); + const F = derived(() => hard($.get(D)[0]! && $.get(B), 'F')); + const G = derived(() => $.get(C) + ($.get(C) || $.get(E) % 2) + $.get(D)[0]! + $.get(F)); + + const destroy = effect_root(() => { + effect(() => { + res.push(hard($.get(G), 'H')); + }); + effect(() => { + res.push($.get(G)); + }); + effect(() => { + res.push(hard($.get(F), 'J')); + }); + }); + + flushSync(); + + let i = 2; + while (--i) { + res.length = 0; + set(B, 1); + set(A, 1 + i * 2); + flushSync(); + + set(A, 2 + i * 2); + set(B, 2); + flushSync(); + + assert.equal(res.length, 4); + assert.deepEqual(res, [3198, 1601, 3195, 1598]); + } + + destroy(); + assert(A.reactions === null); + assert(B.reactions === null); }; - const derivedCount = derived(() => read().count); - user_effect(() => { - log.push($.get(derivedCount)); - }); + }); + + test('effects correctly handle unowned derived values that do not change', () => { + const log: number[] = []; return () => { + let count = state(0); + const read = () => { + const x = derived(() => ({ count: $.get(count) })); + return $.get(x); + }; + const derivedCount = derived(() => read().count); + + const destroy = effect_root(() => { + user_effect(() => { + log.push($.get(derivedCount)); + }); + }); + flushSync(() => set(count, 1)); // Ensure we're not leaking consumers assert.deepEqual(count.reactions?.length, 1); @@ -248,6 +303,8 @@ describe('signals', () => { // Ensure we're not leaking consumers assert.deepEqual(count.reactions?.length, 1); assert.deepEqual(log, [0, 1, 2, 3]); + + destroy(); }; }); @@ -343,25 +400,69 @@ describe('signals', () => { }; }); - let some_state = state({}); - let some_deps = derived(() => { - return [$.get(some_state)]; - }); - test('two effects with an unowned derived that has some dependencies', () => { const log: Array> = []; - render_effect(() => { - log.push($.get(some_deps)); - }); + return () => { + let some_state = state({}); + let some_deps = derived(() => { + return [$.get(some_state)]; + }); + let destroy2: any; - render_effect(() => { - log.push($.get(some_deps)); - }); + const destroy = effect_root(() => { + render_effect(() => { + $.untrack(() => { + log.push($.get(some_deps)); + }); + }); - return () => { + destroy2 = effect_root(() => { + render_effect(() => { + log.push($.get(some_deps)); + }); + + render_effect(() => { + log.push($.get(some_deps)); + }); + }); + }); + + set(some_state, {}); + flushSync(); + + assert.deepEqual(log, [[{}], [{}], [{}], [{}], [{}]]); + + destroy2(); + + set(some_state, {}); flushSync(); - assert.deepEqual(log, [[{}], [{}]]); + + assert.deepEqual(log, [[{}], [{}], [{}], [{}], [{}]]); + + log.length = 0 + + const destroy3 = effect_root(() => { + render_effect(() => { + $.untrack(() => { + log.push($.get(some_deps)); + }); + log.push($.get(some_deps)); + }); + }); + + set(some_state, {}); + flushSync(); + + assert.deepEqual(log, [[{}], [{}], [{}], [{}]]); + + destroy3(); + + assert(some_state.reactions === null); + + destroy(); + + assert(some_state.reactions === null); }; }); From b05474692cf048a5c8997a479c1afc87328fb9ab Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 12:43:23 +0000 Subject: [PATCH 04/13] test case --- 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 4c65ea216491..046f833e0ed7 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -440,7 +440,7 @@ describe('signals', () => { assert.deepEqual(log, [[{}], [{}], [{}], [{}], [{}]]); - log.length = 0 + log.length = 0; const destroy3 = effect_root(() => { render_effect(() => { From a3398900832e11d4b0f3e997796d791f8fcdcd8a Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:04:42 +0000 Subject: [PATCH 05/13] cleanup unused logic --- .../internal/client/reactivity/deriveds.js | 2 - .../src/internal/client/reactivity/props.js | 50 ++++++------------- .../svelte/src/internal/client/runtime.js | 2 +- 3 files changed, 15 insertions(+), 39 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 60b55970e6b3..371c498c2463 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -3,7 +3,6 @@ import { DEV } from 'esm-env'; import { CLEAN, DERIVED, - DESTROYED, DIRTY, EFFECT_HAS_DERIVED, MAYBE_DIRTY, @@ -12,7 +11,6 @@ import { import { active_reaction, active_effect, - remove_reactions, set_signal_status, skip_reaction, update_reaction, diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index d157642d5d35..3d399676882a 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -241,26 +241,6 @@ export function spread_props(...props) { return new Proxy({ props }, spread_props_handler); } -/** - * @template T - * @param {() => T} fn - * @returns {T} - */ -function with_parent_branch(fn) { - var effect = active_effect; - var previous_effect = active_effect; - - while (effect !== null && (effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0) { - effect = effect.parent; - } - try { - set_active_effect(effect); - return fn(); - } finally { - set_active_effect(previous_effect); - } -} - /** * This function is responsible for synchronizing a possibly bound prop with the inner component state. * It is used whenever the compiler sees that the component writes to the prop, or when it has a default prop_value. @@ -335,8 +315,8 @@ export function prop(props, key, flags, fallback) { } else { // Svelte 4 did not trigger updates when a primitive value was updated to the same value. // Replicate that behavior through using a derived - var derived_getter = with_parent_branch(() => - (immutable ? derived : derived_safe_equal)(() => /** @type {V} */ (props[key])) + var derived_getter = (immutable ? derived : derived_safe_equal)( + () => /** @type {V} */ (props[key]) ); derived_getter.f |= LEGACY_DERIVED_PROP; getter = () => { @@ -380,21 +360,19 @@ export function prop(props, key, flags, fallback) { // The derived returns the current value. The underlying mutable // source is written to from various places to persist this value. var inner_current_value = mutable_source(prop_value); - var current_value = with_parent_branch(() => - derived(() => { - var parent_value = getter(); - var child_value = get(inner_current_value); - - if (from_child) { - from_child = false; - was_from_child = true; - return child_value; - } + var current_value = derived(() => { + var parent_value = getter(); + var child_value = get(inner_current_value); + + if (from_child) { + from_child = false; + was_from_child = true; + return child_value; + } - was_from_child = false; - return (inner_current_value.v = parent_value); - }) - ); + was_from_child = false; + return (inner_current_value.v = parent_value); + }); if (!immutable) current_value.equals = safe_equals; diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 923d89b0a8f4..7eccc4cfe41e 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -206,7 +206,7 @@ export function check_dirtiness(reaction) { // If the derived is owned by another derived then mark it as owned agaub // as the derived value might have been referenced in a different context // and now has a reacive context managing it - derived.f ^= UNOWNED; + // derived.f ^= UNOWNED; } } From 5cc1d14b775579cdb2a4e15b44f697f4d13a0fcf Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:09:37 +0000 Subject: [PATCH 06/13] simplify --- .../src/internal/client/reactivity/props.js | 45 ++++++++++++++++--- .../svelte/src/internal/client/runtime.js | 16 ++----- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 3d399676882a..5ba9b78e1b5f 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -10,7 +10,15 @@ import { import { get_descriptor, is_function } from '../../shared/utils.js'; import { mutable_source, set, source, update } from './sources.js'; import { derived, derived_safe_equal } from './deriveds.js'; -import { active_effect, get, captured_signals, set_active_effect, untrack } from '../runtime.js'; +import { + active_effect, + get, + captured_signals, + set_active_effect, + untrack, + active_reaction, + set_active_reaction +} from '../runtime.js'; import { safe_equals } from './equality.js'; import * as e from '../errors.js'; import { @@ -241,6 +249,29 @@ export function spread_props(...props) { return new Proxy({ props }, spread_props_handler); } +/** + * @template T + * @param {() => T} fn + * @returns {T} + */ +function with_parent_branch(fn) { + var effect = active_effect; + var previous_effect = active_effect; + var previous_reaction = active_reaction; + + while (effect !== null && (effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0) { + effect = effect.parent; + } + try { + set_active_effect(effect); + set_active_reaction(effect); + return fn(); + } finally { + set_active_effect(previous_effect); + set_active_reaction(previous_reaction); + } +} + /** * This function is responsible for synchronizing a possibly bound prop with the inner component state. * It is used whenever the compiler sees that the component writes to the prop, or when it has a default prop_value. @@ -259,11 +290,13 @@ export function prop(props, key, flags, fallback) { var is_store_sub = false; var prop_value; - if (bindable) { - [prop_value, is_store_sub] = capture_store_binding(() => /** @type {V} */ (props[key])); - } else { - prop_value = /** @type {V} */ (props[key]); - } + with_parent_branch(() => { + if (bindable) { + [prop_value, is_store_sub] = capture_store_binding(() => /** @type {V} */ (props[key])); + } else { + prop_value = /** @type {V} */ (props[key]); + } + }); // Can be the case when someone does `mount(Component, props)` with `let props = $state({...})` // or `createClassComponent(Component, props)` diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 7eccc4cfe41e..30f14b7356ce 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -184,29 +184,19 @@ export function check_dirtiness(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 (is_disconnected || is_unowned_connected) { - var derived = /** @type {Derived} */ (reaction); - for (i = 0; i < length; i++) { dependency = dependencies[i]; // We always re-add all reactions (even duplicates) if the derived was // previously disconnected, however we don't if it was unowned as we // de-duplicate dependencies in that case - if (is_disconnected || !dependency?.reactions?.includes(derived)) { - (dependency.reactions ??= []).push(derived); + if (is_disconnected || !dependency?.reactions?.includes(reaction)) { + (dependency.reactions ??= []).push(reaction); } } if (is_disconnected) { - derived.f ^= DISCONNECTED; - } - var parent = derived.parent; - - if (is_unowned_connected && parent !== null && (parent.f & UNOWNED) === 0) { - // If the derived is owned by another derived then mark it as owned agaub - // as the derived value might have been referenced in a different context - // and now has a reacive context managing it - // derived.f ^= UNOWNED; + reaction.f ^= DISCONNECTED; } } From ecb87680126b326e98578bfa663f720f98b5d618 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:10:33 +0000 Subject: [PATCH 07/13] simplify --- .changeset/nasty-pigs-lay.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/nasty-pigs-lay.md b/.changeset/nasty-pigs-lay.md index b7252b70f77b..c15f11525219 100644 --- a/.changeset/nasty-pigs-lay.md +++ b/.changeset/nasty-pigs-lay.md @@ -2,4 +2,4 @@ 'svelte': patch --- -fix: when an unowned derived is tracked again, remove unowned flag +fix: alter heuristic for reading data eagerly in props handling From a26b7797a7633929642187e8dee2b5533a5a5ae1 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:13:44 +0000 Subject: [PATCH 08/13] simplify --- packages/svelte/src/internal/client/reactivity/props.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 5ba9b78e1b5f..61f4108a009a 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -254,7 +254,7 @@ export function spread_props(...props) { * @param {() => T} fn * @returns {T} */ -function with_parent_branch(fn) { +function with_parent_tracking_context(fn) { var effect = active_effect; var previous_effect = active_effect; var previous_reaction = active_reaction; @@ -290,7 +290,7 @@ export function prop(props, key, flags, fallback) { var is_store_sub = false; var prop_value; - with_parent_branch(() => { + with_parent_tracking_context(() => { if (bindable) { [prop_value, is_store_sub] = capture_store_binding(() => /** @type {V} */ (props[key])); } else { @@ -331,7 +331,7 @@ export function prop(props, key, flags, fallback) { e.props_invalid_value(key); } - prop_value = get_fallback(); + prop_value = with_parent_tracking_context(get_fallback); if (setter) setter(prop_value); } From 2957e6c68f1456f8cca3643b8a5a57bf637962fe Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:16:29 +0000 Subject: [PATCH 09/13] simplify --- .../svelte/src/internal/client/reactivity/deriveds.js | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 371c498c2463..59a7ed0f16d6 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -1,13 +1,6 @@ /** @import { Derived, Effect } from '#client' */ import { DEV } from 'esm-env'; -import { - CLEAN, - DERIVED, - DIRTY, - EFFECT_HAS_DERIVED, - MAYBE_DIRTY, - UNOWNED -} from '../constants.js'; +import { CLEAN, DERIVED, DIRTY, EFFECT_HAS_DERIVED, MAYBE_DIRTY, UNOWNED } from '../constants.js'; import { active_reaction, active_effect, From 56cdbe41f74bb90c8c19891c3d9c998d7dbba496 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:30:06 +0000 Subject: [PATCH 10/13] add test --- .../svelte/src/internal/client/runtime.js | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 30f14b7356ce..b0fcf87fe856 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -184,19 +184,33 @@ export function check_dirtiness(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 (is_disconnected || is_unowned_connected) { + var derived = /** @type {Derived} */ (reaction); + for (i = 0; i < length; i++) { dependency = dependencies[i]; // We always re-add all reactions (even duplicates) if the derived was // previously disconnected, however we don't if it was unowned as we // de-duplicate dependencies in that case - if (is_disconnected || !dependency?.reactions?.includes(reaction)) { - (dependency.reactions ??= []).push(reaction); + if (is_disconnected || !dependency?.reactions?.includes(derived)) { + (dependency.reactions ??= []).push(derived); } } if (is_disconnected) { - reaction.f ^= DISCONNECTED; + derived.f ^= DISCONNECTED; + } + var parent = derived.parent; + // If the unowned derived is now fully connected to the graph again (it has reactions, has a parent and + // the parent is not unowned), then we can mark it as connected again, removing the need for the unowned + // flag + if ( + is_unowned_connected && + derived.reactions !== null && + parent !== null && + (parent.f & UNOWNED) === 0 + ) { + derived.f ^= UNOWNED; } } From 1c0bfc3fb0244bf6c5c3fa99b8eb72f9167a4c00 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:30:56 +0000 Subject: [PATCH 11/13] fix other issue --- .../samples/derived-unowned-12/_config.js | 25 +++++++++++++++++++ .../samples/derived-unowned-12/main.svelte | 18 +++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 packages/svelte/tests/runtime-runes/samples/derived-unowned-12/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/_config.js b/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/_config.js new file mode 100644 index 000000000000..8cd4af05488e --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/_config.js @@ -0,0 +1,25 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, target }) { + let [btn1, btn2] = target.querySelectorAll('button'); + + btn1?.click(); + flushSync(); + + btn2?.click(); + flushSync(); + + btn1?.click(); + flushSync(); + + btn1?.click(); + flushSync(); + + assert.htmlEqual( + target.innerHTML, + `\n3\n\n1` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte b/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte new file mode 100644 index 000000000000..f7d9329befc2 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte @@ -0,0 +1,18 @@ + + + {linked.current} + {count} From 3556152c890727fa2c6edeb5ed76707e4576bbec Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Mon, 10 Feb 2025 14:38:03 +0000 Subject: [PATCH 12/13] back to original plan --- .changeset/nasty-pigs-lay.md | 2 +- .../src/internal/client/reactivity/props.js | 37 +++---------------- .../svelte/src/internal/client/runtime.js | 13 ++----- 3 files changed, 11 insertions(+), 41 deletions(-) diff --git a/.changeset/nasty-pigs-lay.md b/.changeset/nasty-pigs-lay.md index c15f11525219..64ef72103a94 100644 --- a/.changeset/nasty-pigs-lay.md +++ b/.changeset/nasty-pigs-lay.md @@ -2,4 +2,4 @@ 'svelte': patch --- -fix: alter heuristic for reading data eagerly in props handling +fix: when re-connecting unowned deriveds, remove their unowned flag diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 61f4108a009a..5a3b30281f9f 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -249,29 +249,6 @@ export function spread_props(...props) { return new Proxy({ props }, spread_props_handler); } -/** - * @template T - * @param {() => T} fn - * @returns {T} - */ -function with_parent_tracking_context(fn) { - var effect = active_effect; - var previous_effect = active_effect; - var previous_reaction = active_reaction; - - while (effect !== null && (effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0) { - effect = effect.parent; - } - try { - set_active_effect(effect); - set_active_reaction(effect); - return fn(); - } finally { - set_active_effect(previous_effect); - set_active_reaction(previous_reaction); - } -} - /** * This function is responsible for synchronizing a possibly bound prop with the inner component state. * It is used whenever the compiler sees that the component writes to the prop, or when it has a default prop_value. @@ -290,13 +267,11 @@ export function prop(props, key, flags, fallback) { var is_store_sub = false; var prop_value; - with_parent_tracking_context(() => { - if (bindable) { - [prop_value, is_store_sub] = capture_store_binding(() => /** @type {V} */ (props[key])); - } else { - prop_value = /** @type {V} */ (props[key]); - } - }); + if (bindable) { + [prop_value, is_store_sub] = capture_store_binding(() => /** @type {V} */ (props[key])); + } else { + prop_value = /** @type {V} */ (props[key]); + } // Can be the case when someone does `mount(Component, props)` with `let props = $state({...})` // or `createClassComponent(Component, props)` @@ -331,7 +306,7 @@ export function prop(props, key, flags, fallback) { e.props_invalid_value(key); } - prop_value = with_parent_tracking_context(get_fallback); + prop_value = get_fallback(); if (setter) setter(prop_value); } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index b0fcf87fe856..096b01401d68 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -185,6 +185,7 @@ export function check_dirtiness(reaction) { // then we need to re-connect the reaction to the dependency if (is_disconnected || is_unowned_connected) { var derived = /** @type {Derived} */ (reaction); + var parent = derived.parent; for (i = 0; i < length; i++) { dependency = dependencies[i]; @@ -200,16 +201,10 @@ export function check_dirtiness(reaction) { if (is_disconnected) { derived.f ^= DISCONNECTED; } - var parent = derived.parent; - // If the unowned derived is now fully connected to the graph again (it has reactions, has a parent and - // the parent is not unowned), then we can mark it as connected again, removing the need for the unowned + // If the unowned derived is now fully connected to the graph again (it unowned and reconnected, has a parent + // and the parent is not unowned), then we can mark it as connected again, removing the need for the unowned // flag - if ( - is_unowned_connected && - derived.reactions !== null && - parent !== null && - (parent.f & UNOWNED) === 0 - ) { + if (is_unowned_connected && parent !== null && (parent.f & UNOWNED) === 0) { derived.f ^= UNOWNED; } } From 8a952bac8b7b312bcb4d732fa25b2099664e09a1 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 11 Feb 2025 13:58:55 +0100 Subject: [PATCH 13/13] Apply suggestions from code review --- packages/svelte/src/internal/client/runtime.js | 2 +- .../tests/runtime-runes/samples/derived-unowned-12/main.svelte | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 096b01401d68..8b0f84268c5a 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -201,7 +201,7 @@ export function check_dirtiness(reaction) { if (is_disconnected) { derived.f ^= DISCONNECTED; } - // If the unowned derived is now fully connected to the graph again (it unowned and reconnected, has a parent + // If the unowned derived is now fully connected to the graph again (it's unowned and reconnected, has a parent // and the parent is not unowned), then we can mark it as connected again, removing the need for the unowned // flag if (is_unowned_connected && parent !== null && (parent.f & UNOWNED) === 0) { diff --git a/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte b/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte index f7d9329befc2..48d4f5fd0b75 100644 --- a/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/derived-unowned-12/main.svelte @@ -8,7 +8,7 @@ count; untrack(() => state.current = count); - return untrack(() => state); + return untrack(() => state); }); linked.current++;