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 c61ccaa988..1f201e531b 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 @@ -23,8 +23,6 @@ export interface ClientTransformState extends TransformState { * us to rewrite `this.foo` as `this.#foo.value` */ readonly in_constructor: boolean; - readonly safe_props_ids?: Map; - readonly safe_props_name?: string; readonly transform: Record< string, @@ -47,6 +45,7 @@ export interface ComponentClientTransformState extends ClientTransformState { readonly hoisted: Array; readonly events: Set; readonly is_instance: boolean; + readonly needs_safe_props: boolean; readonly store_to_invalidate?: string; /** Stuff that happens before the render effect(s) */ diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js index 99eae046b0..ada1978f08 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js @@ -10,10 +10,6 @@ import { build_component } from './shared/component.js'; */ export function Component(node, context) { if (node.metadata.dynamic) { - let safe_props_ids = new Map(); - - const safe_props_name = context.state.scope.generate('$$safe_props'); - // Handle dynamic references to what seems like static inline components const component = build_component( node, @@ -22,8 +18,7 @@ export function Component(node, context) { ...context, state: { ...context.state, - safe_props_ids, - safe_props_name + needs_safe_props: true } }, b.id('$$anchor') @@ -36,19 +31,7 @@ export function Component(node, context) { // TODO use untrack here to not update when binding changes? // Would align with Svelte 4 behavior, but it's arguably nicer/expected to update this b.thunk(/** @type {Expression} */ (context.visit(b.member_id(node.name)))), - b.arrow( - [b.id('$$anchor'), b.id('$$component')], - b.block([ - b.const( - safe_props_name, - b.call( - '$.safe_props', - b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)]))) - ) - ), - component - ]) - ) + b.arrow([b.id('$$anchor'), b.id('$$component')], b.block([component])) ) ) ); 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 40144859a4..ae62909eff 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 @@ -9,9 +9,9 @@ import { build_getter } from '../utils.js'; * @param {Context} context */ export function Identifier(node, context) { - let parent = context.path.at(-1); + const parent = /** @type {Node} */ (context.path.at(-1)); - if (is_reference(node, /** @type {Node} */ (parent))) { + if (is_reference(node, parent)) { if (node.name === '$$props') { return b.id('$$sanitized_props'); } @@ -36,36 +36,6 @@ export function Identifier(node, context) { } } - const getter = build_getter(node, context.state); - - if ( - // this means we are inside an if or as an attribute of a dynamic component - // and we want to access `$$safe_props` to allow for the component to access them - // after destructuring - context.state.safe_props_name != null && - context.state.safe_props_ids != null && - // the parent can either be a component/svelte component in that case we - // check if this identifier is one of the attributes - (((parent?.type === 'Component' || parent?.type === 'SvelteComponent') && - parent.attributes.some( - (el) => - (el.type === 'Attribute' && - typeof el.value !== 'boolean' && - !Array.isArray(el.value) && - el.value.expression === node) || - (el.type === 'BindDirective' && el.expression === node) - )) || - // or a spread and we check the expression - (parent?.type === 'SpreadAttribute' && parent.expression === node)) && - // we also don't want to transform bindings that are defined withing the if block - // itself (for example an each local variable) - !binding?.references[0].path.some((node) => node.type === 'IfBlock') - ) { - // we store the getter in the safe props id and return an access to `$$safe_props.name` - context.state.safe_props_ids.set(node.name, getter); - return b.member(b.id(context.state.safe_props_name), b.id(node.name)); - } - - return getter; + return build_getter(node, context.state); } } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js index 2aec6e0156..78cc266443 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/IfBlock.js @@ -11,29 +11,13 @@ export function IfBlock(node, context) { context.state.template.push(''); const statements = []; - let safe_props_ids = new Map(); - - const safe_props_id = context.state.scope.generate('$$safe_props'); - const consequent = /** @type {BlockStatement} */ ( context.visit(node.consequent, { ...context.state, - safe_props_ids, - safe_props_name: safe_props_id + needs_safe_props: true }) ); - if (consequent.body.length > 0 && safe_props_ids) { - consequent.body.unshift( - b.const( - safe_props_id, - b.call( - '$.safe_props', - b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)]))) - ) - ) - ); - } const consequent_id = context.state.scope.generate('consequent'); statements.push(b.var(b.id(consequent_id), b.arrow([b.id('$$anchor')], consequent))); @@ -41,7 +25,12 @@ export function IfBlock(node, context) { let alternate_id; if (node.alternate) { - const alternate = /** @type {BlockStatement} */ (context.visit(node.alternate)); + const alternate = /** @type {BlockStatement} */ ( + context.visit(node.alternate, { + ...context.state, + needs_safe_props: true + }) + ); alternate_id = context.state.scope.generate('alternate'); statements.push(b.var(b.id(alternate_id), b.arrow([b.id('$$anchor')], alternate))); } diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js index 2bae4486dc..4801015c9e 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js @@ -92,6 +92,21 @@ export function build_component(node, component_name, context, anchor = context. } } + let safe_props_ids = new Map(); + let safe_props_name = context.state.scope.generate('$$safe_props'); + + /** + * @param {string} name + * @param {Expression} expression + */ + function safe_propify(name, expression) { + if (context.state.needs_safe_props) { + safe_props_ids.set(name, expression); + return b.member(b.id(safe_props_name), b.id(name)); + } + return expression; + } + for (const attribute of node.attributes) { if (attribute.type === 'LetDirective') { if (!slot_scope_applies_to_itself) { @@ -118,13 +133,14 @@ export function build_component(node, component_name, context, anchor = context. if (attribute.metadata.expression.has_state) { let value = expression; + const name = context.state.scope.generate('spread_element'); if (attribute.metadata.expression.has_call) { - const id = b.id(context.state.scope.generate('spread_element')); + const id = b.id(name); context.state.init.push(b.var(id, b.call('$.derived', b.thunk(value)))); value = b.call('$.get', id); } - props_and_spreads.push(b.thunk(value)); + props_and_spreads.push(b.thunk(safe_propify(name, value))); } else { props_and_spreads.push(expression); } @@ -172,7 +188,7 @@ export function build_component(node, component_name, context, anchor = context. ); if (has_state) { - push_prop(b.get(attribute.name, [b.return(value)])); + push_prop(b.get(attribute.name, [b.return(safe_propify(attribute.name, value))])); } else { push_prop(b.init(attribute.name, value)); } @@ -205,7 +221,9 @@ export function build_component(node, component_name, context, anchor = context. context.state.init.push(b.var(get_id, get)); context.state.init.push(b.var(set_id, set)); - push_prop(b.get(attribute.name, [b.return(b.call(get_id))])); + push_prop( + b.get(attribute.name, [b.return(safe_propify(attribute.name, b.call(get_id)))]) + ); push_prop(b.set(attribute.name, [b.stmt(b.call(set_id, b.id('$$value')))])); } } else { @@ -228,11 +246,17 @@ export function build_component(node, component_name, context, anchor = context. // Delay prop pushes so bindings come at the end, to avoid spreads overwriting them if (is_store_sub) { push_prop( - b.get(attribute.name, [b.stmt(b.call('$.mark_store_binding')), b.return(expression)]), + b.get(attribute.name, [ + b.stmt(b.call('$.mark_store_binding')), + b.return(safe_propify(attribute.name, expression)) + ]), true ); } else { - push_prop(b.get(attribute.name, [b.return(expression)]), true); + push_prop( + b.get(attribute.name, [b.return(safe_propify(attribute.name, expression))]), + true + ); } const assignment = b.assignment( @@ -403,6 +427,32 @@ export function build_component(node, component_name, context, anchor = context. const statements = [...snippet_declarations]; + if (safe_props_ids.size > 0) { + // if it is a dynamic component we need to include the safe props call inside the component + // function otherwise in the init (which in case of the if will be in the consequent/alternate function) + if (component_name === '$$component') { + statements.push( + b.const( + safe_props_name, + b.call( + '$.safe_props', + b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)]))) + ) + ) + ); + } else { + context.state.init.push( + b.const( + safe_props_name, + b.call( + '$.safe_props', + b.object([...safe_props_ids].map(([name, id]) => b.get(name, [b.return(id)]))) + ) + ) + ); + } + } + if (node.type === 'SvelteComponent') { const prev = fn; diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js index f75e9dd93c..abffad0ff7 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js @@ -185,7 +185,7 @@ export function build_attribute_value(value, context, memoize = (value) => value return { value: b.literal(chunk.data), has_state: false }; } - let expression = /** @type {Expression} */ (context.visit(chunk.expression, context.state)); + let expression = /** @type {Expression} */ (context.visit(chunk.expression)); return { value: memoize(expression, chunk.metadata.expression), diff --git a/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/Component.svelte b/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/Component.svelte new file mode 100644 index 0000000000..a1626ce276 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/Component.svelte @@ -0,0 +1,11 @@ + + +

{count}

+ + \ No newline at end of file diff --git a/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/_config.js b/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/_config.js new file mode 100644 index 0000000000..7d1e67e212 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/_config.js @@ -0,0 +1,68 @@ +import { ok, test } from '../../test'; +import { flushSync } from 'svelte'; + +export default test({ + async test({ assert, target, logs }) { + const [btn1, btn2, btn3] = target.querySelectorAll('button'); + let ps = [...target.querySelectorAll('p')]; + + for (const p of ps) { + assert.equal(p.innerHTML, '0'); + } + + flushSync(() => { + btn1.click(); + }); + + // prop update normally if we are not unmounting + for (const p of ps) { + assert.equal(p.innerHTML, '1'); + } + + flushSync(() => { + btn3.click(); + }); + + // binding still works and update the value correctly + for (const p of ps) { + assert.equal(p.innerHTML, '0'); + } + + flushSync(() => { + btn1.click(); + }); + + flushSync(() => { + btn1.click(); + }); + + console.warn(logs); + + // the five components guarded by `count < 2` unmount and log + assert.deepEqual(logs, [1, true, 1, true, 1, true, 1, true, 1, true]); + + flushSync(() => { + btn2.click(); + }); + + // the three components guarded by `show` unmount and log + assert.deepEqual(logs, [ + 1, + true, + 1, + true, + 1, + true, + 1, + true, + 1, + true, + 2, + true, + 2, + true, + 2, + true + ]); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/main.svelte b/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/main.svelte new file mode 100644 index 0000000000..b7d4a0ecc5 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/ondestroy-prop-access/main.svelte @@ -0,0 +1,45 @@ + + + + + + +{#if count < 2} + +{/if} + + +{#if count < 2} + +{/if} + + +{#if count < 2} + +{/if} + + +{#if show} + +{/if} + + + + + + + + + + + +