From 0488ae818d5dc7ada558c390a6fe1544d9876346 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 22 Jan 2025 09:46:31 -0500 Subject: [PATCH 1/5] fix: prevent double deriveds in component props --- .../client/visitors/RegularElement.js | 8 ++-- .../client/visitors/SlotElement.js | 6 ++- .../client/visitors/shared/component.js | 46 +++++++++---------- .../client/visitors/shared/element.js | 16 ++++--- .../client/visitors/shared/utils.js | 15 +++--- 5 files changed, 46 insertions(+), 45 deletions(-) diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js index 21a78de032c4..d0a148f216aa 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js @@ -537,8 +537,8 @@ function build_element_attribute_update_assignment( const is_svg = context.state.metadata.namespace === 'svg' || element.name === 'svg'; const is_mathml = context.state.metadata.namespace === 'mathml'; - let { value, has_state } = build_attribute_value(attribute.value, context, (value) => - get_expression_id(state, value) + let { value, has_state } = build_attribute_value(attribute.value, context, (value, metadata) => + metadata.has_call ? get_expression_id(state, value) : value ); if (name === 'autofocus') { @@ -665,8 +665,8 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co */ function build_element_special_value_attribute(element, node_id, attribute, context) { const state = context.state; - const { value, has_state } = build_attribute_value(attribute.value, context, (value) => - get_expression_id(state, value) + const { value, has_state } = build_attribute_value(attribute.value, context, (value, metadata) => + metadata.has_call ? get_expression_id(state, value) : value ); const inner_assignment = b.assignment( diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js index f1b08acbc695..fdd705e32e75 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js @@ -30,8 +30,10 @@ export function SlotElement(node, context) { if (attribute.type === 'SpreadAttribute') { spreads.push(b.thunk(/** @type {Expression} */ (context.visit(attribute)))); } else if (attribute.type === 'Attribute') { - const { value, has_state } = build_attribute_value(attribute.value, context, (value) => - memoize_expression(context.state, value) + const { value, has_state } = build_attribute_value( + attribute.value, + context, + (value, metadata) => (metadata.has_call ? memoize_expression(context.state, value) : value) ); if (attribute.name === 'name') { 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 9ac0bac12046..aaf9d11fbde3 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 @@ -134,9 +134,9 @@ export function build_component(node, component_name, context, anchor = context. custom_css_props.push( b.init( attribute.name, - build_attribute_value(attribute.value, context, (value) => + build_attribute_value(attribute.value, context, (value, metadata) => // TODO put the derived in the local block - memoize_expression(context.state, value) + metadata.has_call ? memoize_expression(context.state, value) : value ).value ) ); @@ -151,31 +151,29 @@ export function build_component(node, component_name, context, anchor = context. has_children_prop = true; } - const { value, has_state } = build_attribute_value(attribute.value, context, (value) => - memoize_expression(context.state, value) - ); - - if (has_state) { - let arg = value; - - // When we have a non-simple computation, anything other than an Identifier or Member expression, - // then there's a good chance it needs to be memoized to avoid over-firing when read within the - // child component. - const should_wrap_in_derived = get_attribute_chunks(attribute.value).some((n) => { - return ( - n.type === 'ExpressionTag' && - n.expression.type !== 'Identifier' && - n.expression.type !== 'MemberExpression' - ); - }); + const { value, has_state } = build_attribute_value( + attribute.value, + context, + (value, metadata) => { + if (!metadata.has_state) return value; + + // When we have a non-simple computation, anything other than an Identifier or Member expression, + // then there's a good chance it needs to be memoized to avoid over-firing when read within the + // child component (e.g. `active={i === index}`) + const should_wrap_in_derived = get_attribute_chunks(attribute.value).some((n) => { + return ( + n.type === 'ExpressionTag' && + n.expression.type !== 'Identifier' && + n.expression.type !== 'MemberExpression' + ); + }); - if (should_wrap_in_derived) { - const id = b.id(context.state.scope.generate(attribute.name)); - context.state.init.push(b.var(id, create_derived(context.state, b.thunk(value)))); - arg = b.call('$.get', id); + return should_wrap_in_derived ? memoize_expression(context.state, value) : value; } + ); - push_prop(b.get(attribute.name, [b.return(arg)])); + if (has_state) { + push_prop(b.get(attribute.name, [b.return(value)])); } else { push_prop(b.init(attribute.name, value)); } 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 8fb6b8bdde84..9f5cb757af73 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 @@ -1,5 +1,5 @@ /** @import { Expression, Identifier, ObjectExpression } from 'estree' */ -/** @import { AST } from '#compiler' */ +/** @import { AST, ExpressionMetadata } from '#compiler' */ /** @import { ComponentClientTransformState, ComponentContext } from '../../types' */ import { normalize_attribute } from '../../../../../../utils.js'; import { is_ignored } from '../../../../../state.js'; @@ -35,8 +35,10 @@ export function build_set_attributes( for (const attribute of attributes) { if (attribute.type === 'Attribute') { - const { value, has_state } = build_attribute_value(attribute.value, context, (value) => - get_expression_id(context.state, value) + const { value, has_state } = build_attribute_value( + attribute.value, + context, + (value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value) ); if ( @@ -111,8 +113,8 @@ export function build_style_directives( let value = directive.value === true ? build_getter({ name: directive.name, type: 'Identifier' }, context.state) - : build_attribute_value(directive.value, context, (value) => - get_expression_id(context.state, value) + : build_attribute_value(directive.value, context, (value, metadata) => + metadata.has_call ? get_expression_id(context.state, value) : value ).value; const update = b.stmt( @@ -169,7 +171,7 @@ export function build_class_directives( /** * @param {AST.Attribute['value']} value * @param {ComponentContext} context - * @param {(value: Expression) => Expression} memoize + * @param {(value: Expression, metadata: ExpressionMetadata) => Expression} memoize * @returns {{ value: Expression, has_state: boolean }} */ export function build_attribute_value(value, context, memoize = (value) => value) { @@ -187,7 +189,7 @@ export function build_attribute_value(value, context, memoize = (value) => value let expression = /** @type {Expression} */ (context.visit(chunk.expression)); return { - value: chunk.metadata.expression.has_call ? memoize(expression) : expression, + value: memoize(expression, chunk.metadata.expression), has_state: chunk.metadata.expression.has_state }; } 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 c4f81274d97e..4c286166650e 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 @@ -1,5 +1,5 @@ /** @import { Expression, ExpressionStatement, Identifier, MemberExpression, SequenceExpression, Statement, Super } from 'estree' */ -/** @import { AST } from '#compiler' */ +/** @import { AST, ExpressionMetadata } from '#compiler' */ /** @import { ComponentClientTransformState } from '../../types' */ import { walk } from 'zimmerframe'; import { object } from '../../../../../utils/ast.js'; @@ -79,14 +79,14 @@ function compare_expressions(a, b) { * @param {Array} values * @param {(node: AST.SvelteNode, state: any) => any} visit * @param {ComponentClientTransformState} state - * @param {(value: Expression) => Expression} memoize + * @param {(value: Expression, metadata: ExpressionMetadata) => Expression} memoize * @returns {{ value: Expression, has_state: boolean }} */ export function build_template_chunk( values, visit, state, - memoize = (value) => get_expression_id(state, value) + memoize = (value, metadata) => (metadata.has_call ? get_expression_id(state, value) : value) ) { /** @type {Expression[]} */ const expressions = []; @@ -106,14 +106,13 @@ export function build_template_chunk( quasi.value.cooked += node.expression.value + ''; } } else { - let value = /** @type {Expression} */ (visit(node.expression, state)); + let value = memoize( + /** @type {Expression} */ (visit(node.expression, state)), + node.metadata.expression + ); has_state ||= node.metadata.expression.has_state; - if (node.metadata.expression.has_call) { - value = memoize(value); - } - if (values.length === 1) { // If we have a single expression, then pass that in directly to possibly avoid doing // extra work in the template_effect (instead we do the work in set_text). From 3ec1267f6e086a0ed2735d543667311ca7835702 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 22 Jan 2025 09:46:53 -0500 Subject: [PATCH 2/5] unused --- .../phases/3-transform/client/visitors/shared/component.js | 1 - 1 file changed, 1 deletion(-) 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 aaf9d11fbde3..15e4f68e9e49 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 @@ -4,7 +4,6 @@ import { dev, is_ignored } from '../../../../../state.js'; import { get_attribute_chunks, object } from '../../../../../utils/ast.js'; import * as b from '../../../../../utils/builders.js'; -import { create_derived } from '../../utils.js'; import { build_bind_this, memoize_expression, validate_binding } from '../shared/utils.js'; import { build_attribute_value } from '../shared/element.js'; import { build_event_handler } from './events.js'; From 984e1021fab60f625b5d42cc5569d2c12d514c14 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 22 Jan 2025 09:54:05 -0500 Subject: [PATCH 3/5] reuse mechanism --- .../phases/3-transform/client/visitors/shared/element.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 9f5cb757af73..abffad0ff7a4 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 @@ -5,7 +5,7 @@ import { normalize_attribute } from '../../../../../../utils.js'; import { is_ignored } from '../../../../../state.js'; import { is_event_attribute } from '../../../../../utils/ast.js'; import * as b from '../../../../../utils/builders.js'; -import { build_getter, create_derived } from '../../utils.js'; +import { build_getter } from '../../utils.js'; import { build_template_chunk, get_expression_id } from './utils.js'; /** @@ -61,10 +61,9 @@ export function build_set_attributes( let value = /** @type {Expression} */ (context.visit(attribute)); if (attribute.metadata.expression.has_call) { - const id = b.id(state.scope.generate('spread_with_call')); - state.init.push(b.const(id, create_derived(state, b.thunk(value)))); - value = b.call('$.get', id); + value = get_expression_id(context.state, value); } + values.push(b.spread(value)); } } From b89124d1329afc1d93f88d6fca4e4055aa8615d2 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 22 Jan 2025 09:59:26 -0500 Subject: [PATCH 4/5] move code to where it is used --- .../phases/3-transform/client/utils.js | 35 ---------------- .../3-transform/client/visitors/AwaitBlock.js | 41 ++++++++++++++++++- 2 files changed, 39 insertions(+), 37 deletions(-) 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 c59a5544dfb2..421118cf680b 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/utils.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/utils.js @@ -269,41 +269,6 @@ export function should_proxy(node, scope) { return true; } -/** - * @param {Pattern} node - * @param {import('zimmerframe').Context} context - * @returns {{ id: Pattern, declarations: null | Statement[] }} - */ -export function create_derived_block_argument(node, context) { - if (node.type === 'Identifier') { - context.state.transform[node.name] = { read: get_value }; - return { id: node, declarations: null }; - } - - const pattern = /** @type {Pattern} */ (context.visit(node)); - const identifiers = extract_identifiers(node); - - const id = b.id('$$source'); - const value = b.id('$$value'); - - const block = b.block([ - b.var(pattern, b.call('$.get', id)), - b.return(b.object(identifiers.map((identifier) => b.prop('init', identifier, identifier)))) - ]); - - const declarations = [b.var(value, create_derived(context.state, b.thunk(block)))]; - - for (const id of identifiers) { - context.state.transform[id.name] = { read: get_value }; - - declarations.push( - b.var(id, create_derived(context.state, b.thunk(b.member(b.call('$.get', value), id)))) - ); - } - - return { id, declarations }; -} - /** * Svelte legacy mode should use safe equals in most places, runes mode shouldn't * @param {ComponentClientTransformState} state diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js index 146f75d40585..e0aef2d316a7 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js @@ -1,8 +1,10 @@ /** @import { BlockStatement, Expression, Pattern, Statement } from 'estree' */ /** @import { AST } from '#compiler' */ -/** @import { ComponentContext } from '../types' */ +/** @import { ComponentClientTransformState, ComponentContext } from '../types' */ +import { extract_identifiers } from '../../../../utils/ast.js'; import * as b from '../../../../utils/builders.js'; -import { create_derived_block_argument } from '../utils.js'; +import { create_derived } from '../utils.js'; +import { get_value } from './shared/declarations.js'; /** * @param {AST.AwaitBlock} node @@ -65,3 +67,38 @@ export function AwaitBlock(node, context) { ) ); } + +/** + * @param {Pattern} node + * @param {import('zimmerframe').Context} context + * @returns {{ id: Pattern, declarations: null | Statement[] }} + */ +function create_derived_block_argument(node, context) { + if (node.type === 'Identifier') { + context.state.transform[node.name] = { read: get_value }; + return { id: node, declarations: null }; + } + + const pattern = /** @type {Pattern} */ (context.visit(node)); + const identifiers = extract_identifiers(node); + + const id = b.id('$$source'); + const value = b.id('$$value'); + + const block = b.block([ + b.var(pattern, b.call('$.get', id)), + b.return(b.object(identifiers.map((identifier) => b.prop('init', identifier, identifier)))) + ]); + + const declarations = [b.var(value, create_derived(context.state, b.thunk(block)))]; + + for (const id of identifiers) { + context.state.transform[id.name] = { read: get_value }; + + declarations.push( + b.var(id, create_derived(context.state, b.thunk(b.member(b.call('$.get', value), id)))) + ); + } + + return { id, declarations }; +} From 1c38b6816f2b0762206407c7761f243c67119a21 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Wed, 22 Jan 2025 10:01:01 -0500 Subject: [PATCH 5/5] changeset --- .changeset/tasty-pigs-greet.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/tasty-pigs-greet.md diff --git a/.changeset/tasty-pigs-greet.md b/.changeset/tasty-pigs-greet.md new file mode 100644 index 000000000000..63decceed8de --- /dev/null +++ b/.changeset/tasty-pigs-greet.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: avoid double deriveds in component props