Skip to content

Commit 82f3be2

Browse files
dummdidummdummdidumm
committed
fix: untrack reads of mutated object
prevents infinite loops when mutation happens inside an effect came up in #9639
1 parent a31b2e1 commit 82f3be2

File tree

10 files changed

+180
-51
lines changed

10 files changed

+180
-51
lines changed

‎.changeset/beige-donkeys-remain.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: untrack reads of mutated object

‎packages/svelte/src/compiler/phases/2-analyze/validation.js

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { error } from '../../errors.js';
2-
import { extract_identifiers, is_text_attribute } from '../../utils/ast.js';
2+
import { extract_identifiers, is_text_attribute, object } from '../../utils/ast.js';
33
import { warn } from '../../warnings.js';
44
import fuzzymatch from '../1-parse/utils/fuzzymatch.js';
55
import { binding_properties } from '../bindings.js';
@@ -248,12 +248,8 @@ export const validation = {
248248
BindDirective(node, context) {
249249
validate_no_const_assignment(node, node.expression, context.state.scope, true);
250250

251-
let left = node.expression;
252-
while (left.type === 'MemberExpression') {
253-
left = /** @type {import('estree').MemberExpression} */ (left.object);
254-
}
255-
256-
if (left.type !== 'Identifier') {
251+
const left = object(node.expression);
252+
if (left === null) {
257253
error(node, 'invalid-binding-expression');
258254
}
259255

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

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -205,27 +205,31 @@ export function serialize_set_binding(node, context, fallback) {
205205
return b.call('$.set', b.id(left_name), value);
206206
}
207207
} else {
208+
const left = /** @type {import('estree').MemberExpression} */ (visit(node.left));
209+
// When reading the object that's mutated we need to untrack that read in order
210+
// to avoid infinite loops. $.mutate(_store) does that by accepting a callback
211+
// function it hands the value to. For this, we need to adjust the code to
212+
// reference that passed value instead of reading the object again.
213+
let id = left;
214+
while (id.type === 'MemberExpression') {
215+
if (id.object.type !== 'MemberExpression') {
216+
id.object = b.id('$$to_mutate');
217+
break;
218+
} else {
219+
id = id.object;
220+
}
221+
}
222+
const update = b.arrow([b.id('$$to_mutate')], b.assignment(node.operator, left, value));
223+
208224
if (is_store) {
209225
return b.call(
210226
'$.mutate_store',
211227
serialize_get_binding(b.id(left_name), state),
212-
b.assignment(
213-
node.operator,
214-
/** @type {import('estree').Pattern} */ (visit(node.left)),
215-
value
216-
),
217-
b.call('$' + left_name)
228+
b.id('$' + left_name),
229+
update
218230
);
219231
} else {
220-
return b.call(
221-
'$.mutate',
222-
b.id(left_name),
223-
b.assignment(
224-
node.operator,
225-
/** @type {import('estree').Pattern} */ (visit(node.left)),
226-
value
227-
)
228-
);
232+
return b.call('$.mutate', b.id(left_name), update);
229233
}
230234
}
231235
};

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

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -206,13 +206,7 @@ function collect_transitive_dependencies(binding, seen = new Set()) {
206206
* @param {import('../types.js').ComponentContext} context
207207
*/
208208
function setup_select_synchronization(value_binding, context) {
209-
let bound = value_binding.expression;
210-
while (bound.type === 'MemberExpression') {
211-
bound = /** @type {import('estree').Identifier | import('estree').MemberExpression} */ (
212-
bound.object
213-
);
214-
}
215-
209+
const bound = /** @type {import('estree').Identifier} */ (object(value_binding.expression));
216210
/** @type {string[]} */
217211
const names = [];
218212

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

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
import { walk } from 'zimmerframe';
22
import { set_scope, get_rune } from '../../scope.js';
3-
import { extract_identifiers, extract_paths, is_event_attribute } from '../../../utils/ast.js';
3+
import {
4+
extract_identifiers,
5+
extract_paths,
6+
is_event_attribute,
7+
object
8+
} from '../../../utils/ast.js';
49
import * as b from '../../../utils/builders.js';
510
import is_reference from 'is-reference';
611
import {
@@ -417,14 +422,8 @@ function serialize_set_binding(node, context, fallback) {
417422
error(node, 'INTERNAL', `Unexpected assignment type ${node.left.type}`);
418423
}
419424

420-
let left = node.left;
421-
422-
while (left.type === 'MemberExpression') {
423-
// @ts-expect-error
424-
left = left.object;
425-
}
426-
427-
if (left.type !== 'Identifier') {
425+
const left = object(node.left);
426+
if (left === null) {
428427
return fallback();
429428
}
430429

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

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -862,34 +862,35 @@ export function invalidate_inner_signals(fn) {
862862
}
863863
let signal;
864864
for (signal of captured_signals) {
865-
mutate(signal, null /* doesnt matter */);
865+
mutate(signal, () => {} /* doesnt matter */);
866866
}
867867
return captured_signals;
868868
}
869869

870870
/**
871871
* @template V
872872
* @param {import('./types.js').Signal<V>} source
873-
* @param {V} value
873+
* @param {(v: V) => any} update
874874
*/
875-
export function mutate(source, value) {
876-
set_signal_value(
877-
source,
878-
untrack(() => get(source))
879-
);
880-
return value;
875+
export function mutate(source, update) {
876+
var value = untrack(() => get(source));
877+
var updated = update(value);
878+
set_signal_value(source, value);
879+
return updated;
881880
}
882881

883882
/**
884883
* Updates a store with a new value.
885884
* @param {import('./types.js').Store<V>} store the store to update
886-
* @param {any} expression the expression that mutates the store
887-
* @param {V} new_value the new store value
885+
* @param {() => V} get_value function to retrieve the current store value
886+
* @param {(v: V) => any} update the expression that mutates the store
888887
* @template V
889888
*/
890-
export function mutate_store(store, expression, new_value) {
891-
store.set(new_value);
892-
return expression;
889+
export function mutate_store(store, get_value, update) {
890+
var updated = update(untrack(get_value));
891+
// mutation could result in a new store being tracked, therefore call get_value again
892+
store.set(untrack(get_value));
893+
return updated;
893894
}
894895

895896
/**

‎packages/svelte/tests/runtime-runes/samples/mutating-state/_config.js

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import { flushSync } from 'svelte';
2+
import { test } from '../../test';
3+
4+
// Test ensures that reading the state that's mutated is done in a manner
5+
// so that the read is untracked so it doesn't trigger infinite loops
6+
export default test({
7+
html: `
8+
<input>
9+
<input>
10+
<p>text: foo</p>
11+
<p>wrapped.contents: foo</p>
12+
`,
13+
skip_if_ssr: true,
14+
15+
test({ assert, target }) {
16+
const [input1, input2] = target.querySelectorAll('input');
17+
18+
input1.value = 'bar';
19+
flushSync(() => input1.dispatchEvent(new Event('input')));
20+
assert.htmlEqual(
21+
target.innerHTML,
22+
`
23+
<input>
24+
<input>
25+
<p>text: bar</p>
26+
<p>wrapped.contents: bar</p>
27+
`
28+
);
29+
30+
input2.value = 'baz';
31+
flushSync(() => input2.dispatchEvent(new Event('input')));
32+
assert.htmlEqual(
33+
target.innerHTML,
34+
`
35+
<input>
36+
<input>
37+
<p>text: bar</p>
38+
<p>wrapped.contents: baz</p>
39+
`
40+
);
41+
42+
input1.value = 'foo';
43+
flushSync(() => input1.dispatchEvent(new Event('input')));
44+
assert.htmlEqual(
45+
target.innerHTML,
46+
`
47+
<input>
48+
<input>
49+
<p>text: foo</p>
50+
<p>wrapped.contents: foo</p>
51+
`
52+
);
53+
}
54+
});

‎packages/svelte/tests/runtime-runes/samples/mutating-state/main.svelte

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<script>
2+
let text = $state('foo');
3+
let wrapped = $state({});
4+
$effect(() => { wrapped.contents = text; })
5+
</script>
6+
7+
<input bind:value={text} />
8+
<input bind:value={wrapped.contents} />
9+
<p>text: {text}</p>
10+
<p>wrapped.contents: {wrapped.contents}</p>

‎packages/svelte/tests/runtime-runes/samples/mutating-store-state/_config.js

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import { flushSync } from 'svelte';
2+
import { test } from '../../test';
3+
4+
// Test ensures that reading the state that's mutated is done in a manner
5+
// so that the read is untracked so it doesn't trigger infinite loops
6+
export default test({
7+
html: `
8+
<input>
9+
<input>
10+
<p>text: foo</p>
11+
<p>wrapped.contents: foo</p>
12+
`,
13+
skip_if_ssr: true,
14+
15+
test({ assert, target }) {
16+
const [input1, input2] = target.querySelectorAll('input');
17+
18+
input1.value = 'bar';
19+
flushSync(() => input1.dispatchEvent(new Event('input')));
20+
assert.htmlEqual(
21+
target.innerHTML,
22+
`
23+
<input>
24+
<input>
25+
<p>text: bar</p>
26+
<p>wrapped.contents: bar</p>
27+
`
28+
);
29+
30+
input2.value = 'baz';
31+
flushSync(() => input2.dispatchEvent(new Event('input')));
32+
assert.htmlEqual(
33+
target.innerHTML,
34+
`
35+
<input>
36+
<input>
37+
<p>text: bar</p>
38+
<p>wrapped.contents: baz</p>
39+
`
40+
);
41+
42+
input1.value = 'foo';
43+
flushSync(() => input1.dispatchEvent(new Event('input')));
44+
assert.htmlEqual(
45+
target.innerHTML,
46+
`
47+
<input>
48+
<input>
49+
<p>text: foo</p>
50+
<p>wrapped.contents: foo</p>
51+
`
52+
);
53+
}
54+
});

‎packages/svelte/tests/runtime-runes/samples/mutating-store-state/main.svelte

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
<script>
2+
import { writable } from "svelte/store";
3+
4+
let text = writable('foo');
5+
let wrapped = writable({});
6+
$effect(() => { $wrapped.contents = $text; })
7+
</script>
8+
9+
<input bind:value={$text} />
10+
<input bind:value={$wrapped.contents} />
11+
<p>text: {$text}</p>
12+
<p>wrapped.contents: {$wrapped.contents}</p>

0 commit comments

Comments
 (0)