Skip to content

Commit 570884e

Browse files
dummdidummdummdidumm
authored
fix: spread attributes reactivity improvements (#10071)
- the objects could contain getters with reactive values, so we play it safe and assume they're always reactive - fixes #10065 - isolate spreads with call expression similar to how we do it with other effects -fixes #10013
1 parent 877ff1e commit 570884e

File tree

8 files changed

+206
-49
lines changed

8 files changed

+206
-49
lines changed

‎.changeset/sharp-kids-happen.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: always treat spread attributes as reactive and separate them if needed

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

Lines changed: 44 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -289,15 +289,15 @@ function setup_select_synchronization(value_binding, context) {
289289
* value = $.spread_attributes(element, value, [...])
290290
* });
291291
* ```
292-
* Returns the id of the spread_attribute variable if spread is deemed reactive, `null` otherwise.
292+
* Returns the id of the spread_attribute variable if spread isn't isolated, `null` otherwise.
293293
* @param {Array<import('#compiler').Attribute | import('#compiler').SpreadAttribute>} attributes
294294
* @param {import('../types.js').ComponentContext} context
295295
* @param {import('#compiler').RegularElement} element
296296
* @param {import('estree').Identifier} element_id
297297
* @returns {string | null}
298298
*/
299299
function serialize_element_spread_attributes(attributes, context, element, element_id) {
300-
let is_reactive = false;
300+
let needs_isolation = false;
301301

302302
/** @type {import('estree').Expression[]} */
303303
const values = [];
@@ -312,18 +312,32 @@ function serialize_element_spread_attributes(attributes, context, element, eleme
312312
values.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
313313
}
314314

315-
is_reactive ||=
316-
attribute.metadata.dynamic ||
317-
(attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression);
315+
needs_isolation ||=
316+
attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression;
318317
}
319318

320319
const lowercase_attributes =
321320
element.metadata.svg || is_custom_element_node(element) ? b.false : b.true;
322321

323-
if (is_reactive) {
322+
const isolated = b.stmt(
323+
b.call(
324+
'$.spread_attributes_effect',
325+
element_id,
326+
b.thunk(b.array(values)),
327+
lowercase_attributes,
328+
b.literal(context.state.analysis.stylesheet.id)
329+
)
330+
);
331+
332+
// objects could contain reactive getters -> play it safe and always assume spread attributes are reactive
333+
if (needs_isolation) {
334+
context.state.update_effects.push(isolated);
335+
return null;
336+
} else {
324337
const id = context.state.scope.generate('spread_attributes');
325-
context.state.init.push(b.let(id, undefined));
338+
context.state.init.push(b.let(id));
326339
context.state.update.push({
340+
singular: isolated,
327341
grouped: b.stmt(
328342
b.assignment(
329343
'=',
@@ -340,20 +354,6 @@ function serialize_element_spread_attributes(attributes, context, element, eleme
340354
)
341355
});
342356
return id;
343-
} else {
344-
context.state.init.push(
345-
b.stmt(
346-
b.call(
347-
'$.spread_attributes',
348-
element_id,
349-
b.literal(null),
350-
b.array(values),
351-
lowercase_attributes,
352-
b.literal(context.state.analysis.stylesheet.id)
353-
)
354-
)
355-
);
356-
return null;
357357
}
358358
}
359359

@@ -365,7 +365,7 @@ function serialize_element_spread_attributes(attributes, context, element, eleme
365365
* @param {import('estree').Identifier} element_id
366366
* @returns {boolean}
367367
*/
368-
function serialize_dynamic_element_spread_attributes(attributes, context, element_id) {
368+
function serialize_dynamic_element_attributes(attributes, context, element_id) {
369369
if (attributes.length === 0) {
370370
if (context.state.analysis.stylesheet.id) {
371371
context.state.init.push(
@@ -375,6 +375,7 @@ function serialize_dynamic_element_spread_attributes(attributes, context, elemen
375375
return false;
376376
}
377377

378+
let needs_isolation = false;
378379
let is_reactive = false;
379380

380381
/** @type {import('estree').Expression[]} */
@@ -388,13 +389,31 @@ function serialize_dynamic_element_spread_attributes(attributes, context, elemen
388389
values.push(/** @type {import('estree').Expression} */ (context.visit(attribute)));
389390
}
390391

391-
is_reactive ||= attribute.metadata.dynamic;
392+
is_reactive ||=
393+
attribute.metadata.dynamic ||
394+
// objects could contain reactive getters -> play it safe and always assume spread attributes are reactive
395+
attribute.type === 'SpreadAttribute';
396+
needs_isolation ||=
397+
attribute.type === 'SpreadAttribute' && attribute.metadata.contains_call_expression;
392398
}
393399

394-
if (is_reactive) {
400+
const isolated = b.stmt(
401+
b.call(
402+
'$.spread_dynamic_element_attributes_effect',
403+
element_id,
404+
b.thunk(b.array(values)),
405+
b.literal(context.state.analysis.stylesheet.id)
406+
)
407+
);
408+
409+
if (needs_isolation) {
410+
context.state.update_effects.push(isolated);
411+
return false;
412+
} else if (is_reactive) {
395413
const id = context.state.scope.generate('spread_attributes');
396414
context.state.init.push(b.let(id));
397415
context.state.update.push({
416+
singular: isolated,
398417
grouped: b.stmt(
399418
b.assignment(
400419
'=',
@@ -2101,7 +2120,7 @@ export const template_visitors = {
21012120
// Always use spread because we don't know whether the element is a custom element or not,
21022121
// therefore we need to do the "how to set an attribute" logic at runtime.
21032122
const is_attributes_reactive =
2104-
serialize_dynamic_element_spread_attributes(attributes, inner_context, element_id) !== null;
2123+
serialize_dynamic_element_attributes(attributes, inner_context, element_id) !== null;
21052124

21062125
// class/style directives must be applied last since they could override class/style attributes
21072126
serialize_class_directives(class_directives, element_id, inner_context, is_attributes_reactive);

‎packages/svelte/src/internal/client/render.js‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2428,10 +2428,26 @@ function get_setters(element) {
24282428
return setters;
24292429
}
24302430

2431+
/**
2432+
* Like `spread_attributes` but self-contained
2433+
* @param {Element & ElementCSSInlineStyle} dom
2434+
* @param {() => Record<string, unknown>[]} attrs
2435+
* @param {boolean} lowercase_attributes
2436+
* @param {string} css_hash
2437+
*/
2438+
export function spread_attributes_effect(dom, attrs, lowercase_attributes, css_hash) {
2439+
/** @type {Record<string, any> | undefined} */
2440+
let current = undefined;
2441+
2442+
render_effect(() => {
2443+
current = spread_attributes(dom, current, attrs(), lowercase_attributes, css_hash);
2444+
});
2445+
}
2446+
24312447
/**
24322448
* Spreads attributes onto a DOM element, taking into account the currently set attributes
24332449
* @param {Element & ElementCSSInlineStyle} dom
2434-
* @param {Record<string, unknown> | null} prev
2450+
* @param {Record<string, unknown> | undefined} prev
24352451
* @param {Record<string, unknown>[]} attrs
24362452
* @param {boolean} lowercase_attributes
24372453
* @param {string} css_hash
@@ -2528,18 +2544,30 @@ export function spread_attributes(dom, prev, attrs, lowercase_attributes, css_ha
25282544

25292545
/**
25302546
* @param {Element} node
2531-
* @param {Record<string, unknown> | null} prev
2547+
* @param {() => Record<string, unknown>[]} attrs
2548+
* @param {string} css_hash
2549+
*/
2550+
export function spread_dynamic_element_attributes_effect(node, attrs, css_hash) {
2551+
/** @type {Record<string, any> | undefined} */
2552+
let current = undefined;
2553+
2554+
render_effect(() => {
2555+
current = spread_dynamic_element_attributes(node, current, attrs(), css_hash);
2556+
});
2557+
}
2558+
2559+
/**
2560+
* @param {Element} node
2561+
* @param {Record<string, unknown> | undefined} prev
25322562
* @param {Record<string, unknown>[]} attrs
25332563
* @param {string} css_hash
25342564
*/
25352565
export function spread_dynamic_element_attributes(node, prev, attrs, css_hash) {
25362566
if (node.tagName.includes('-')) {
25372567
const next = object_assign({}, ...attrs);
2538-
if (prev !== null) {
2539-
for (const key in prev) {
2540-
if (!(key in next)) {
2541-
next[key] = null;
2542-
}
2568+
for (const key in prev) {
2569+
if (!(key in next)) {
2570+
next[key] = null;
25432571
}
25442572
}
25452573
for (const key in next) {

‎packages/svelte/tests/runtime-runes/samples/attribute-spread-call-expression/_config.js‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
import { test } from '../../test';
2+
3+
export default test({
4+
html: `
5+
<button class="red">red</button>
6+
<button class="red">red</button>
7+
<button class="red">red</button>
8+
<button class="red">red</button>
9+
`,
10+
11+
async test({ assert, target }) {
12+
const [b1, b2, b3, b4] = target.querySelectorAll('button');
13+
14+
b1?.click();
15+
await Promise.resolve();
16+
assert.htmlEqual(
17+
target.innerHTML,
18+
`
19+
<button class="blue">blue</button>
20+
<button class="red">red</button>
21+
<button class="red">red</button>
22+
<button class="red">red</button>
23+
`
24+
);
25+
26+
b2?.click();
27+
await Promise.resolve();
28+
assert.htmlEqual(
29+
target.innerHTML,
30+
`
31+
<button class="blue">blue</button>
32+
<button class="blue">blue</button>
33+
<button class="red">red</button>
34+
<button class="red">red</button>
35+
`
36+
);
37+
38+
b3?.click();
39+
await Promise.resolve();
40+
assert.htmlEqual(
41+
target.innerHTML,
42+
`
43+
<button class="blue">blue</button>
44+
<button class="blue">blue</button>
45+
<button class="blue">blue</button>
46+
<button class="red">red</button>
47+
`
48+
);
49+
50+
b4?.click();
51+
await Promise.resolve();
52+
assert.htmlEqual(
53+
target.innerHTML,
54+
`
55+
<button class="blue">blue</button>
56+
<button class="blue">blue</button>
57+
<button class="blue">blue</button>
58+
<button class="blue">blue</button>
59+
`
60+
);
61+
}
62+
});

‎packages/svelte/tests/runtime-runes/samples/attribute-spread-call-expression/main.svelte‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<script>
2+
let tag = $state('button');
3+
let values = $state({ a: 'red', b: 'red', c: 'red', d: 'red' });
4+
5+
let count = 0;
6+
const factory = (name) => {
7+
count++;
8+
// check that spread effects are isolated from each other
9+
if (count > 8) throw new Error('too many calls');
10+
11+
return {
12+
class: values[name],
13+
onclick: () => {
14+
values[name] = 'blue';
15+
}
16+
}
17+
}
18+
</script>
19+
20+
<button {...factory('a')}>{values.a}</button>
21+
<button {...factory('b')}>{values.b}</button>
22+
23+
<svelte:element this={tag} {...factory('c')}>{values.c}</svelte:element>
24+
<svelte:element this={tag} {...factory('d')}>{values.d}</svelte:element>

‎packages/svelte/tests/runtime-runes/samples/attribute-spread-reactivitiy/_config.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { test } from '../../test';
2+
3+
export default test({
4+
html: `
5+
<div style="color: red;"></div><div class="red"></div><div class="red"></div>
6+
<div style="color: red;"></div><div class="red"></div><div class="red"></div>
7+
<button>toggle</button
8+
`,
9+
10+
async test({ assert, target }) {
11+
const [b1] = target.querySelectorAll('button');
12+
13+
b1?.click();
14+
await Promise.resolve();
15+
assert.htmlEqual(
16+
target.innerHTML,
17+
`
18+
<div class="blue" style="color: blue;"></div><div class="blue"></div><div class="blue"></div>
19+
<div class="blue" style="color: blue;"></div><div class="blue"></div><div class="blue"></div>
20+
<button>toggle</button
21+
`
22+
);
23+
}
24+
});

packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/main.svelte renamed to packages/svelte/tests/runtime-runes/samples/attribute-spread-reactivitiy/main.svelte

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
<script>
22
let value = $state('red');
3+
let tag = $state('div');
34
45
const getValue = () => {
56
return value;
@@ -10,9 +11,19 @@
1011
const getSpread = () => {
1112
return { class: value };
1213
}
14+
const props = {
15+
get class() {
16+
return value;
17+
}
18+
}
1319
</script>
1420

1521
<div class:blue={getClass()} style:color={getValue()}></div>
1622
<div {...getSpread()}></div>
17-
<button on:click={() => value = 'blue'}>toggle</button>
23+
<div {...props}></div>
1824

25+
<svelte:element this={tag} class:blue={getClass()} style:color={getValue()}></svelte:element>
26+
<svelte:element this={tag} {...getSpread()}></svelte:element>
27+
<svelte:element this={tag} {...props}></svelte:element>
28+
29+
<button on:click={() => value = 'blue'}>toggle</button>

‎packages/svelte/tests/runtime-runes/samples/event-attribute-call-expressions/_config.js‎

Lines changed: 0 additions & 16 deletions
This file was deleted.

0 commit comments

Comments
 (0)