From f5102013af6ba14774c0fe2d575ea8e06b92697a Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Tue, 20 Feb 2024 10:50:22 +0100 Subject: [PATCH] fix: better interop of `$state` with actions/`$:` statements (#10543) Ensure update methods of actions and reactive statements work with fine-grained `$state` by deep-reading them. This is necessary because mutations to `$state` objects don't notify listeners to only the object as a whole, it only notifies the listeners of the property that changed. fixes #10460 fixes https://github.com/simeydotme/svelte-range-slider-pips/issues/130 --- .changeset/light-days-clean.md | 5 ++++ .../client/visitors/javascript-legacy.js | 6 +++-- packages/svelte/src/internal/client/render.js | 22 ++++++++--------- .../svelte/src/internal/client/runtime.js | 4 +++- packages/svelte/src/internal/index.js | 3 ++- packages/svelte/src/legacy/legacy-client.js | 3 +++ .../samples/binding-backflow/_config.js | 4 ++-- .../samples/action-state-arg/_config.js | 24 +++++++++++++++++++ .../samples/action-state-arg/main.svelte | 16 +++++++++++++ .../_config.js | 24 +++++++++++++++++++ .../main.svelte | 9 +++++++ .../old.svelte | 11 +++++++++ 12 files changed, 113 insertions(+), 18 deletions(-) create mode 100644 .changeset/light-days-clean.md create mode 100644 packages/svelte/tests/runtime-runes/samples/action-state-arg/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/_config.js create mode 100644 packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/main.svelte create mode 100644 packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/old.svelte diff --git a/.changeset/light-days-clean.md b/.changeset/light-days-clean.md new file mode 100644 index 000000000..361e2c2c4 --- /dev/null +++ b/.changeset/light-days-clean.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: ensure update methods of actions and reactive statements work with fine-grained `$state` diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js index ba09d5f04..02f4895ae 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/javascript-legacy.js @@ -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); diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index a4a0e5265..74a41ccc9 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -43,7 +43,8 @@ import { managed_effect, push, current_component_context, - pop + pop, + deep_read } from './runtime.js'; import { current_hydration_fragment, @@ -1890,9 +1891,11 @@ 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) || {}; + needs_deep_read = !!payload?.update; } else { const update = payload.update; if (typeof update === 'function') { @@ -1900,6 +1903,12 @@ export function action(dom, action, value_fn) { } } }); + // 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))); } @@ -2620,17 +2629,6 @@ export function unmount(component) { fn?.(); } -/** - * @param {Record} props - * @returns {void} - */ -export function access_props(props) { - for (const prop in props) { - // eslint-disable-next-line no-unused-expressions - props[prop]; - } -} - /** * @param {Record} props * @returns {Record} diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 8db5cfa54..429fd95d3 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -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} 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) { diff --git a/packages/svelte/src/internal/index.js b/packages/svelte/src/internal/index.js index 31e52917c..939544d8e 100644 --- a/packages/svelte/src/internal/index.js +++ b/packages/svelte/src/internal/index.js @@ -38,7 +38,8 @@ export { inspect, unwrap, freeze, - init + init, + deep_read } from './client/runtime.js'; export { await_block as await } from './client/dom/blocks/await.js'; export { if_block as if } from './client/dom/blocks/if.js'; diff --git a/packages/svelte/src/legacy/legacy-client.js b/packages/svelte/src/legacy/legacy-client.js index 312a62e24..5b962d309 100644 --- a/packages/svelte/src/legacy/legacy-client.js +++ b/packages/svelte/src/legacy/legacy-client.js @@ -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, diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-backflow/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-backflow/_config.js index 7eefd6a75..9e1561f40 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-backflow/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-backflow/_config.js @@ -34,7 +34,7 @@ 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' }); @@ -42,6 +42,6 @@ export default test({ p = parents['init_mutate']; assert.deepEqual(p.value, { foo: 'kid' }); - assert.equal(p.updates.length, 1); + assert.equal(p.updates.length, 2); } }); diff --git a/packages/svelte/tests/runtime-runes/samples/action-state-arg/_config.js b/packages/svelte/tests/runtime-runes/samples/action-state-arg/_config.js new file mode 100644 index 000000000..e2418a6cb --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/action-state-arg/_config.js @@ -0,0 +1,24 @@ +import { test } from '../../test'; +import { tick } from 'svelte'; + +export default test({ + html: `
0
`, + + async test({ assert, target }) { + const [btn1, btn2] = target.querySelectorAll('button'); + + btn1.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + `
1
` + ); + + btn2.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + `
2
` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte b/packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte new file mode 100644 index 000000000..efdea8d55 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte @@ -0,0 +1,16 @@ + + + + +
{count}
diff --git a/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/_config.js b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/_config.js new file mode 100644 index 000000000..67f75b1cf --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/_config.js @@ -0,0 +1,24 @@ +import { test } from '../../test'; +import { tick } from 'svelte'; + +export default test({ + html: `

0 / 0

`, + + async test({ assert, target }) { + const [btn1, btn2] = target.querySelectorAll('button'); + + btn1.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + `

1 / 1

` + ); + + btn2.click(); + await tick(); + assert.htmlEqual( + target.innerHTML, + `

2 / 2

` + ); + } +}); diff --git a/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/main.svelte b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/main.svelte new file mode 100644 index 000000000..96df04df5 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/main.svelte @@ -0,0 +1,9 @@ + + + + + diff --git a/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/old.svelte b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/old.svelte new file mode 100644 index 000000000..0bee43ce5 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/old.svelte @@ -0,0 +1,11 @@ + + + +

{count_1} / {count_2}