msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: correctly handle functions when determining async blockers (#17137)

Browse Source 
* fix: correctly handle functions when determining async blockers

We didn't properly handle functions (function declarations/expressions/arrow functions) when calculating what is a blocker. More specifically
- we did defer assignment of variable declarations even for arrow functions and function expressions, which is unnecessary and causes bugs when they're then referenced eagerly further below
- we did not compute blockers for functions. They could reference blockers themselves, as such other code referencing them should wait on the related blockers

Fixes #17129

* put into its own function

* fix: take blockers into account when creating `#await` blocks

The unrelated-but-in-the-same-issue-referenced-bug

* oops

* fix

* minimize compiled output changes

* no idea why editor showed these as unused
pull/17168/head
Simon H 3 days ago committed by GitHub
parent e0501ede03
commit a7a6d898d5
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
15 changed files with 434 additions and 207 deletions
Show Stats Download Patch File Download Diff File

5
.changeset/gentle-showers-speak.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: correctly handle functions when determining async blockers

5
.changeset/soft-radios-make.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: take blockers into account when creating `#await` blocks

456
packages/svelte/src/compiler/phases/2-analyze/index.js
Unescape View File

@ -687,193 +687,7 @@ export function analyze_component(root, source, options) {
}
}
/**
* @param {ESTree.Node} expression
* @param {Scope} scope
* @param {Set<Binding>} touched
* @param {Set<ESTree.Node>} seen
*/
const touch = (expression, scope, touched, seen = new Set()) => {
if (seen.has(expression)) return;
seen.add(expression);
walk(
expression,
{ scope },
{
ImportDeclaration(node) {},
Identifier(node, context) {
const parent = /** @type {ESTree.Node} */ (context.path.at(-1));
if (is_reference(node, parent)) {
const binding = context.state.scope.get(node.name);
if (binding) {
touched.add(binding);
for (const assignment of binding.assignments) {
touch(assignment.value, assignment.scope, touched, seen);
}
}
}
}
}
);
};
/**
* @param {ESTree.Node} node
* @param {Set<ESTree.Node>} seen
* @param {Set<Binding>} reads
* @param {Set<Binding>} writes
*/
const trace_references = (node, reads, writes, seen = new Set()) => {
if (seen.has(node)) return;
seen.add(node);
/**
* @param {ESTree.Pattern} node
* @param {Scope} scope
*/
function update(node, scope) {
for (const pattern of unwrap_pattern(node)) {
const node = object(pattern);
if (!node) return;
const binding = scope.get(node.name);
if (!binding) return;
writes.add(binding);
}
}
walk(
node,
{ scope: instance.scope },
{
_(node, context) {
const scope = scopes.get(node);
if (scope) {
context.next({ scope });
} else {
context.next();
}
},
AssignmentExpression(node, context) {
update(node.left, context.state.scope);
},
UpdateExpression(node, context) {
update(
/** @type {ESTree.Identifier | ESTree.MemberExpression} */ (node.argument),
context.state.scope
);
},
CallExpression(node, context) {
// for now, assume everything touched by the callee ends up mutating the object
// TODO optimise this better
// special case — no need to peek inside effects as they only run once async work has completed
const rune = get_rune(node, context.state.scope);
if (rune === '$effect') return;
/** @type {Set<Binding>} */
const touched = new Set();
touch(node, context.state.scope, touched);
for (const b of touched) {
writes.add(b);
}
},
// don't look inside functions until they are called
ArrowFunctionExpression(_, context) {},
FunctionDeclaration(_, context) {},
FunctionExpression(_, context) {},
Identifier(node, context) {
const parent = /** @type {ESTree.Node} */ (context.path.at(-1));
if (is_reference(node, parent)) {
const binding = context.state.scope.get(node.name);
if (binding) {
reads.add(binding);
}
}
}
}
);
};
let awaited = false;
// TODO this should probably be attached to the scope?
var promises = b.id('$$promises');
/**
* @param {ESTree.Identifier} id
* @param {ESTree.Expression} blocker
*/
function push_declaration(id, blocker) {
analysis.instance_body.declarations.push(id);
const binding = /** @type {Binding} */ (instance.scope.get(id.name));
binding.blocker = blocker;
}
for (let node of instance.ast.body) {
if (node.type === 'ImportDeclaration') {
analysis.instance_body.hoisted.push(node);
continue;
}
if (node.type === 'ExportDefaultDeclaration' || node.type === 'ExportAllDeclaration') {
// these can't exist inside `<script>` but TypeScript doesn't know that
continue;
}
if (node.type === 'ExportNamedDeclaration') {
if (node.declaration) {
node = node.declaration;
} else {
continue;
}
}
const has_await = has_await_expression(node);
awaited ||= has_await;
if (awaited && node.type !== 'FunctionDeclaration') {
/** @type {Set<Binding>} */
const reads = new Set(); // TODO we're not actually using this yet
/** @type {Set<Binding>} */
const writes = new Set();
trace_references(node, reads, writes);
const blocker = b.member(promises, b.literal(analysis.instance_body.async.length), true);
for (const binding of writes) {
binding.blocker = blocker;
}
if (node.type === 'VariableDeclaration') {
for (const declarator of node.declarations) {
for (const id of extract_identifiers(declarator.id)) {
push_declaration(id, blocker);
}
// one declarator per declaration, makes things simpler
analysis.instance_body.async.push({
node: declarator,
has_await
});
}
} else if (node.type === 'ClassDeclaration') {
push_declaration(node.id, blocker);
analysis.instance_body.async.push({ node, has_await });
} else {
analysis.instance_body.async.push({ node, has_await });
}
} else {
analysis.instance_body.sync.push(node);
}
}
calculate_blockers(instance, scopes, analysis);
if (analysis.runes) {
const props_refs = module.scope.references.get('$$props');
@ -1118,6 +932,274 @@ export function analyze_component(root, source, options) {
return analysis;
}
/**
* Analyzes the instance's top level statements to calculate which bindings need to wait on which
* top level statements. This includes indirect blockers such as functions referencing async top level statements.
*
* @param {Js} instance
* @param {Map<AST.SvelteNode, Scope>} scopes
* @param {ComponentAnalysis} analysis
* @returns {void}
*/
function calculate_blockers(instance, scopes, analysis) {
/**
* @param {ESTree.Node} expression
* @param {Scope} scope
* @param {Set<Binding>} touched
* @param {Set<ESTree.Node>} seen
*/
const touch = (expression, scope, touched, seen = new Set()) => {
if (seen.has(expression)) return;
seen.add(expression);
walk(
expression,
{ scope },
{
ImportDeclaration(node) {},
Identifier(node, context) {
const parent = /** @type {ESTree.Node} */ (context.path.at(-1));
if (is_reference(node, parent)) {
const binding = context.state.scope.get(node.name);
if (binding) {
touched.add(binding);
for (const assignment of binding.assignments) {
touch(assignment.value, assignment.scope, touched, seen);
}
}
}
}
}
);
};
/**
* @param {ESTree.Node} node
* @param {Set<ESTree.Node>} seen
* @param {Set<Binding>} reads
* @param {Set<Binding>} writes
*/
const trace_references = (node, reads, writes, seen = new Set()) => {
if (seen.has(node)) return;
seen.add(node);
/**
* @param {ESTree.Pattern} node
* @param {Scope} scope
*/
function update(node, scope) {
for (const pattern of unwrap_pattern(node)) {
const node = object(pattern);
if (!node) return;
const binding = scope.get(node.name);
if (!binding) return;
writes.add(binding);
}
}
walk(
node,
{ scope: instance.scope },
{
_(node, context) {
const scope = scopes.get(node);
if (scope) {
context.next({ scope });
} else {
context.next();
}
},
AssignmentExpression(node, context) {
update(node.left, context.state.scope);
},
UpdateExpression(node, context) {
update(
/** @type {ESTree.Identifier | ESTree.MemberExpression} */ (node.argument),
context.state.scope
);
},
CallExpression(node, context) {
// for now, assume everything touched by the callee ends up mutating the object
// TODO optimise this better
// special case — no need to peek inside effects as they only run once async work has completed
const rune = get_rune(node, context.state.scope);
if (rune === '$effect') return;
/** @type {Set<Binding>} */
const touched = new Set();
touch(node, context.state.scope, touched);
for (const b of touched) {
writes.add(b);
}
},
// don't look inside functions until they are called
ArrowFunctionExpression(_, context) {},
FunctionDeclaration(_, context) {},
FunctionExpression(_, context) {},
Identifier(node, context) {
const parent = /** @type {ESTree.Node} */ (context.path.at(-1));
if (is_reference(node, parent)) {
const binding = context.state.scope.get(node.name);
if (binding) {
reads.add(binding);
}
}
}
}
);
};
let awaited = false;
// TODO this should probably be attached to the scope?
const promises = b.id('$$promises');
/**
* @param {ESTree.Identifier} id
* @param {NonNullable<Binding['blocker']>} blocker
*/
function push_declaration(id, blocker) {
analysis.instance_body.declarations.push(id);
const binding = /** @type {Binding} */ (instance.scope.get(id.name));
binding.blocker = blocker;
}
/**
* Analysis of blockers for functions is deferred until we know which statements are async/blockers
* @type {Array<ESTree.FunctionDeclaration | ESTree.VariableDeclarator>}
*/
const functions = [];
for (let node of instance.ast.body) {
if (node.type === 'ImportDeclaration') {
analysis.instance_body.hoisted.push(node);
continue;
}
if (node.type === 'ExportDefaultDeclaration' || node.type === 'ExportAllDeclaration') {
// these can't exist inside `<script>` but TypeScript doesn't know that
continue;
}
if (node.type === 'ExportNamedDeclaration') {
if (node.declaration) {
node = node.declaration;
} else {
continue;
}
}
const has_await = has_await_expression(node);
awaited ||= has_await;
if (node.type === 'FunctionDeclaration') {
analysis.instance_body.sync.push(node);
functions.push(node);
} else if (node.type === 'VariableDeclaration') {
for (const declarator of node.declarations) {
if (
declarator.init?.type === 'ArrowFunctionExpression' ||
declarator.init?.type === 'FunctionExpression'
) {
// One declarator per declaration, makes things simpler. The ternary ensures more accurate source maps in the common case
analysis.instance_body.sync.push(
node.declarations.length === 1 ? node : b.declaration(node.kind, [declarator])
);
functions.push(declarator);
} else if (!awaited) {
// One declarator per declaration, makes things simpler. The ternary ensures more accurate source maps in the common case
analysis.instance_body.sync.push(
node.declarations.length === 1 ? node : b.declaration(node.kind, [declarator])
);
} else {
/** @type {Set<Binding>} */
const reads = new Set(); // TODO we're not actually using this yet
/** @type {Set<Binding>} */
const writes = new Set();
trace_references(declarator, reads, writes);
const blocker = /** @type {NonNullable<Binding['blocker']>} */ (
b.member(promises, b.literal(analysis.instance_body.async.length), true)
);
for (const binding of writes) {
binding.blocker = blocker;
}
for (const id of extract_identifiers(declarator.id)) {
push_declaration(id, blocker);
}
// one declarator per declaration, makes things simpler
analysis.instance_body.async.push({
node: declarator,
has_await
});
}
}
} else if (awaited) {
/** @type {Set<Binding>} */
const reads = new Set(); // TODO we're not actually using this yet
/** @type {Set<Binding>} */
const writes = new Set();
trace_references(node, reads, writes);
const blocker = /** @type {NonNullable<Binding['blocker']>} */ (
b.member(promises, b.literal(analysis.instance_body.async.length), true)
);
for (const binding of writes) {
binding.blocker = blocker;
}
if (node.type === 'ClassDeclaration') {
push_declaration(node.id, blocker);
analysis.instance_body.async.push({ node, has_await });
} else {
analysis.instance_body.async.push({ node, has_await });
}
} else {
analysis.instance_body.sync.push(node);
}
}
for (const fn of functions) {
/** @type {Set<Binding>} */
const reads_writes = new Set();
const body =
fn.type === 'VariableDeclarator'
? /** @type {ESTree.FunctionExpression | ESTree.ArrowFunctionExpression} */ (fn.init).body
: fn.body;
trace_references(body, reads_writes, reads_writes);
const max = [...reads_writes].reduce((max, binding) => {
return binding.blocker ? Math.max(binding.blocker.property.value, max) : max;
}, -1);
if (max === -1) continue;
const blocker = b.member(promises, b.literal(max), true);
const binding = /** @type {Binding} */ (
fn.type === 'FunctionDeclaration'
? instance.scope.get(fn.id.name)
: instance.scope.get(/** @type {ESTree.Identifier} */ (fn.id).name)
);
binding.blocker = /** @type {typeof binding['blocker']} */ (blocker);
}
}
/**
* @param {Map<import('estree').LabeledStatement, ReactiveStatement>} unsorted_reactive_declarations
*/

44
packages/svelte/src/compiler/phases/3-transform/client/visitors/AwaitBlock.js
Unescape View File

@ -56,22 +56,36 @@ export function AwaitBlock(node, context) {
catch_block = b.arrow(args, b.block([...declarations, ...block.body]));
}
context.state.init.push(
add_svelte_meta(
b.call(
'$.await',
context.state.node,
expression,
node.pending
? b.arrow([b.id('$$anchor')], /** @type {BlockStatement} */ (context.visit(node.pending)))
: b.null,
then_block,
catch_block
),
node,
'await'
)
const stmt = add_svelte_meta(
b.call(
'$.await',
context.state.node,
expression,
node.pending
? b.arrow([b.id('$$anchor')], /** @type {BlockStatement} */ (context.visit(node.pending)))
: b.null,
then_block,
catch_block
),
node,
'await'
);
if (node.metadata.expression.has_blockers()) {
context.state.init.push(
b.stmt(
b.call(
'$.async',
context.state.node,
node.metadata.expression.blockers(),
b.array([]),
b.arrow([context.state.node], b.block([stmt]))
)
)
);
} else {
context.state.init.push(stmt);
}
}
/**

4
packages/svelte/src/compiler/phases/nodes.js
Unescape View File

@ -112,6 +112,10 @@ export class ExpressionMetadata {
return b.array([...this.#get_blockers()]);
}
has_blockers() {
return this.#get_blockers().size > 0;
}
is_async() {
return this.has_await || this.#get_blockers().size > 0;
}

12
packages/svelte/src/compiler/phases/scope.js
Unescape View File

@ -1,4 +1,4 @@
/** @import { ArrowFunctionExpression, BinaryOperator, ClassDeclaration, Expression, FunctionDeclaration, FunctionExpression, Identifier, ImportDeclaration, MemberExpression, LogicalOperator, Node, Pattern, UnaryOperator, VariableDeclarator, Super } from 'estree' */
/** @import { BinaryOperator, ClassDeclaration, Expression, FunctionDeclaration, Identifier, ImportDeclaration, MemberExpression, LogicalOperator, Node, Pattern, UnaryOperator, VariableDeclarator, Super, SimpleLiteral, FunctionExpression, ArrowFunctionExpression } from 'estree' */
/** @import { Context, Visitor } from 'zimmerframe' */
/** @import { AST, BindingKind, DeclarationKind } from '#compiler' */
import is_reference from 'is-reference';
@ -108,7 +108,10 @@ export class Binding {
/** @type {Array<{ node: Identifier; path: AST.SvelteNode[] }>} */
references = [];
/** @type {Array<{ value: Expression; scope: Scope }>} */
/**
* (Re)assignments of this binding. Includes declarations such as `function x() {}`.
* @type {Array<{ value: Expression; scope: Scope }>}
*/
assignments = [];
/**
@ -135,9 +138,10 @@ export class Binding {
/**
* Instance-level declarations may follow (or contain) a top-level `await`. In these cases,
* any reads that occur in the template must wait for the corresponding promise to resolve
* otherwise the initial value will not have been assigned
* otherwise the initial value will not have been assigned.
* It is a member expression of the form `$$blockers[n]`.
* TODO the blocker is set during transform which feels a bit grubby
* @type {Expression | null}
* @type {MemberExpression & { object: Identifier, property: SimpleLiteral & { value: number } } | null}
*/
blocker = null;

2
packages/svelte/src/compiler/utils/builders.js
Unescape View File

@ -181,7 +181,7 @@ export function logical(operator, left, right) {
}
/**
* @param {'const' | 'let' | 'var'} kind
* @param {ESTree.VariableDeclaration['kind']} kind
* @param {ESTree.VariableDeclarator[]} declarations
* @returns {ESTree.VariableDeclaration}
*/

9
packages/svelte/tests/runtime-runes/samples/async-await-block/_config.js
Unescape View File

@ -0,0 +1,9 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
async test({ assert, target }) {
await tick();
assert.htmlEqual(target.innerHTML, '<p>1</p>');
}
});

7
packages/svelte/tests/runtime-runes/samples/async-await-block/main.svelte
Unescape View File

@ -0,0 +1,7 @@
<script>
let foo = $derived(await 1);
</script>
{#await foo then x}
<p>{x}</p>
{/await}

13
packages/svelte/tests/runtime-runes/samples/async-indirect-blockers/Component1.svelte
Unescape View File

@ -0,0 +1,13 @@
<script>
await 1;
// Test arrow function style
let value = $state('');
const getValue = () => {
return value;
};
const setValue = (v) => { value = v }
</script>
<input bind:value={getValue, setValue} />
<p>{getValue()}</p>

11
packages/svelte/tests/runtime-runes/samples/async-indirect-blockers/Component2.svelte
Unescape View File

@ -0,0 +1,11 @@
<script>
await 1;
// Test function declaration style
let value = $state('');
function getValue() { return value }
function setValue(v) { value = v }
</script>
<input bind:value={getValue, setValue} />
<p>{getValue()}</p>

16
packages/svelte/tests/runtime-runes/samples/async-indirect-blockers/Component3.svelte
Unescape View File

@ -0,0 +1,16 @@
<script>
await 1;
// Test indirect blocker dependencies
let value = $state('');
function x() {
return value;
}
function getValue() {
return x()
}
function setValue(v) { value = v }
</script>
<input bind:value={getValue, setValue} />
<p>{getValue()}</p>

19
packages/svelte/tests/runtime-runes/samples/async-indirect-blockers/Component4.svelte
Unescape View File

@ -0,0 +1,19 @@
<script>
await 1;
let value = $state('');
// getValue is declared BEFORE x
function getValue() {
return x()
}
function x() {
return value;
}
function setValue(v) { value = v }
</script>
<input bind:value={getValue, setValue} />
<p>{getValue()}</p>

27
packages/svelte/tests/runtime-runes/samples/async-indirect-blockers/_config.js
Unescape View File

@ -0,0 +1,27 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
mode: ['async-server', 'client', 'hydrate'],
ssrHtml:
'<input value=""> <p></p> <input value=""> <p></p> <input value=""> <p></p> <input value=""> <p></p>',
async test({ assert, target }) {
await tick();
const inputs = Array.from(target.querySelectorAll('input'));
const paragraphs = Array.from(target.querySelectorAll('p'));
for (let i = 0; i < 4; i++) {
assert.equal(inputs[i].value, '');
assert.htmlEqual(paragraphs[i].innerHTML, '');
inputs[i].value = 'hello';
inputs[i].dispatchEvent(new InputEvent('input', { bubbles: true }));
await tick();
assert.equal(inputs[i].value, 'hello');
assert.htmlEqual(paragraphs[i].innerHTML, 'hello');
}
}
});

11
packages/svelte/tests/runtime-runes/samples/async-indirect-blockers/main.svelte
Unescape View File

@ -0,0 +1,11 @@
<script>
import Component1 from './Component1.svelte';
import Component2 from './Component2.svelte';
import Component3 from './Component3.svelte';
import Component4 from './Component4.svelte';
</script>
<Component1 />
<Component2 />
<Component3 />
<Component4 />
Write Preview
Loading…
Cancel
Save