diff --git a/.changeset/orange-tips-pull.md b/.changeset/orange-tips-pull.md new file mode 100644 index 000000000000..4d474dafaf46 --- /dev/null +++ b/.changeset/orange-tips-pull.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: treat transitive dependencies of each blocks as mutable in legacy mode if item is mutated diff --git a/packages/svelte/src/compiler/phases/2-analyze/visitors/EachBlock.js b/packages/svelte/src/compiler/phases/2-analyze/visitors/EachBlock.js index bd6c936f99a7..0ebfa563cf9a 100644 --- a/packages/svelte/src/compiler/phases/2-analyze/visitors/EachBlock.js +++ b/packages/svelte/src/compiler/phases/2-analyze/visitors/EachBlock.js @@ -1,7 +1,8 @@ -/** @import { AST } from '#compiler' */ +/** @import { AST, Binding } from '#compiler' */ /** @import { Context } from '../types' */ /** @import { Scope } from '../../scope' */ import * as e from '../../../errors.js'; +import { extract_identifiers } from '../../../utils/ast.js'; import { mark_subtree_dynamic } from './shared/fragment.js'; import { validate_block_not_empty, validate_opening_tag } from './shared/utils.js'; @@ -38,5 +39,49 @@ export function EachBlock(node, context) { if (node.key) context.visit(node.key); if (node.fallback) context.visit(node.fallback); + if (!context.state.analysis.runes) { + let mutated = + !!node.context && + extract_identifiers(node.context).some((id) => { + const binding = context.state.scope.get(id.name); + return !!binding?.mutated; + }); + + // collect transitive dependencies... + for (const binding of node.metadata.expression.dependencies) { + collect_transitive_dependencies(binding, node.metadata.transitive_deps); + } + + // ...and ensure they are marked as state, so they can be turned + // into mutable sources and invalidated + if (mutated) { + for (const binding of node.metadata.transitive_deps) { + if ( + binding.kind === 'normal' && + (binding.declaration_kind === 'const' || + binding.declaration_kind === 'let' || + binding.declaration_kind === 'var') + ) { + binding.kind = 'state'; + } + } + } + } + mark_subtree_dynamic(context.path); } + +/** + * @param {Binding} binding + * @param {Set} bindings + * @returns {void} + */ +function collect_transitive_dependencies(binding, bindings) { + bindings.add(binding); + + if (binding.kind === 'legacy_reactive') { + for (const dep of binding.legacy_dependencies) { + collect_transitive_dependencies(dep, bindings); + } + } +} diff --git a/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js b/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js index 5a944b6db303..6c651464f1e8 100644 --- a/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js +++ b/packages/svelte/src/compiler/phases/3-transform/client/visitors/EachBlock.js @@ -1,4 +1,4 @@ -/** @import { BlockStatement, Expression, Identifier, Pattern, Statement } from 'estree' */ +/** @import { BlockStatement, Expression, Identifier, Pattern, SequenceExpression, Statement } from 'estree' */ /** @import { AST, Binding } from '#compiler' */ /** @import { ComponentContext } from '../types' */ /** @import { Scope } from '../../../scope' */ @@ -106,16 +106,6 @@ export function EachBlock(node, context) { } } - // Legacy mode: find the parent each blocks which contain the arrays to invalidate - const indirect_dependencies = collect_parent_each_blocks(context).flatMap((block) => { - const array = /** @type {Expression} */ (context.visit(block.expression)); - const transitive_dependencies = build_transitive_dependencies( - block.metadata.expression.dependencies, - context - ); - return [array, ...transitive_dependencies]; - }); - /** @type {Identifier | null} */ let collection_id = null; @@ -129,18 +119,6 @@ export function EachBlock(node, context) { } } - if (collection_id) { - indirect_dependencies.push(b.call(collection_id)); - } else { - indirect_dependencies.push(collection); - - const transitive_dependencies = build_transitive_dependencies( - each_node_meta.expression.dependencies, - context - ); - indirect_dependencies.push(...transitive_dependencies); - } - const child_state = { ...context.state, transform: { ...context.state.transform }, @@ -181,19 +159,51 @@ export function EachBlock(node, context) { /** @type {Statement[]} */ const declarations = []; - const invalidate = b.call( - '$.invalidate_inner_signals', - b.thunk(b.sequence(indirect_dependencies)) - ); - const invalidate_store = store_to_invalidate ? b.call('$.invalidate_store', b.id('$$stores'), b.literal(store_to_invalidate)) : undefined; /** @type {Expression[]} */ const sequence = []; - if (!context.state.analysis.runes) sequence.push(invalidate); - if (invalidate_store) sequence.push(invalidate_store); + + if (!context.state.analysis.runes) { + /** @type {Set} */ + const transitive_deps = new Set(); + + if (collection_id) { + transitive_deps.add(collection_id); + child_state.transform[collection_id.name] = { read: b.call }; + } else { + for (const binding of each_node_meta.transitive_deps) { + transitive_deps.add(binding.node); + } + } + + for (const block of collect_parent_each_blocks(context)) { + for (const binding of block.metadata.transitive_deps) { + transitive_deps.add(binding.node); + } + } + + if (transitive_deps.size > 0) { + const invalidate = b.call( + '$.invalidate_inner_signals', + b.thunk( + b.sequence( + [...transitive_deps].map( + (node) => /** @type {Expression} */ (context.visit({ ...node }, child_state)) + ) + ) + ) + ); + + sequence.push(invalidate); + } + } + + if (invalidate_store) { + sequence.push(invalidate_store); + } if (node.context?.type === 'Identifier') { const binding = /** @type {Binding} */ (context.state.scope.get(node.context.name)); @@ -329,41 +339,3 @@ export function EachBlock(node, context) { function collect_parent_each_blocks(context) { return /** @type {AST.EachBlock[]} */ (context.path.filter((node) => node.type === 'EachBlock')); } - -/** - * @param {Set} references - * @param {ComponentContext} context - */ -function build_transitive_dependencies(references, context) { - /** @type {Set} */ - const dependencies = new Set(); - - for (const ref of references) { - const deps = collect_transitive_dependencies(ref); - for (const dep of deps) { - dependencies.add(dep); - } - } - - return [...dependencies].map((dep) => build_getter({ ...dep.node }, context.state)); -} - -/** - * @param {Binding} binding - * @param {Set} seen - * @returns {Binding[]} - */ -function collect_transitive_dependencies(binding, seen = new Set()) { - if (binding.kind !== 'legacy_reactive') return []; - - for (const dep of binding.legacy_dependencies) { - if (!seen.has(dep)) { - seen.add(dep); - for (const transitive_dep of collect_transitive_dependencies(dep, seen)) { - seen.add(transitive_dep); - } - } - } - - return [...seen]; -} diff --git a/packages/svelte/src/compiler/phases/scope.js b/packages/svelte/src/compiler/phases/scope.js index 8b4967a82baf..8a2cc39ba735 100644 --- a/packages/svelte/src/compiler/phases/scope.js +++ b/packages/svelte/src/compiler/phases/scope.js @@ -1169,7 +1169,9 @@ export function create_scopes(ast, root, allow_reactive_declarations, parent) { contains_group_binding: false, index: scope.root.unique('$$index'), declarations: scope.declarations, - is_controlled: false + is_controlled: false, + // filled in during analysis + transitive_deps: new Set() }; }, diff --git a/packages/svelte/src/compiler/types/template.d.ts b/packages/svelte/src/compiler/types/template.d.ts index b51c9e9a8dc6..cefc7fa7a20d 100644 --- a/packages/svelte/src/compiler/types/template.d.ts +++ b/packages/svelte/src/compiler/types/template.d.ts @@ -432,6 +432,11 @@ export namespace AST { * This saves us from creating an extra comment and insertion being faster. */ is_controlled: boolean; + /** + * Bindings this each block transitively depends on. In legacy mode, we + * invalidate these bindings when mutations happen to each block items + */ + transitive_deps: Set; }; } diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 77c58720e128..f3111361c051 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -335,7 +335,7 @@ export function prop(props, key, flags, fallback) { } // easy mode — prop is never written to - if ((flags & PROPS_IS_UPDATED) === 0) { + if ((flags & PROPS_IS_UPDATED) === 0 && runes) { return getter; } diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/_config.js new file mode 100644 index 000000000000..d20e4a1e88d3 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/_config.js @@ -0,0 +1,156 @@ +import { flushSync } from 'svelte'; +import { test } from '../../test'; + +export default test({ + get props() { + return { + items: [ + { done: false, text: 'one' }, + { done: true, text: 'two' }, + { done: false, text: 'three' } + ] + }; + }, + + html: ` +
+ +

one

+
+
+ +

two

+
+
+ +

three

+
+ +

remaining:one / done:two / remaining:three

+ `, + + ssrHtml: ` +
+ +

one

+
+
+ +

two

+
+
+ +

three

+
+ +

remaining:one / done:two / remaining:three

+ `, + + test({ assert, component, target, window }) { + /** + * @param {number} i + * @param {string} text + */ + function set_text(i, text) { + const input = /** @type {HTMLInputElement} */ ( + target.querySelectorAll('input[type="text"]')[i] + ); + input.value = text; + input.dispatchEvent(new window.Event('input')); + } + + /** + * @param {number} i + * @param {boolean} done + */ + function set_done(i, done) { + const input = /** @type {HTMLInputElement} */ ( + target.querySelectorAll('input[type="checkbox"]')[i] + ); + input.checked = done; + input.dispatchEvent(new window.Event('change')); + } + + component.filter = 'remaining'; + + assert.htmlEqual( + target.innerHTML, + ` +
+ +

one

+
+
+ +

three

+
+ +

remaining:one / done:two / remaining:three

+ ` + ); + + set_text(1, 'four'); + flushSync(); + + assert.htmlEqual( + target.innerHTML, + ` +
+ +

one

+
+
+ +

four

+
+ +

remaining:one / done:two / remaining:four

+ ` + ); + + assert.deepEqual(component.items, [ + { done: false, text: 'one' }, + { done: true, text: 'two' }, + { done: false, text: 'four' } + ]); + + set_done(0, true); + flushSync(); + + assert.htmlEqual( + target.innerHTML, + ` +
+ +

four

+
+ +

done:one / done:two / remaining:four

+ ` + ); + + assert.deepEqual(component.items, [ + { done: true, text: 'one' }, + { done: true, text: 'two' }, + { done: false, text: 'four' } + ]); + + component.filter = 'done'; + + assert.htmlEqual( + target.innerHTML, + ` +
+ +

one

+
+
+ +

two

+
+ +

done:one / done:two / remaining:four

+ ` + ); + } +}); diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/main.svelte b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/main.svelte new file mode 100644 index 000000000000..7e1019953036 --- /dev/null +++ b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive-prop/main.svelte @@ -0,0 +1,25 @@ + + +{#each filtered as item} +
+ + +

{item.text}

+
+{/each} + +

{summary}

\ No newline at end of file diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/_config.js b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/_config.js index d20e4a1e88d3..c8019e11e94a 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/_config.js +++ b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/_config.js @@ -2,16 +2,6 @@ import { flushSync } from 'svelte'; import { test } from '../../test'; export default test({ - get props() { - return { - items: [ - { done: false, text: 'one' }, - { done: true, text: 'two' }, - { done: false, text: 'three' } - ] - }; - }, - html: `
@@ -108,12 +98,6 @@ export default test({ ` ); - assert.deepEqual(component.items, [ - { done: false, text: 'one' }, - { done: true, text: 'two' }, - { done: false, text: 'four' } - ]); - set_done(0, true); flushSync(); @@ -129,12 +113,6 @@ export default test({ ` ); - assert.deepEqual(component.items, [ - { done: true, text: 'one' }, - { done: true, text: 'two' }, - { done: false, text: 'four' } - ]); - component.filter = 'done'; assert.htmlEqual( diff --git a/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/main.svelte b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/main.svelte index 7e1019953036..9ca7832a72c9 100644 --- a/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/main.svelte +++ b/packages/svelte/tests/runtime-legacy/samples/binding-input-text-contextual-reactive/main.svelte @@ -1,5 +1,10 @@