Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: better interop of $state with actions/$: statements #10543

Merged
merged 5 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/light-days-clean.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: ensure update methods of actions and reactive statements work with fine-grained `$state`
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,10 @@ export const javascript_visitors_legacy = {
const name = binding.node.name;
let serialized = serialize_get_binding(b.id(name), state);

if (name === '$$props' || name === '$$restProps') {
serialized = b.call('$.access_props', serialized);
// If the binding is a prop, we need to deep read it because it could be fine-grained $state
// from a runes-component, where mutations don't trigger an update on the prop as a whole.
if (name === '$$props' || name === '$$restProps' || binding.kind === 'prop') {
serialized = b.call('$.deep_read', serialized);
}

sequence.push(serialized);
Expand Down
23 changes: 20 additions & 3 deletions packages/svelte/src/internal/client/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ import {
managed_effect,
push,
current_component_context,
pop
pop,
deep_read
} from './runtime.js';
import {
current_hydration_fragment,
Expand Down Expand Up @@ -2312,16 +2313,24 @@ export function action(dom, action, value_fn) {
effect(() => {
if (value_fn) {
const value = value_fn();
let needs_deep_read = false;
untrack(() => {
if (payload === undefined) {
payload = action(dom, value) || {};
} else {
const update = payload.update;
if (typeof update === 'function') {
needs_deep_read = true;
update(value);
}
}
});
// Action's update method is coarse-grained, i.e. when anything in the passed value changes, update.
// This works in legacy mode because of mutable_source being updated as a whole, but when using $state
// together with actions and mutation, it wouldn't notice the change without a deep read.
if (needs_deep_read) {
deep_read(value);
}
} else {
untrack(() => (payload = action(dom)));
}
Expand Down Expand Up @@ -3048,8 +3057,16 @@ export function unmount(component) {
*/
export function access_props(props) {
for (const prop in props) {
// eslint-disable-next-line no-unused-expressions
props[prop];
deep_read(props[prop]);
}
}

/**
* @param {any[]} values
*/
export function deep_read_all(...values) {
for (const value of values) {
deep_read(value);
}
}

Expand Down
4 changes: 3 additions & 1 deletion packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -1985,11 +1985,13 @@ export function init() {
}

/**
* Deeply traverse an object and read all its properties
* so that they're all reactive in case this is `$state`
* @param {any} value
* @param {Set<any>} visited
* @returns {void}
*/
function deep_read(value, visited = new Set()) {
export function deep_read(value, visited = new Set()) {
if (typeof value === 'object' && value !== null && !visited.has(value)) {
visited.add(value);
for (let key in value) {
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/src/internal/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ export {
inspect,
unwrap,
freeze,
init
init,
deep_read
} from './client/runtime.js';
export * from './client/each.js';
export * from './client/render.js';
Expand Down
3 changes: 3 additions & 0 deletions packages/svelte/src/legacy/legacy-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ class Svelte4Component {
* }} options
*/
constructor(options) {
// Using proxy state here isn't completely mirroring the Svelte 4 behavior, because mutations to a property
// cause fine-grained updates to only the places where that property is used, and not the entire property.
// Reactive statements and actions (the things where this matters) are handling this properly regardless, so it should be fine in practise.
const props = $.proxy({ ...(options.props || {}), $$events: this.#events }, false);
this.#instance = (options.hydrate ? $.hydrate : $.mount)(options.component, {
target: options.target,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ export default test({

p = parents['reactive_mutate'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 1);
assert.equal(p.updates.length, 2);

p = parents['init_update'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 2);

p = parents['init_mutate'];
assert.deepEqual(p.value, { foo: 'kid' });
assert.equal(p.updates.length, 1);
assert.equal(p.updates.length, 2);
Copy link
Member Author

@dummdidumm dummdidumm Feb 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is from #8992 and was (turns out, wrongfully) changed when moving to proxies in #9826 . This uncovered that using $.proxy inside svelte/legacy could also have caused these kinds of problems this PR fixes in some situations, but it shouldn't matter anymore now that deeply reactive state is properly handled in the places where it matters.

}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { test } from '../../test';
import { tick } from 'svelte';

export default test({
html: `<button>reassign</button><button>mutate</button><div>0</div>`,

async test({ assert, target }) {
const [btn1, btn2] = target.querySelectorAll('button');

btn1.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<button>reassign</button><button>mutate</button><div>1</div>`
);

btn2.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<button>reassign</button><button>mutate</button><div>2</div>`
);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<script>
let foo = $state({ count: 0 });
let count = $state(0);

function action() {
return {
update(foo) {
count = foo.count;
}
}
}
</script>

<button onclick={() => foo = {...foo, count: foo.count + 1 }}>reassign</button>
<button onclick={() => foo.count++}>mutate</button>
<div use:action={foo}>{count}</div>
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { test } from '../../test';
import { tick } from 'svelte';

export default test({
html: `<button>reassign</button><button>mutate</button><p>0 / 0</p>`,

async test({ assert, target }) {
const [btn1, btn2] = target.querySelectorAll('button');

btn1.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<button>reassign</button><button>mutate</button><p>1 / 1</p>`
);

btn2.click();
await tick();
assert.htmlEqual(
target.innerHTML,
`<button>reassign</button><button>mutate</button><p>2 / 2</p>`
);
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<script>
import Old from './old.svelte';

let prop = $state({ count: 0 });
</script>

<button onclick={() => prop = {...prop, count: prop.count + 1 }}>reassign</button>
<button onclick={() => prop.count++}>mutate</button>
<Old {prop}></Old>
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<svelte:options runes={false} />
<script>
export let prop;
let count_1 = prop.count;
$: {
count_1 = prop.count;
}
$: count_2 = prop.count;
</script>

<p>{count_1} / {count_2}</p>
Loading