From 9ca41a3adda7224e015a45459ebd5ffa53ed0baa Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 1 Sep 2026 15:07:20 +0200 Subject: [PATCH] fix: compute teardown values more correctly --- .../phases/3-transform/client/types.d.ts | 2 + .../phases/3-transform/client/utils.js | 5 +- .../3-transform/client/visitors/Identifier.js | 24 ------ .../client/visitors/MemberExpression.js | 22 +++++ .../3-transform/client/visitors/Program.js | 6 +- .../client/visitors/SnippetBlock.js | 6 +- .../client/visitors/shared/utils.js | 6 +- .../client/dom/elements/bindings/this.js | 10 ++- packages/svelte/src/internal/client/index.js | 1 + .../src/internal/client/reactivity/effects.js | 9 +- .../src/internal/client/reactivity/props.js | 14 +-- .../svelte/src/internal/client/runtime.js | 86 +++++++++++++++++-- .../Child.svelte | 9 ++ .../_config.js | 19 ++++ .../main.svelte | 43 ++++++++++ .../samples/nested-effect-conflict/_config.js | 2 +- .../_expected/client/index.svelte.js | 6 +- 17 files changed, 218 insertions(+), 52 deletions(-) create mode 100644 packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/Child.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/main.svelte diff --git a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts index 7a95a2d43c..77ca04eace 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts +++ b/packages/svelte/src/compiler/phases/3-transform/client/types.d.ts @@ -26,6 +26,8 @@ export interface ClientTransformState extends TransformState { { /** turn `foo` into e.g. `$.get(foo)` */ read: (id: Identifier) => Expression; + /** turn `foo` into e.g. `$$props.foo` inside the template */ + read_template?: (id: Identifier) => Expression; /** turn `foo = bar` into e.g. `$.set(foo, bar)` */ assign?: (node: Identifier, value: Expression, proxy?: boolean) => Expression; /** turn `foo.bar = baz` into e.g. `$.mutate(foo, $.get(foo).bar = baz);` */ diff --git a/packages/svelte/src/compiler/phases/3-transform/client/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/utils.js index 9cdfa5cae1..11d5bc8f39 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -37,7 +37,10 @@ export function build_getter(node, state) { // don't transform the declaration itself if (node !== binding?.node) { - return state.transform[node.name].read(node); + var transform = state.transform[node.name]; + return state.is_instance || transform.read_template === undefined + ? transform.read(node) + : transform.read_template(node); } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Identifier.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Identifier.js index b43ec7891e..8525c7abc1 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Identifier.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Identifier.js @@ -16,30 +16,6 @@ export function Identifier(node, context) { return b.id('$$sanitized_props'); } - // Optimize prop access: If it's a member read access, we can use the $$props object directly - const binding = context.state.scope.get(node.name); - if ( - context.state.analysis.runes && // can't do this in legacy mode because the proxy does more than just read/write - binding !== null && - node !== binding.node && - binding.kind === 'rest_prop' - ) { - const grand_parent = context.path.at(-2); - - if ( - parent?.type === 'MemberExpression' && - !parent.computed && - grand_parent?.type !== 'AssignmentExpression' && - grand_parent?.type !== 'UpdateExpression' - ) { - const key = /** @type {Identifier} */ (parent.property); - - if (!binding.metadata?.exclude_props?.includes(key.name)) { - return b.id('$$props'); - } - } - } - return build_getter(node, context.state); } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js index 7e89b0e705..34393fb721 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/MemberExpression.js @@ -7,6 +7,28 @@ import * as b from '#compiler/builders'; * @param {Context} context */ export function MemberExpression(node, context) { + if ( + context.state.analysis.runes && + node.object.type === 'Identifier' && + node.property.type === 'Identifier' && + !node.computed + ) { + const binding = context.state.scope.get(node.object.name); + const parent = context.path.at(-1); + + if ( + binding?.kind === 'rest_prop' && + node.object !== binding.node && + parent?.type !== 'AssignmentExpression' && + parent?.type !== 'UpdateExpression' && + !binding.metadata?.exclude_props?.includes(node.property.name) + ) { + return context.state.is_instance + ? b.call('$.get_prop_value', b.id('$$props'), b.literal(node.property.name)) + : b.member(b.id('$$props'), node.property); + } + } + // rewrite `this.#foo` as `this.#foo.v` inside a constructor if (node.property.type === 'PrivateIdentifier') { const field = context.state.state_fields.get('#' + node.property.name); diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Program.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Program.js index f6352e986c..df2de61209 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Program.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Program.js @@ -126,11 +126,13 @@ export function Program(node, context) { const key = b.key(binding.prop_alias); context.state.transform[name] = { - read: (_) => b.member(b.id('$$props'), key, key.type === 'Literal') + read: (_) => b.call('$.get_prop_value', b.id('$$props'), b.literal(binding.prop_alias)), + read_template: (_) => b.member(b.id('$$props'), key, key.type === 'Literal') }; } else { context.state.transform[name] = { - read: (node) => b.member(b.id('$$props'), node) + read: (_) => b.call('$.get_prop_value', b.id('$$props'), b.literal(name)), + read_template: (node) => b.member(b.id('$$props'), node) }; } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SnippetBlock.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SnippetBlock.js index 6c3a48c5ea..67444f80e7 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SnippetBlock.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SnippetBlock.js @@ -63,7 +63,11 @@ export function SnippetBlock(node, context) { // we need to eagerly evaluate the expression in order to hit any // 'Cannot access x before initialization' errors if (dev) { - declarations.push(b.stmt(transform[name].read(b.id(name)))); + var read = + context.state.is_instance || transform[name].read_template === undefined + ? transform[name].read + : transform[name].read_template; + declarations.push(b.stmt(read(b.id(name)))); } } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js index 64ae573984..b2519c5426 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js @@ -416,7 +416,11 @@ export function validate_mutation(node, context, expression) { ? context.state.transform[left.property.name] : null; if (left.computed) { - path.unshift(transform?.read ? transform.read(left.property) : left.property); + var read = + state.is_instance || transform?.read_template === undefined + ? transform?.read + : transform.read_template; + path.unshift(read ? read(left.property) : left.property); } else { path.unshift(b.literal(left.property.name)); } diff --git a/packages/svelte/src/internal/client/dom/elements/bindings/this.js b/packages/svelte/src/internal/client/dom/elements/bindings/this.js index 705adc99ec..ed96d2a0f9 100644 --- a/packages/svelte/src/internal/client/dom/elements/bindings/this.js +++ b/packages/svelte/src/internal/client/dom/elements/bindings/this.js @@ -2,7 +2,7 @@ import { DESTROYING, STATE_SYMBOL } from '#client/constants'; import { component_context, mark_as_component } from '../../../context.js'; import { effect, render_effect } from '../../../reactivity/effects.js'; -import { active_effect, untrack } from '../../../runtime.js'; +import { active_effect, untrack, with_old_values } from '../../../runtime.js'; /** * @param {any} bound_value @@ -67,9 +67,11 @@ export function bind_this( p = p.parent; } const teardown = () => { - if (parts && is_bound_this(get_value(...parts), element_or_component)) { - update(null, ...parts); - } + with_old_values(() => { + if (parts && is_bound_this(get_value(...parts), element_or_component)) { + update(null, ...parts); + } + }); }; const original_teardown = p.teardown; p.teardown = () => { diff --git a/packages/svelte/src/internal/client/index.js b/packages/svelte/src/internal/client/index.js index 15e92aa255..d95605db5c 100644 --- a/packages/svelte/src/internal/client/index.js +++ b/packages/svelte/src/internal/client/index.js @@ -156,6 +156,7 @@ export { invalidate_inner_signals } from './legacy.js'; export { set_text } from './render.js'; export { get, + get_prop_value, safe_get, tick, untrack, diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js index e69a639a94..cd449ac015 100644 --- a/packages/svelte/src/internal/client/reactivity/effects.js +++ b/packages/svelte/src/internal/client/reactivity/effects.js @@ -3,12 +3,13 @@ import { is_dirty, active_effect, active_reaction, + destroying_effect, update_effect, get, is_destroying_effect, remove_reactions, set_active_reaction, - set_is_destroying_effect, + set_destroying_effect, untrack, untracking, set_active_effect @@ -445,9 +446,9 @@ export function branch(fn) { export function execute_effect_teardown(effect) { var teardown = effect.teardown; if (teardown !== null) { - const previously_destroying_effect = is_destroying_effect; + const previous_destroying_effect = destroying_effect; const previous_reaction = active_reaction; - set_is_destroying_effect(true); + set_destroying_effect(effect); set_active_reaction(null); try { teardown.call(null); @@ -457,7 +458,7 @@ export function execute_effect_teardown(effect) { // themselves mid-teardown are skipped by invoke_error_boundary. invoke_error_boundary(error, effect.parent); } finally { - set_is_destroying_effect(previously_destroying_effect); + set_destroying_effect(previous_destroying_effect); set_active_reaction(previous_reaction); } } diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 274d780b1e..829c3fd876 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -15,7 +15,8 @@ import { get, is_destroying_effect, set_active_effect, - untrack + untrack, + with_old_values } from '../runtime.js'; import * as e from '../errors.js'; import { DESTROYED, LEGACY_PROPS, STATE_SYMBOL } from '#client/constants'; @@ -54,7 +55,7 @@ export function update_pre_prop(fn, d = 1) { const rest_props_handler = { get(target, key) { if (target.exclude.has(key)) return; - return target.props[key]; + return with_old_values(() => target.props[key]); }, set(target, key) { if (DEV) { @@ -70,7 +71,7 @@ const rest_props_handler = { return { enumerable: true, configurable: true, - value: target.props[key] + value: with_old_values(() => target.props[key]) }; } }, @@ -359,7 +360,7 @@ export function prop(props, key, flags, fallback) { // prop is never written to — we only need a getter if (runes && (flags & PROPS_IS_UPDATED) === 0) { - return getter; + return () => with_old_values(getter); } // prop is written to, but the parent component had `bind:foo` which @@ -380,7 +381,7 @@ export function prop(props, key, flags, fallback) { return value; } - return getter(); + return with_old_values(getter); } ); } @@ -404,6 +405,7 @@ export function prop(props, key, flags, fallback) { if (bindable) get(d); var parent_effect = /** @type {Effect} */ (active_effect); + var get_derived = () => get(d); return /** @type {() => V} */ ( function (/** @type {any} */ value, /** @type {boolean} */ mutation) { @@ -427,7 +429,7 @@ export function prop(props, key, flags, fallback) { return d.v; } - return get(d); + return with_old_values(get_derived); } ); } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index a881403f58..c98cf3a8ce 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -68,9 +68,51 @@ let is_updating_effect = false; export let is_destroying_effect = false; -/** @param {boolean} value */ -export function set_is_destroying_effect(value) { - is_destroying_effect = value; +/** @type {Effect | null} */ +export let destroying_effect = null; + +let is_reading_old_value = false; +let old_value_read_version = 0; + +/** @param {Effect | null} effect */ +export function set_destroying_effect(effect) { + destroying_effect = effect; + is_destroying_effect = effect !== null; +} + +/** + * @template V + * @param {() => V} fn + * @returns {V} + */ +export function with_old_values(fn) { + if (!is_destroying_effect) return fn(); + + var previous_is_reading_old_value = is_reading_old_value; + is_reading_old_value = true; + + try { + return fn(); + } finally { + is_reading_old_value = previous_is_reading_old_value; + } +} + +/** + * @param {Record} props + * @param {string} key + */ +export function get_prop_value(props, key) { + if (!is_destroying_effect) return props[key]; + + var previous_is_reading_old_value = is_reading_old_value; + is_reading_old_value = true; + + try { + return props[key]; + } finally { + is_reading_old_value = previous_is_reading_old_value; + } } /** @type {null | Reaction} */ @@ -660,7 +702,12 @@ export function get(signal) { } } - if (is_destroying_effect && old_values.has(signal)) { + if ( + is_destroying_effect && + old_values.has(signal) && + (is_reading_old_value || reaction_depends_on(/** @type {Effect} */ (destroying_effect), signal)) + ) { + old_value_read_version += 1; return old_values.get(signal); } @@ -669,6 +716,7 @@ export function get(signal) { if (is_destroying_effect) { var value = derived.v; + var previous_old_value_read_version = old_value_read_version; // if the derived is dirty and has reactions, or depends on the values that just changed, re-execute // (a derived can be maybe_dirty due to the effect destroy removing its last reaction) @@ -679,7 +727,11 @@ export function get(signal) { value = execute_derived(derived); } - old_values.set(derived, value); + // Don't let a current value calculated for one teardown become the old value + // observed by a later teardown in the same flush. + if (old_value_read_version !== previous_old_value_read_version) { + old_values.set(derived, value); + } return value; } @@ -759,6 +811,30 @@ function depends_on_old_values(derived) { return false; } +/** + * @param {Reaction} reaction + * @param {Value} signal + * @param {Set} checked + */ +function reaction_depends_on(reaction, signal, checked = new Set()) { + if (reaction.deps === null || checked.has(reaction)) return false; + + checked.add(reaction); + + for (const dep of reaction.deps) { + if (dep === signal) return true; + + if ( + (dep.f & DERIVED) !== 0 && + reaction_depends_on(/** @type {Derived} */ (dep), signal, checked) + ) { + 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/effect-teardown-unrelated-state-write/Child.svelte b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/Child.svelte new file mode 100644 index 0000000000..e2b6c716ab --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/Child.svelte @@ -0,0 +1,9 @@ + diff --git a/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/_config.js b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/_config.js new file mode 100644 index 0000000000..3c758ca3e5 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/_config.js @@ -0,0 +1,19 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + compileOptions: { + accessors: false + }, + test({ assert, target, logs }) { + const button = target.querySelector('button'); + flushSync(() => button?.click()); + + assert.deepEqual(logs, [ + 'value = true, other = true', + 'track = 0, thing1 = false, thing2 = false, derived = false', + 'tracked derived = true', + 'tracked derived = false' + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/main.svelte b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/main.svelte new file mode 100644 index 0000000000..00739da158 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/main.svelte @@ -0,0 +1,43 @@ + + + + +{thing1} {thing2} + + diff --git a/packages/svelte/tests/runtime-runes/samples/nested-effect-conflict/_config.js b/packages/svelte/tests/runtime-runes/samples/nested-effect-conflict/_config.js index eb631bc9f4..ee0201c51d 100644 --- a/packages/svelte/tests/runtime-runes/samples/nested-effect-conflict/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/nested-effect-conflict/_config.js @@ -10,6 +10,6 @@ export default test({ }); await Promise.resolve(); - assert.deepEqual(logs, ['top level', 'inner', 0, 'destroy inner', 0, 'destroy outer', 0]); + assert.deepEqual(logs, ['top level', 'inner', 0, 'destroy inner', 0, 'destroy outer', 1]); } }); diff --git a/packages/svelte/tests/snapshot/samples/props-identifier/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/props-identifier/_expected/client/index.svelte.js index 7df616f694..8c5db59d4c 100644 --- a/packages/svelte/tests/snapshot/samples/props-identifier/_expected/client/index.svelte.js +++ b/packages/svelte/tests/snapshot/samples/props-identifier/_expected/client/index.svelte.js @@ -8,10 +8,10 @@ export default function Props_identifier($$anchor, $$props) { let props = $.rest_props($$props, rest_excludes); - $$props.a; + $.get_prop_value($$props, 'a'); props[a]; - $$props.a.b; - $$props.a.b = true; + $.get_prop_value($$props, 'a').b; + $.get_prop_value($$props, 'a').b = true; props.a = true; props[a] = true; props;