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..d402d764da 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -13,6 +13,7 @@ import { PROPS_IS_UPDATED, PROPS_IS_BINDABLE } from '../../../../constants.js'; +import { get_rune } from '../../scope.js'; /** * @param {Binding} binding @@ -44,6 +45,185 @@ export function build_getter(node, state) { return node; } +/** + * @param {Binding} binding + * @param {Identifier} node + * @param {ClientTransformState} state + */ +export function prop_read_may_be_in_teardown(binding, node, state) { + const reference = binding.references.find((reference) => reference.node === node); + if (reference === undefined) return false; + + return reference_may_be_in_teardown(reference, state, new Set()); +} + +/** + * @param {Binding['references'][number]} reference + * @param {ClientTransformState} state + * @param {Set} checked + */ +function reference_may_be_in_teardown(reference, state, checked) { + const { path } = reference; + + for (let i = path.length - 1; i >= 0; i -= 1) { + const fn = path[i]; + + if ( + fn.type !== 'ArrowFunctionExpression' && + fn.type !== 'FunctionExpression' && + fn.type !== 'FunctionDeclaration' + ) { + continue; + } + + const parent = path[i - 1]; + + if (parent?.type === 'CallExpression') { + // An IIFE executes in the same context as the function containing it. + if (parent.callee === fn) continue; + + if (parent.arguments.includes(/** @type {Expression} */ (fn))) { + const rune = get_rune(parent, get_scope(path, i - 1, state)); + + if (is_effect_rune(rune)) return false; + if (rune === '$derived.by') { + return derived_may_be_read_in_teardown(parent, path, i - 1, state, checked); + } + } + } + + const function_binding = get_function_binding(fn, path, i, state); + return function_binding === null + ? true + : function_may_be_called_in_teardown(function_binding, state, checked); + } + + for (let i = path.length - 1; i >= 0; i -= 1) { + const node = path[i]; + if (node.type !== 'CallExpression') continue; + + const rune = get_rune(node, get_scope(path, i, state)); + if (rune === '$derived' || rune === '$derived.by') { + return derived_may_be_read_in_teardown(node, path, i, state, checked); + } + } + + return false; +} + +/** + * @param {Binding} binding + * @param {ClientTransformState} state + * @param {Set} checked + */ +function function_may_be_called_in_teardown(binding, state, checked) { + if (checked.has(binding)) return false; + checked.add(binding); + + for (const reference of binding.references) { + if (reference.node === binding.node) continue; + + const parent = reference.path.at(-1); + + if (parent?.type !== 'CallExpression') return true; + + if (parent.callee === reference.node) { + if (reference_may_be_in_teardown(reference, state, checked)) return true; + continue; + } + + if (!parent.arguments.includes(reference.node)) return true; + + const rune = get_rune(parent, get_scope(reference.path, reference.path.length - 1, state)); + + if (is_effect_rune(rune)) continue; + if ( + rune === '$derived.by' && + !derived_may_be_read_in_teardown( + parent, + reference.path, + reference.path.length - 1, + state, + checked + ) + ) { + continue; + } + + return true; + } + + return false; +} + +/** + * @param {import('estree').CallExpression} call + * @param {import('#compiler').AST.SvelteNode[]} path + * @param {number} index + * @param {ClientTransformState} state + * @param {Set} checked + */ +function derived_may_be_read_in_teardown(call, path, index, state, checked) { + const parent = path[index - 1]; + if (parent?.type !== 'VariableDeclarator' || parent.init !== call) return true; + if (parent.id.type !== 'Identifier') return true; + + const binding = get_scope(path, index - 1, state).get(parent.id.name); + if (binding === null || checked.has(binding)) return false; + + checked.add(binding); + + for (const reference of binding.references) { + if (reference.node === binding.node) continue; + if (reference_may_be_in_teardown(reference, state, checked)) return true; + } + + return false; +} + +/** + * @param {import('estree').FunctionDeclaration | import('estree').FunctionExpression | import('estree').ArrowFunctionExpression} fn + * @param {import('#compiler').AST.SvelteNode[]} path + * @param {number} index + * @param {ClientTransformState} state + * @returns {Binding | null} + */ +function get_function_binding(fn, path, index, state) { + if (fn.type === 'FunctionDeclaration' && fn.id !== null) { + return get_scope(path, index - 1, state).get(fn.id.name); + } + + const parent = path[index - 1]; + if ( + parent?.type === 'VariableDeclarator' && + parent.init === fn && + parent.id.type === 'Identifier' + ) { + return get_scope(path, index - 1, state).get(parent.id.name); + } + + return null; +} + +/** + * @param {import('#compiler').AST.SvelteNode[]} path + * @param {number} index + * @param {ClientTransformState} state + */ +function get_scope(path, index, state) { + for (let i = index; i >= 0; i -= 1) { + const scope = state.scopes.get(path[i]); + if (scope !== undefined) return scope; + } + + return state.scope; +} + +/** @param {string | null} rune */ +function is_effect_rune(rune) { + return rune === '$effect' || rune === '$effect.pre' || rune === '$effect.root'; +} + /** * @param {Binding} binding * @param {ComponentClientTransformState} state 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..1e339798d1 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 @@ -2,7 +2,7 @@ /** @import { Context } from '../types' */ import is_reference from 'is-reference'; import * as b from '#compiler/builders'; -import { build_getter } from '../utils.js'; +import { build_getter, is_prop_source, prop_read_may_be_in_teardown } from '../utils.js'; /** * @param {Identifier} node @@ -16,28 +16,20 @@ 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 + context.state.is_instance && binding !== null && node !== binding.node && - binding.kind === 'rest_prop' + (binding.kind === 'prop' || binding.kind === 'bindable_prop') && + !is_prop_source(binding, context.state) && + prop_read_may_be_in_teardown(binding, node, context.state) ) { - 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 b.call( + '$.get_prop_value', + b.id('$$props'), + b.literal(binding.prop_alias ?? node.name) + ); } 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..86d51234ce 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 @@ -1,12 +1,36 @@ /** @import { MemberExpression } from 'estree' */ /** @import { Context } from '../types' */ import * as b from '#compiler/builders'; +import { prop_read_may_be_in_teardown } from '../utils.js'; /** * @param {MemberExpression} node * @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 && + prop_read_may_be_in_teardown(binding, node.object, context.state) + ? 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/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 27def05300..eef1620cf7 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -67,9 +67,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} */ @@ -656,7 +698,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); } @@ -665,6 +712,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) @@ -675,7 +723,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; } @@ -755,6 +807,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..9bb373117c --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/effect-teardown-unrelated-state-write/Child.svelte @@ -0,0 +1,10 @@ + 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-teardown-optimization/_expected/client/index.svelte.js b/packages/svelte/tests/snapshot/samples/props-teardown-optimization/_expected/client/index.svelte.js new file mode 100644 index 0000000000..81c6d9c18e --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/props-teardown-optimization/_expected/client/index.svelte.js @@ -0,0 +1,25 @@ +import 'svelte/internal/disclose-version'; +import * as $ from 'svelte/internal/client'; + +export default function Props_teardown_optimization($$anchor, $$props) { + $.push($$props, true); + + let doubled = $.derived(() => $$props.derived_prop * 2); + let indirect = $.derived(() => $.get_prop_value($$props, 'indirect_prop')); + let read_function_prop = () => $$props.function_prop; + let read_function_cleanup_prop = () => $.get_prop_value($$props, 'function_cleanup_prop'); + + $.user_effect(() => console.log($$props.effect_prop)); + $.user_effect(() => console.log(read_function_prop())); + $.user_effect(() => () => console.log($.get_prop_value($$props, 'cleanup_prop'))); + $.user_effect(() => () => console.log($.get(indirect))); + $.user_effect(() => () => console.log(read_function_cleanup_prop())); + someFunction(() => $.get_prop_value($$props, 'unknown_prop')); + $.next(); + + var text = $.text(); + + $.template_effect(() => $.set_text(text, $.get(doubled))); + $.append($$anchor, text); + $.pop(); +} \ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/props-teardown-optimization/_expected/server/index.svelte.js b/packages/svelte/tests/snapshot/samples/props-teardown-optimization/_expected/server/index.svelte.js new file mode 100644 index 0000000000..8f689df23b --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/props-teardown-optimization/_expected/server/index.svelte.js @@ -0,0 +1,23 @@ +import * as $ from 'svelte/internal/server'; + +export default function Props_teardown_optimization($$renderer, $$props) { + $$renderer.component(($$renderer) => { + let { + derived_prop, + indirect_prop, + function_prop, + function_cleanup_prop, + effect_prop, + unknown_prop, + cleanup_prop + } = $$props; + + let doubled = $.derived(() => derived_prop * 2); + let indirect = $.derived(() => indirect_prop); + let read_function_prop = () => function_prop; + let read_function_cleanup_prop = () => function_cleanup_prop; + + someFunction(() => unknown_prop); + $$renderer.push(`${$.escape(doubled())}`); + }); +} \ No newline at end of file diff --git a/packages/svelte/tests/snapshot/samples/props-teardown-optimization/index.svelte b/packages/svelte/tests/snapshot/samples/props-teardown-optimization/index.svelte new file mode 100644 index 0000000000..d26289a447 --- /dev/null +++ b/packages/svelte/tests/snapshot/samples/props-teardown-optimization/index.svelte @@ -0,0 +1,26 @@ + + +{doubled}