diff --git a/.changeset/light-rivers-jump.md b/.changeset/light-rivers-jump.md new file mode 100644 index 000000000000..2454d5715602 --- /dev/null +++ b/.changeset/light-rivers-jump.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: re-evaluate derived props during teardown diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index d3123d24a1ca..37e8643e1618 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -19,6 +19,7 @@ import { inspect_effects, set_inspect_effects } from './sources.js'; import { get_stack } from '../dev/tracing.js'; import { tracing_mode_flag } from '../../flags/index.js'; import { component_context } from '../context.js'; +import { UNINITIALIZED } from '../../../constants.js'; /** * @template V @@ -51,7 +52,7 @@ export function derived(fn) { fn, reactions: null, rv: 0, - v: /** @type {V} */ (null), + v: /** @type {V} */ (UNINITIALIZED), wv: 0, parent: parent_derived ?? active_effect, ac: null diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 3501bcd3c722..03daad5251f2 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -1,18 +1,26 @@ -/** @import { Derived, Source } from './types.js' */ +/** @import { ComponentContext } from '#client' */ +/** @import { Derived, Effect, Source } from './types.js' */ import { DEV } from 'esm-env'; import { PROPS_IS_BINDABLE, PROPS_IS_IMMUTABLE, PROPS_IS_LAZY_INITIAL, PROPS_IS_RUNES, - PROPS_IS_UPDATED + PROPS_IS_UPDATED, + UNINITIALIZED } from '../../../constants.js'; import { get_descriptor, is_function } from '../../shared/utils.js'; import { set, source, update } from './sources.js'; import { derived, derived_safe_equal } from './deriveds.js'; -import { get, untrack } from '../runtime.js'; +import { + active_effect, + get, + is_destroying_effect, + set_active_effect, + untrack +} from '../runtime.js'; import * as e from '../errors.js'; -import { LEGACY_PROPS, STATE_SYMBOL } from '#client/constants'; +import { DESTROYED, LEGACY_PROPS, STATE_SYMBOL } from '#client/constants'; import { proxy } from '../proxy.js'; import { capture_store_binding } from './store.js'; import { legacy_mode_flag } from '../../flags/index.js'; @@ -92,7 +100,7 @@ export function rest_props(props, exclude, name) { /** * The proxy handler for legacy $$restProps and $$props - * @type {ProxyHandler<{ props: Record, exclude: Array, special: Record unknown>, version: Source }>}} + * @type {ProxyHandler<{ props: Record, exclude: Array, special: Record unknown>, version: Source, parent_effect: Effect }>}} */ const legacy_rest_props_handler = { get(target, key) { @@ -102,17 +110,25 @@ const legacy_rest_props_handler = { }, set(target, key, value) { if (!(key in target.special)) { - // Handle props that can temporarily get out of sync with the parent - /** @type {Record unknown>} */ - target.special[key] = prop( - { - get [key]() { - return target.props[key]; - } - }, - /** @type {string} */ (key), - PROPS_IS_UPDATED - ); + var previous_effect = active_effect; + + try { + set_active_effect(target.parent_effect); + + // Handle props that can temporarily get out of sync with the parent + /** @type {Record unknown>} */ + target.special[key] = prop( + { + get [key]() { + return target.props[key]; + } + }, + /** @type {string} */ (key), + PROPS_IS_UPDATED + ); + } finally { + set_active_effect(previous_effect); + } } target.special[key](value); @@ -151,7 +167,19 @@ const legacy_rest_props_handler = { * @returns {Record} */ export function legacy_rest_props(props, exclude) { - return new Proxy({ props, exclude, special: {}, version: source(0) }, legacy_rest_props_handler); + return new Proxy( + { + props, + exclude, + special: {}, + version: source(0), + // TODO this is only necessary because we need to track component + // destruction inside `prop`, because of `bind:this`, but it + // seems likely that we can simplify `bind:this` instead + parent_effect: /** @type {Effect} */ (active_effect) + }, + legacy_rest_props_handler + ); } /** @@ -366,16 +394,24 @@ export function prop(props, key, flags, fallback) { // create a derived that we can write to locally. // Or we are in legacy mode where we always create a derived to replicate that // Svelte 4 did not trigger updates when a primitive value was updated to the same value. - var d = ((flags & PROPS_IS_IMMUTABLE) !== 0 ? derived : derived_safe_equal)(getter); + var overridden = false; + + var d = ((flags & PROPS_IS_IMMUTABLE) !== 0 ? derived : derived_safe_equal)(() => { + overridden = false; + return getter(); + }); // Capture the initial value if it's bindable if (bindable) get(d); + var parent_effect = /** @type {Effect} */ (active_effect); + return function (/** @type {any} */ value, /** @type {boolean} */ mutation) { if (arguments.length > 0) { const new_value = mutation ? get(d) : runes && bindable ? proxy(value) : value; set(d, new_value); + overridden = true; if (fallback_value !== undefined) { fallback_value = new_value; @@ -384,8 +420,12 @@ export function prop(props, key, flags, fallback) { return value; } - // TODO is this still necessary post-#16263? - if (has_destroyed_component_ctx(d)) { + // special case — avoid recalculating the derived if we're in a + // teardown function and the prop was overridden locally, or the + // component was already destroyed (this latter part is necessary + // because `bind:this` can read props after the component has + // been destroyed. TODO simplify `bind:this` + if ((is_destroying_effect && overridden) || (parent_effect.f & DESTROYED) !== 0) { return d.v; } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 8e6242447e73..5d3a7b49fc27 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -26,7 +26,7 @@ import { } from './constants.js'; import { flush_tasks } from './dom/task.js'; import { internal_set, old_values } from './reactivity/sources.js'; -import { destroy_derived_effects, update_derived } from './reactivity/deriveds.js'; +import { destroy_derived_effects, execute_derived, update_derived } from './reactivity/deriveds.js'; import * as e from './errors.js'; import { tracing_mode_flag } from '../flags/index.js'; @@ -41,6 +41,7 @@ import { set_dev_stack } from './context.js'; import { handle_error, invoke_error_boundary } from './error-handling.js'; +import { UNINITIALIZED } from '../../constants.js'; let is_flushing = false; @@ -774,7 +775,7 @@ export function get(signal) { } } - if (is_derived) { + if (is_derived && !is_destroying_effect) { derived = /** @type {Derived} */ (signal); if (check_dirtiness(derived)) { @@ -815,13 +816,48 @@ export function get(signal) { } } - if (is_destroying_effect && old_values.has(signal)) { - return old_values.get(signal); + if (is_destroying_effect) { + if (old_values.has(signal)) { + return old_values.get(signal); + } + + if (is_derived) { + derived = /** @type {Derived} */ (signal); + + var value = derived.v; + + // if the derived is dirty, or depends on the values that just changed, re-execute + if ((derived.f & CLEAN) !== 0 || depends_on_old_values(derived)) { + value = execute_derived(derived); + } + + old_values.set(derived, value); + + return value; + } } return signal.v; } +/** @param {Derived} derived */ +function depends_on_old_values(derived) { + if (derived.v === UNINITIALIZED) return true; // we don't know, so assume the worst + if (derived.deps === null) return false; + + for (const dep of derived.deps) { + if (old_values.has(dep)) { + return true; + } + + if ((dep.f & DERIVED) !== 0 && depends_on_old_values(/** @type {Derived} */ (dep))) { + return true; + } + } + + return false; +} + /** * Like `get`, but checks for `undefined`. Used for `var` declarations because they can be accessed before being declared * @template V diff --git a/packages/svelte/tests/runtime-runes/samples/derived-cleanup-old-value/_config.js b/packages/svelte/tests/runtime-runes/samples/derived-cleanup-old-value/_config.js new file mode 100644 index 000000000000..694dccdcf8a0 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/derived-cleanup-old-value/_config.js @@ -0,0 +1,22 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + async test({ assert, logs, target }) { + const [increment] = target.querySelectorAll('button'); + + flushSync(() => increment.click()); + flushSync(() => increment.click()); + flushSync(() => increment.click()); + + assert.deepEqual(logs, [ + 'count: 1', + 'squared: 1', + 'count: 2', + 'squared: 4', + 'count: 3', + 'squared: 9', + 'count: 4' + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/derived-cleanup-old-value/main.svelte b/packages/svelte/tests/runtime-runes/samples/derived-cleanup-old-value/main.svelte new file mode 100644 index 000000000000..a4c58a8e991c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/derived-cleanup-old-value/main.svelte @@ -0,0 +1,20 @@ + + + + +

count: {count}

+ +{#if count % 2 === 0} +

squared: {squared}

+{/if} diff --git a/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/Component.svelte b/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/Component.svelte new file mode 100644 index 000000000000..3c863c7fc94c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/Component.svelte @@ -0,0 +1,15 @@ + + +

{count}

+ + + diff --git a/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/_config.js b/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/_config.js new file mode 100644 index 000000000000..1f6e5d1f7f96 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/_config.js @@ -0,0 +1,17 @@ +import { test } from '../../test'; +import { flushSync } from 'svelte'; + +export default test({ + async test({ assert, target, logs }) { + const [increment, toggle] = target.querySelectorAll('button'); + + flushSync(() => toggle.click()); + assert.deepEqual(logs, [0, 'hello']); + + flushSync(() => toggle.click()); + flushSync(() => increment.click()); + flushSync(() => increment.click()); + + assert.deepEqual(logs, [0, 'hello', 1, 'hello']); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/main.svelte b/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/main.svelte new file mode 100644 index 000000000000..1a97d3e760d6 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-derived/main.svelte @@ -0,0 +1,13 @@ + + + + + +{#if count < 2 && message === 'hello'} + +{/if}