From 999b8e5ea08ac9e7e4048ac24fb31128969322b2 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 30 Oct 2024 16:22:44 +0000 Subject: [PATCH 1/6] fix: ensure legacy props cache last value when destroyed --- .changeset/khaki-carrots-mix.md | 5 +++++ .../src/internal/client/reactivity/props.js | 17 ++++++++++++++--- .../samples/props-reactive-destroy/Child.svelte | 9 +++++++++ .../samples/props-reactive-destroy/_config.js | 10 ++++++++++ .../samples/props-reactive-destroy/main.svelte | 17 +++++++++++++++++ 5 files changed, 55 insertions(+), 3 deletions(-) create mode 100644 .changeset/khaki-carrots-mix.md create mode 100644 packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte create mode 100644 packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/_config.js create mode 100644 packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/main.svelte diff --git a/.changeset/khaki-carrots-mix.md b/.changeset/khaki-carrots-mix.md new file mode 100644 index 000000000000..13f7b90cb930 --- /dev/null +++ b/.changeset/khaki-carrots-mix.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: ensure legacy props cache last value when destroyed diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index eec8e70dc68d..0cb5d3bd9d33 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -324,12 +324,23 @@ 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 = with_parent_branch(() => { + return (immutable ? derived : derived_safe_equal)(() => /** @type {V} */ (props[key])); + }); + // Connect the derived getter to the parent branch effect + get(derived_getter); + derived_getter.f |= LEGACY_DERIVED_PROP; + /** @type {V} */ + var last_value; + getter = () => { + // Emulate the Svelte 4 behaviour of returning the last known value + if ((derived_getter.f & DESTROYED) !== 0) { + return last_value; + } var value = get(derived_getter); + last_value = value; if (value !== undefined) fallback_value = /** @type {V} */ (undefined); return value === undefined ? fallback_value : value; }; diff --git a/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte new file mode 100644 index 000000000000..2051517eafb0 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte @@ -0,0 +1,9 @@ + diff --git a/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/_config.js b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/_config.js new file mode 100644 index 000000000000..363c850c8bdb --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/_config.js @@ -0,0 +1,10 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + test({ assert, logs, target }) { + target.querySelector('button')?.click(); + flushSync(); + assert.deepEqual(logs, ['should fire once']); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/main.svelte b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/main.svelte new file mode 100644 index 000000000000..00c6a5f71c93 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/main.svelte @@ -0,0 +1,17 @@ + + + + +{#if active} + +{/if} \ No newline at end of file From 60d5da15da66c0299d984bea4831a6632f0d4f0c Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 30 Oct 2024 18:19:04 +0000 Subject: [PATCH 2/6] fix runes --- .changeset/khaki-carrots-mix.md | 2 +- .../src/internal/client/reactivity/props.js | 39 +++++++++++-------- .../props-reactive-destroy/Child.svelte | 2 + 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/.changeset/khaki-carrots-mix.md b/.changeset/khaki-carrots-mix.md index 13f7b90cb930..bab5e9d86e6c 100644 --- a/.changeset/khaki-carrots-mix.md +++ b/.changeset/khaki-carrots-mix.md @@ -2,4 +2,4 @@ 'svelte': patch --- -fix: ensure legacy props cache last value when destroyed +fix: ensure derived props cache last value when destroyed diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 0cb5d3bd9d33..ccc6e404548d 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -313,36 +313,42 @@ export function prop(props, key, flags, fallback) { /** @type {() => V} */ var getter; + var legacy_parent = props.$$legacy; + // 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(() => { + return ((!runes && immutable) || (runes && !legacy_parent) ? derived : derived_safe_equal)( + () => /** @type {V} */ (props[key]) + ); + }); + // Connect the derived getter to the parent branch effect + get(derived_getter); + /** @type {V} */ + var last_value; + if (runes) { getter = () => { - var value = /** @type {V} */ (props[key]); - if (value === undefined) return get_fallback(); + // If the derived has been destroyed, use the last value we have + if ((derived_getter.f & DESTROYED) !== 0) { + return last_value; + } + var value = get(derived_getter); + if (value === undefined) return (last_value = get_fallback()); fallback_dirty = true; fallback_used = false; + last_value = value; return value; }; } 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(() => { - return (immutable ? derived : derived_safe_equal)(() => /** @type {V} */ (props[key])); - }); - // Connect the derived getter to the parent branch effect - get(derived_getter); - derived_getter.f |= LEGACY_DERIVED_PROP; - /** @type {V} */ - var last_value; - getter = () => { - // Emulate the Svelte 4 behaviour of returning the last known value + // If the derived has been destroyed, use the last value we have if ((derived_getter.f & DESTROYED) !== 0) { return last_value; } var value = get(derived_getter); - last_value = value; if (value !== undefined) fallback_value = /** @type {V} */ (undefined); - return value === undefined ? fallback_value : value; + return (last_value = value === undefined ? fallback_value : value); }; } @@ -354,7 +360,6 @@ export function prop(props, key, flags, fallback) { // intermediate mode — prop is written to, but the parent component had // `bind:foo` which means we can just call `$$props.foo = value` directly if (setter) { - var legacy_parent = props.$$legacy; return function (/** @type {any} */ value, /** @type {boolean} */ mutation) { if (arguments.length > 0) { // We don't want to notify if the value was mutated and the parent is in runes mode. diff --git a/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte index 2051517eafb0..dc6326f8c4e8 100644 --- a/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte +++ b/packages/svelte/tests/runtime-legacy/samples/props-reactive-destroy/Child.svelte @@ -7,3 +7,5 @@ data; }); + +{data ? '' : null} \ No newline at end of file From cdce350f32509d88648bd60ba354f9e2bed2bb1c Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 30 Oct 2024 18:54:39 +0000 Subject: [PATCH 3/6] better approach --- .changeset/khaki-carrots-mix.md | 2 +- .../src/internal/client/reactivity/effects.js | 2 +- .../src/internal/client/reactivity/props.js | 34 +++++-------------- .../props-reactive-destroy/Child.svelte | 11 ++++++ .../samples/props-reactive-destroy/_config.js | 10 ++++++ .../props-reactive-destroy/main.svelte | 17 ++++++++++ 6 files changed, 49 insertions(+), 27 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/Child.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/main.svelte diff --git a/.changeset/khaki-carrots-mix.md b/.changeset/khaki-carrots-mix.md index bab5e9d86e6c..f5ef6b641881 100644 --- a/.changeset/khaki-carrots-mix.md +++ b/.changeset/khaki-carrots-mix.md @@ -2,4 +2,4 @@ 'svelte': patch --- -fix: ensure derived props cache last value when destroyed +fix: ensure child effects are destroyed before their deriveds diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index 82baeae28805..daf28eeec236 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -437,8 +437,8 @@ export function destroy_effect(effect, remove_dom = true) { removed = true; } - destroy_effect_deriveds(effect); 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/props.js b/packages/svelte/src/internal/client/reactivity/props.js index ccc6e404548d..eec8e70dc68d 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -313,42 +313,25 @@ export function prop(props, key, flags, fallback) { /** @type {() => V} */ var getter; - var legacy_parent = props.$$legacy; - // 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(() => { - return ((!runes && immutable) || (runes && !legacy_parent) ? derived : derived_safe_equal)( - () => /** @type {V} */ (props[key]) - ); - }); - // Connect the derived getter to the parent branch effect - get(derived_getter); - /** @type {V} */ - var last_value; - if (runes) { getter = () => { - // If the derived has been destroyed, use the last value we have - if ((derived_getter.f & DESTROYED) !== 0) { - return last_value; - } - var value = get(derived_getter); - if (value === undefined) return (last_value = get_fallback()); + var value = /** @type {V} */ (props[key]); + if (value === undefined) return get_fallback(); fallback_dirty = true; fallback_used = false; - last_value = value; return value; }; } 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])) + ); derived_getter.f |= LEGACY_DERIVED_PROP; getter = () => { - // If the derived has been destroyed, use the last value we have - if ((derived_getter.f & DESTROYED) !== 0) { - return last_value; - } var value = get(derived_getter); if (value !== undefined) fallback_value = /** @type {V} */ (undefined); - return (last_value = value === undefined ? fallback_value : value); + return value === undefined ? fallback_value : value; }; } @@ -360,6 +343,7 @@ export function prop(props, key, flags, fallback) { // intermediate mode — prop is written to, but the parent component had // `bind:foo` which means we can just call `$$props.foo = value` directly if (setter) { + var legacy_parent = props.$$legacy; return function (/** @type {any} */ value, /** @type {boolean} */ mutation) { if (arguments.length > 0) { // We don't want to notify if the value was mutated and the parent is in runes mode. diff --git a/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/Child.svelte b/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/Child.svelte new file mode 100644 index 000000000000..d16689b07d78 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/Child.svelte @@ -0,0 +1,11 @@ + + +{data ? '' : null} diff --git a/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/_config.js b/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/_config.js new file mode 100644 index 000000000000..363c850c8bdb --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/_config.js @@ -0,0 +1,10 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + test({ assert, logs, target }) { + target.querySelector('button')?.click(); + flushSync(); + assert.deepEqual(logs, ['should fire once']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/main.svelte b/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/main.svelte new file mode 100644 index 000000000000..604237072ae4 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/props-reactive-destroy/main.svelte @@ -0,0 +1,17 @@ + + + + +{#if active} + +{/if} From cbf3b09bd72f35d809e090447214dd752a805119 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 30 Oct 2024 18:59:04 +0000 Subject: [PATCH 4/6] better approach --- packages/svelte/src/internal/client/runtime.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 26c8d9adbba6..6bed3825c758 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -432,12 +432,12 @@ export function update_effect(effect) { } try { - destroy_effect_deriveds(effect); if ((flags & BLOCK_EFFECT) !== 0) { destroy_block_effect_children(effect); } else { destroy_effect_children(effect); } + destroy_effect_deriveds(effect); execute_effect_teardown(effect); var teardown = update_reaction(effect); From 1f82c82d63008c40ccf8b76e2f42c6c6d6f46bdc Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 30 Oct 2024 19:02:56 +0000 Subject: [PATCH 5/6] kill code --- .../svelte/src/internal/client/reactivity/props.js | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index eec8e70dc68d..c24ba4b89e58 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -12,7 +12,6 @@ import { mutable_source, set, source } from './sources.js'; import { derived, derived_safe_equal } from './deriveds.js'; import { active_effect, - active_reaction, get, is_signals_recorded, set_active_effect, @@ -21,7 +20,7 @@ import { } from '../runtime.js'; import { safe_equals } from './equality.js'; import * as e from '../errors.js'; -import { BRANCH_EFFECT, DESTROYED, LEGACY_DERIVED_PROP, ROOT_EFFECT } from '../constants.js'; +import { BRANCH_EFFECT, LEGACY_DERIVED_PROP, ROOT_EFFECT } from '../constants.js'; import { proxy } from '../proxy.js'; import { capture_store_binding } from './store.js'; @@ -369,17 +368,12 @@ 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); - var current_derived = /** @type {Derived} */ (active_reaction); - // If the getter from the parent returns undefined, switch - // to using the local value from inner_current_value instead, - // as the parent value might have been torn down - if (from_child || (parent_value === undefined && (current_derived.f & DESTROYED) !== 0)) { + if (from_child) { from_child = false; was_from_child = true; return child_value; From 56499641d9ba7eb65497a8b9d90f08be0f853ccf Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Wed, 30 Oct 2024 19:03:36 +0000 Subject: [PATCH 6/6] lint --- packages/svelte/src/internal/client/reactivity/props.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index c24ba4b89e58..e24deb5a20c2 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -1,4 +1,4 @@ -/** @import { Derived, Source } from './types.js' */ +/** @import { Source } from './types.js' */ import { DEV } from 'esm-env'; import { PROPS_IS_BINDABLE,