From c8b75fa546505ba2dce332184efe5691d5aba1b5 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 19 Feb 2024 17:35:58 +0100 Subject: [PATCH 1/5] fix: better interop of `$state` with actions/`$:` statements 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 | 23 +++++++++++++++--- .../svelte/src/internal/client/runtime.js | 4 +++- packages/svelte/src/internal/index.js | 3 ++- .../samples/action-state-arg/_config.js | 24 +++++++++++++++++++ .../samples/action-state-arg/main.svelte | 16 +++++++++++++ .../_config.js | 24 +++++++++++++++++++ .../main.svelte | 9 +++++++ .../old.svelte | 11 +++++++++ 10 files changed, 118 insertions(+), 7 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 000000000000..361e2c2c47fa --- /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 ba09d5f04e74..02f4895ae9d4 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 db2cee8e0159..51890e5089cc 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -49,7 +49,8 @@ import { managed_effect, push, current_component_context, - pop + pop, + deep_read } from './runtime.js'; import { current_hydration_fragment, @@ -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))); } @@ -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); } } diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 8db5cfa54f91..429fd95d3461 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 9077585a3380..5ee100cc9097 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 * from './client/each.js'; export * from './client/render.js'; 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 000000000000..ecd2c9ad61f0 --- /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 000000000000..dda1753817bb --- /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 000000000000..67f75b1cf5e2 --- /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 000000000000..96df04df59fa --- /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 000000000000..0bee43ce5c07 --- /dev/null +++ b/packages/svelte/tests/runtime-runes/samples/fine-grained-prop-reactive-statement/old.svelte @@ -0,0 +1,11 @@ + + + +

{count_1} / {count_2}

From 45f6c110ed353acb2bea5aff58b2f2a97b19a969 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 20 Feb 2024 10:07:50 +0100 Subject: [PATCH 2/5] fix test, add comment --- packages/svelte/src/legacy/legacy-client.js | 3 +++ .../tests/runtime-legacy/samples/binding-backflow/_config.js | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/svelte/src/legacy/legacy-client.js b/packages/svelte/src/legacy/legacy-client.js index 312a62e24bf0..5b962d309915 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 7eefd6a75ca6..9e1561f40e17 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); } }); From 9b6d41bec3e537895b95648f2144fc660ca303e9 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 20 Feb 2024 10:35:54 +0100 Subject: [PATCH 3/5] fix --- packages/svelte/src/internal/client/render.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index 51890e5089cc..05cc8cd79eb1 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -2317,10 +2317,10 @@ export function action(dom, action, value_fn) { untrack(() => { if (payload === undefined) { payload = action(dom, value) || {}; + needs_deep_read = !!payload?.update; } else { const update = payload.update; if (typeof update === 'function') { - needs_deep_read = true; update(value); } } From c1e5eea6f4b9dc5cc5b401879cd5e98d92bef317 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 20 Feb 2024 10:36:31 +0100 Subject: [PATCH 4/5] no longer needed --- packages/svelte/src/internal/client/render.js | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/packages/svelte/src/internal/client/render.js b/packages/svelte/src/internal/client/render.js index 05cc8cd79eb1..31520b7bb361 100644 --- a/packages/svelte/src/internal/client/render.js +++ b/packages/svelte/src/internal/client/render.js @@ -3051,25 +3051,6 @@ export function unmount(component) { fn?.(); } -/** - * @param {Record} props - * @returns {void} - */ -export function access_props(props) { - for (const prop in props) { - deep_read(props[prop]); - } -} - -/** - * @param {any[]} values - */ -export function deep_read_all(...values) { - for (const value of values) { - deep_read(value); - } -} - /** * @param {Record} props * @returns {Record} From e94df0f14127c931105678d2dfb4ef177d3b0957 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Tue, 20 Feb 2024 10:37:52 +0100 Subject: [PATCH 5/5] tweak test which would've caught my bug --- .../tests/runtime-runes/samples/action-state-arg/_config.js | 6 +++--- .../runtime-runes/samples/action-state-arg/main.svelte | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) 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 index ecd2c9ad61f0..e2418a6cb8c5 100644 --- a/packages/svelte/tests/runtime-runes/samples/action-state-arg/_config.js +++ b/packages/svelte/tests/runtime-runes/samples/action-state-arg/_config.js @@ -2,7 +2,7 @@ import { test } from '../../test'; import { tick } from 'svelte'; export default test({ - html: `
0
`, + html: `
0
`, async test({ assert, target }) { const [btn1, btn2] = target.querySelectorAll('button'); @@ -11,14 +11,14 @@ export default test({ 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/action-state-arg/main.svelte b/packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte index dda1753817bb..efdea8d55096 100644 --- a/packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte +++ b/packages/svelte/tests/runtime-runes/samples/action-state-arg/main.svelte @@ -11,6 +11,6 @@ } - +
{count}