fix: use local mutable sources for props in legacy mode in case they are indirectly invalidated (#16038)

* fix: use local mutable sources for props in legacy mode in case they are indirectly invalidated

* rename test

* add another test

* fix

* more conservative

* Update .changeset/orange-tips-pull.md

* skip work in runes mode

* remove comment

* revert whitespace change
pull/16045/head
Rich Harris 4 months ago committed by GitHub
parent 2f90dd8246
commit 54990f2329
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: treat transitive dependencies of each blocks as mutable in legacy mode if item is mutated

@ -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<Binding>} 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);
}
}
}

@ -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<Identifier>} */
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<Binding>} references
* @param {ComponentContext} context
*/
function build_transitive_dependencies(references, context) {
/** @type {Set<Binding>} */
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<Binding>} 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];
}

@ -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()
};
},

@ -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<Binding>;
};
}

@ -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;
}

@ -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: `
<div>
<input type="checkbox">
<input type="text"><p>one</p>
</div>
<div>
<input type="checkbox">
<input type="text"><p>two</p>
</div>
<div>
<input type="checkbox">
<input type="text"><p>three</p>
</div>
<p>remaining:one / done:two / remaining:three</p>
`,
ssrHtml: `
<div>
<input type="checkbox">
<input type="text" value=one><p>one</p>
</div>
<div>
<input type="checkbox" checked="">
<input type="text" value=two><p>two</p>
</div>
<div>
<input type="checkbox">
<input type="text" value=three><p>three</p>
</div>
<p>remaining:one / done:two / remaining:three</p>
`,
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,
`
<div>
<input type="checkbox">
<input type="text"><p>one</p>
</div>
<div>
<input type="checkbox">
<input type="text"><p>three</p>
</div>
<p>remaining:one / done:two / remaining:three</p>
`
);
set_text(1, 'four');
flushSync();
assert.htmlEqual(
target.innerHTML,
`
<div>
<input type="checkbox">
<input type="text"><p>one</p>
</div>
<div>
<input type="checkbox">
<input type="text"><p>four</p>
</div>
<p>remaining:one / done:two / remaining:four</p>
`
);
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,
`
<div>
<input type="checkbox">
<input type="text"><p>four</p>
</div>
<p>done:one / done:two / remaining:four</p>
`
);
assert.deepEqual(component.items, [
{ done: true, text: 'one' },
{ done: true, text: 'two' },
{ done: false, text: 'four' }
]);
component.filter = 'done';
assert.htmlEqual(
target.innerHTML,
`
<div>
<input type="checkbox">
<input type="text"><p>one</p>
</div>
<div>
<input type="checkbox">
<input type="text"><p>two</p>
</div>
<p>done:one / done:two / remaining:four</p>
`
);
}
});

@ -0,0 +1,25 @@
<script>
export let items;
export let filter = 'all';
$: done = items.filter(item => item.done);
$: remaining = items.filter(item => !item.done);
$: filtered = (
filter === 'all' ? items :
filter === 'done' ? done :
remaining
);
$: summary = items.map(i => `${i.done ? 'done' : 'remaining'}:${i.text}`).join(' / ');
</script>
{#each filtered as item}
<div>
<input type="checkbox" bind:checked={item.done}>
<input type="text" bind:value={item.text}>
<p>{item.text}</p>
</div>
{/each}
<p>{summary}</p>

@ -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: `
<div>
<input type="checkbox">
@ -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(

@ -1,5 +1,10 @@
<script>
export let items;
let items = [
{ done: false, text: 'one' },
{ done: true, text: 'two' },
{ done: false, text: 'three' }
];
export let filter = 'all';
$: done = items.filter(item => item.done);
@ -22,4 +27,4 @@
</div>
{/each}
<p>{summary}</p>
<p>{summary}</p>

Loading…
Cancel
Save