Skip to content

Commit 8bba70b

Browse files
Rich-HarrisRich-Harris
authored
fix: avoid double deriveds in component props (#15089)
* fix: prevent double deriveds in component props * unused * reuse mechanism * move code to where it is used * changeset
1 parent eee808f commit 8bba70b

File tree

8 files changed

+93
-87
lines changed

8 files changed

+93
-87
lines changed

‎.changeset/tasty-pigs-greet.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
fix: avoid double deriveds in component props

‎packages/svelte/src/compiler/phases/3-transform/client/utils.js

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -269,41 +269,6 @@ export function should_proxy(node, scope) {
269269
return true;
270270
}
271271

272-
/**
273-
* @param {Pattern} node
274-
* @param {import('zimmerframe').Context<AST.SvelteNode, ComponentClientTransformState>} context
275-
* @returns {{ id: Pattern, declarations: null | Statement[] }}
276-
*/
277-
export function create_derived_block_argument(node, context) {
278-
if (node.type === 'Identifier') {
279-
context.state.transform[node.name] = { read: get_value };
280-
return { id: node, declarations: null };
281-
}
282-
283-
const pattern = /** @type {Pattern} */ (context.visit(node));
284-
const identifiers = extract_identifiers(node);
285-
286-
const id = b.id('$$source');
287-
const value = b.id('$$value');
288-
289-
const block = b.block([
290-
b.var(pattern, b.call('$.get', id)),
291-
b.return(b.object(identifiers.map((identifier) => b.prop('init', identifier, identifier))))
292-
]);
293-
294-
const declarations = [b.var(value, create_derived(context.state, b.thunk(block)))];
295-
296-
for (const id of identifiers) {
297-
context.state.transform[id.name] = { read: get_value };
298-
299-
declarations.push(
300-
b.var(id, create_derived(context.state, b.thunk(b.member(b.call('$.get', value), id))))
301-
);
302-
}
303-
304-
return { id, declarations };
305-
}
306-
307272
/**
308273
* Svelte legacy mode should use safe equals in most places, runes mode shouldn't
309274
* @param {ComponentClientTransformState} state

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
/** @import { BlockStatement, Expression, Pattern, Statement } from 'estree' */
22
/** @import { AST } from '#compiler' */
3-
/** @import { ComponentContext } from '../types' */
3+
/** @import { ComponentClientTransformState, ComponentContext } from '../types' */
4+
import { extract_identifiers } from '../../../../utils/ast.js';
45
import * as b from '../../../../utils/builders.js';
5-
import { create_derived_block_argument } from '../utils.js';
6+
import { create_derived } from '../utils.js';
7+
import { get_value } from './shared/declarations.js';
68

79
/**
810
* @param {AST.AwaitBlock} node
@@ -65,3 +67,38 @@ export function AwaitBlock(node, context) {
6567
)
6668
);
6769
}
70+
71+
/**
72+
* @param {Pattern} node
73+
* @param {import('zimmerframe').Context<AST.SvelteNode, ComponentClientTransformState>} context
74+
* @returns {{ id: Pattern, declarations: null | Statement[] }}
75+
*/
76+
function create_derived_block_argument(node, context) {
77+
if (node.type === 'Identifier') {
78+
context.state.transform[node.name] = { read: get_value };
79+
return { id: node, declarations: null };
80+
}
81+
82+
const pattern = /** @type {Pattern} */ (context.visit(node));
83+
const identifiers = extract_identifiers(node);
84+
85+
const id = b.id('$$source');
86+
const value = b.id('$$value');
87+
88+
const block = b.block([
89+
b.var(pattern, b.call('$.get', id)),
90+
b.return(b.object(identifiers.map((identifier) => b.prop('init', identifier, identifier))))
91+
]);
92+
93+
const declarations = [b.var(value, create_derived(context.state, b.thunk(block)))];
94+
95+
for (const id of identifiers) {
96+
context.state.transform[id.name] = { read: get_value };
97+
98+
declarations.push(
99+
b.var(id, create_derived(context.state, b.thunk(b.member(b.call('$.get', value), id))))
100+
);
101+
}
102+
103+
return { id, declarations };
104+
}

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -537,8 +537,8 @@ function build_element_attribute_update_assignment(
537537
const is_svg = context.state.metadata.namespace === 'svg' || element.name === 'svg';
538538
const is_mathml = context.state.metadata.namespace === 'mathml';
539539

540-
let { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
541-
get_expression_id(state, value)
540+
let { value, has_state } = build_attribute_value(attribute.value, context, (value, metadata) =>
541+
metadata.has_call ? get_expression_id(state, value) : value
542542
);
543543

544544
if (name === 'autofocus') {
@@ -665,8 +665,8 @@ function build_custom_element_attribute_update_assignment(node_id, attribute, co
665665
*/
666666
function build_element_special_value_attribute(element, node_id, attribute, context) {
667667
const state = context.state;
668-
const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
669-
get_expression_id(state, value)
668+
const { value, has_state } = build_attribute_value(attribute.value, context, (value, metadata) =>
669+
metadata.has_call ? get_expression_id(state, value) : value
670670
);
671671

672672
const inner_assignment = b.assignment(

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/SlotElement.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,10 @@ export function SlotElement(node, context) {
3030
if (attribute.type === 'SpreadAttribute') {
3131
spreads.push(b.thunk(/** @type {Expression} */ (context.visit(attribute))));
3232
} else if (attribute.type === 'Attribute') {
33-
const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
34-
memoize_expression(context.state, value)
33+
const { value, has_state } = build_attribute_value(
34+
attribute.value,
35+
context,
36+
(value, metadata) => (metadata.has_call ? memoize_expression(context.state, value) : value)
3537
);
3638

3739
if (attribute.name === 'name') {

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/component.js

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import { dev, is_ignored } from '../../../../../state.js';
55
import { get_attribute_chunks, object } from '../../../../../utils/ast.js';
66
import * as b from '../../../../../utils/builders.js';
7-
import { create_derived } from '../../utils.js';
87
import { build_bind_this, memoize_expression, validate_binding } from '../shared/utils.js';
98
import { build_attribute_value } from '../shared/element.js';
109
import { build_event_handler } from './events.js';
@@ -134,9 +133,9 @@ export function build_component(node, component_name, context, anchor = context.
134133
custom_css_props.push(
135134
b.init(
136135
attribute.name,
137-
build_attribute_value(attribute.value, context, (value) =>
136+
build_attribute_value(attribute.value, context, (value, metadata) =>
138137
// TODO put the derived in the local block
139-
memoize_expression(context.state, value)
138+
metadata.has_call ? memoize_expression(context.state, value) : value
140139
).value
141140
)
142141
);
@@ -151,31 +150,29 @@ export function build_component(node, component_name, context, anchor = context.
151150
has_children_prop = true;
152151
}
153152

154-
const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
155-
memoize_expression(context.state, value)
156-
);
157-
158-
if (has_state) {
159-
let arg = value;
160-
161-
// When we have a non-simple computation, anything other than an Identifier or Member expression,
162-
// then there's a good chance it needs to be memoized to avoid over-firing when read within the
163-
// child component.
164-
const should_wrap_in_derived = get_attribute_chunks(attribute.value).some((n) => {
165-
return (
166-
n.type === 'ExpressionTag' &&
167-
n.expression.type !== 'Identifier' &&
168-
n.expression.type !== 'MemberExpression'
169-
);
170-
});
153+
const { value, has_state } = build_attribute_value(
154+
attribute.value,
155+
context,
156+
(value, metadata) => {
157+
if (!metadata.has_state) return value;
158+
159+
// When we have a non-simple computation, anything other than an Identifier or Member expression,
160+
// then there's a good chance it needs to be memoized to avoid over-firing when read within the
161+
// child component (e.g. `active={i === index}`)
162+
const should_wrap_in_derived = get_attribute_chunks(attribute.value).some((n) => {
163+
return (
164+
n.type === 'ExpressionTag' &&
165+
n.expression.type !== 'Identifier' &&
166+
n.expression.type !== 'MemberExpression'
167+
);
168+
});
171169

172-
if (should_wrap_in_derived) {
173-
const id = b.id(context.state.scope.generate(attribute.name));
174-
context.state.init.push(b.var(id, create_derived(context.state, b.thunk(value))));
175-
arg = b.call('$.get', id);
170+
return should_wrap_in_derived ? memoize_expression(context.state, value) : value;
176171
}
172+
);
177173

178-
push_prop(b.get(attribute.name, [b.return(arg)]));
174+
if (has_state) {
175+
push_prop(b.get(attribute.name, [b.return(value)]));
179176
} else {
180177
push_prop(b.init(attribute.name, value));
181178
}

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
/** @import { Expression, Identifier, ObjectExpression } from 'estree' */
2-
/** @import { AST } from '#compiler' */
2+
/** @import { AST, ExpressionMetadata } from '#compiler' */
33
/** @import { ComponentClientTransformState, ComponentContext } from '../../types' */
44
import { normalize_attribute } from '../../../../../../utils.js';
55
import { is_ignored } from '../../../../../state.js';
66
import { is_event_attribute } from '../../../../../utils/ast.js';
77
import * as b from '../../../../../utils/builders.js';
8-
import { build_getter, create_derived } from '../../utils.js';
8+
import { build_getter } from '../../utils.js';
99
import { build_template_chunk, get_expression_id } from './utils.js';
1010

1111
/**
@@ -35,8 +35,10 @@ export function build_set_attributes(
3535

3636
for (const attribute of attributes) {
3737
if (attribute.type === 'Attribute') {
38-
const { value, has_state } = build_attribute_value(attribute.value, context, (value) =>
39-
get_expression_id(context.state, value)
38+
const { value, has_state } = build_attribute_value(
39+
attribute.value,
40+
context,
41+
(value, metadata) => (metadata.has_call ? get_expression_id(context.state, value) : value)
4042
);
4143

4244
if (
@@ -59,10 +61,9 @@ export function build_set_attributes(
5961
let value = /** @type {Expression} */ (context.visit(attribute));
6062

6163
if (attribute.metadata.expression.has_call) {
62-
const id = b.id(state.scope.generate('spread_with_call'));
63-
state.init.push(b.const(id, create_derived(state, b.thunk(value))));
64-
value = b.call('$.get', id);
64+
value = get_expression_id(context.state, value);
6565
}
66+
6667
values.push(b.spread(value));
6768
}
6869
}
@@ -111,8 +112,8 @@ export function build_style_directives(
111112
let value =
112113
directive.value === true
113114
? build_getter({ name: directive.name, type: 'Identifier' }, context.state)
114-
: build_attribute_value(directive.value, context, (value) =>
115-
get_expression_id(context.state, value)
115+
: build_attribute_value(directive.value, context, (value, metadata) =>
116+
metadata.has_call ? get_expression_id(context.state, value) : value
116117
).value;
117118

118119
const update = b.stmt(
@@ -169,7 +170,7 @@ export function build_class_directives(
169170
/**
170171
* @param {AST.Attribute['value']} value
171172
* @param {ComponentContext} context
172-
* @param {(value: Expression) => Expression} memoize
173+
* @param {(value: Expression, metadata: ExpressionMetadata) => Expression} memoize
173174
* @returns {{ value: Expression, has_state: boolean }}
174175
*/
175176
export function build_attribute_value(value, context, memoize = (value) => value) {
@@ -187,7 +188,7 @@ export function build_attribute_value(value, context, memoize = (value) => value
187188
let expression = /** @type {Expression} */ (context.visit(chunk.expression));
188189

189190
return {
190-
value: chunk.metadata.expression.has_call ? memoize(expression) : expression,
191+
value: memoize(expression, chunk.metadata.expression),
191192
has_state: chunk.metadata.expression.has_state
192193
};
193194
}

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/utils.js

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/** @import { Expression, ExpressionStatement, Identifier, MemberExpression, SequenceExpression, Statement, Super } from 'estree' */
2-
/** @import { AST } from '#compiler' */
2+
/** @import { AST, ExpressionMetadata } from '#compiler' */
33
/** @import { ComponentClientTransformState } from '../../types' */
44
import { walk } from 'zimmerframe';
55
import { object } from '../../../../../utils/ast.js';
@@ -79,14 +79,14 @@ function compare_expressions(a, b) {
7979
* @param {Array<AST.Text | AST.ExpressionTag>} values
8080
* @param {(node: AST.SvelteNode, state: any) => any} visit
8181
* @param {ComponentClientTransformState} state
82-
* @param {(value: Expression) => Expression} memoize
82+
* @param {(value: Expression, metadata: ExpressionMetadata) => Expression} memoize
8383
* @returns {{ value: Expression, has_state: boolean }}
8484
*/
8585
export function build_template_chunk(
8686
values,
8787
visit,
8888
state,
89-
memoize = (value) => get_expression_id(state, value)
89+
memoize = (value, metadata) => (metadata.has_call ? get_expression_id(state, value) : value)
9090
) {
9191
/** @type {Expression[]} */
9292
const expressions = [];
@@ -106,14 +106,13 @@ export function build_template_chunk(
106106
quasi.value.cooked += node.expression.value + '';
107107
}
108108
} else {
109-
let value = /** @type {Expression} */ (visit(node.expression, state));
109+
let value = memoize(
110+
/** @type {Expression} */ (visit(node.expression, state)),
111+
node.metadata.expression
112+
);
110113

111114
has_state ||= node.metadata.expression.has_state;
112115

113-
if (node.metadata.expression.has_call) {
114-
value = memoize(value);
115-
}
116-
117116
if (values.length === 1) {
118117
// If we have a single expression, then pass that in directly to possibly avoid doing
119118
// extra work in the template_effect (instead we do the work in set_text).

0 commit comments

Comments
 (0)