Skip to content

Commit d50b766

Browse files
trueadmRich-Harris
trueadm
and
Rich-Harris
authored
fix: improve effect sequencing and execution order (#10949)
* WIP * WIP * address bad merge conflict * add test * fix issues * remove debugger * increase count * increase count * something different * change * change * try it * better comment * remove deadcode * move to continue * fix tests * add optimization * unksip test * Update packages/svelte/src/internal/client/dom/elements/bindings/this.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/internal/client/dom/elements/bindings/this.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * Update packages/svelte/src/internal/client/dom/elements/bindings/this.js Co-authored-by: Rich Harris <rich.harris@vercel.com> * remove import * add changeset * tweaks * code golf * remove pre effects * more effect ordering stuff (#10958) * WIP * i guess this change makes sense? * simplify * delete unused code * delete pre_effect * note to self * tidy up * typos * style tweaks * style tweaks * improve reactive statement handling * no return needed * let prettier put everything on a single line * style tweaks * var * failing test * fix test * fix ordering * simplify * ondestroy * working * note breaking change --------- Co-authored-by: Rich Harris <rich.harris@vercel.com>
1 parent 2079e67 commit d50b766

File tree

25 files changed

+401
-309
lines changed

25 files changed

+401
-309
lines changed

‎.changeset/brave-walls-flow.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"svelte": patch
3+
---
4+
5+
fix: improved effect sequencing and execution order

‎.changeset/cool-peas-lick.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'svelte': patch
3+
---
4+
5+
breaking: onDestroy functions run child-first

‎packages/svelte/src/compiler/phases/2-analyze/index.js‎

Lines changed: 63 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -608,20 +608,38 @@ const legacy_scope_tweaker = {
608608
/** @type {import('../types.js').ReactiveStatement} */
609609
const reactive_statement = {
610610
assignments: new Set(),
611-
dependencies: new Set()
611+
dependencies: []
612612
};
613613

614614
next({ ...state, reactive_statement, function_depth: state.scope.function_depth + 1 });
615615

616+
// Every referenced binding becomes a dependency, unless it's on
617+
// the left-hand side of an `=` assignment
616618
for (const [name, nodes] of state.scope.references) {
617619
const binding = state.scope.get(name);
618620
if (binding === null) continue;
619621

620-
// Include bindings that have references other than assignments and their own declarations
621-
if (
622-
nodes.some((n) => n.node !== binding.node && !reactive_statement.assignments.has(n.node))
623-
) {
624-
reactive_statement.dependencies.add(binding);
622+
for (const { node, path } of nodes) {
623+
/** @type {import('estree').Expression} */
624+
let left = node;
625+
626+
let i = path.length - 1;
627+
let parent = /** @type {import('estree').Expression} */ (path.at(i));
628+
while (parent.type === 'MemberExpression') {
629+
left = parent;
630+
parent = /** @type {import('estree').Expression} */ (path.at(--i));
631+
}
632+
633+
if (
634+
parent.type === 'AssignmentExpression' &&
635+
parent.operator === '=' &&
636+
parent.left === left
637+
) {
638+
continue;
639+
}
640+
641+
reactive_statement.dependencies.push(binding);
642+
break;
625643
}
626644
}
627645

@@ -630,8 +648,8 @@ const legacy_scope_tweaker = {
630648
// Ideally this would be in the validation file, but that isn't possible because this visitor
631649
// calls "next" before setting the reactive statements.
632650
if (
633-
reactive_statement.dependencies.size &&
634-
[...reactive_statement.dependencies].every(
651+
reactive_statement.dependencies.length &&
652+
reactive_statement.dependencies.every(
635653
(d) => d.scope === state.analysis.module.scope && d.declaration_kind !== 'const'
636654
)
637655
) {
@@ -660,15 +678,29 @@ const legacy_scope_tweaker = {
660678
}
661679
},
662680
AssignmentExpression(node, { state, next }) {
663-
if (state.reactive_statement && node.operator === '=') {
664-
if (node.left.type === 'MemberExpression') {
665-
const id = object(node.left);
666-
if (id !== null) {
667-
state.reactive_statement.assignments.add(id);
668-
}
669-
} else {
681+
if (state.reactive_statement) {
682+
const id = node.left.type === 'MemberExpression' ? object(node.left) : node.left;
683+
if (id !== null) {
670684
for (const id of extract_identifiers(node.left)) {
671-
state.reactive_statement.assignments.add(id);
685+
const binding = state.scope.get(id.name);
686+
687+
if (binding) {
688+
state.reactive_statement.assignments.add(binding);
689+
}
690+
}
691+
}
692+
}
693+
694+
next();
695+
},
696+
UpdateExpression(node, { state, next }) {
697+
if (state.reactive_statement) {
698+
const id = node.argument.type === 'MemberExpression' ? object(node.argument) : node.argument;
699+
if (id?.type === 'Identifier') {
700+
const binding = state.scope.get(id.name);
701+
702+
if (binding) {
703+
state.reactive_statement.assignments.add(binding);
672704
}
673705
}
674706
}
@@ -1343,21 +1375,21 @@ function order_reactive_statements(unsorted_reactive_declarations) {
13431375
const lookup = new Map();
13441376

13451377
for (const [node, declaration] of unsorted_reactive_declarations) {
1346-
declaration.assignments.forEach(({ name }) => {
1347-
const statements = lookup.get(name) ?? [];
1378+
for (const binding of declaration.assignments) {
1379+
const statements = lookup.get(binding.node.name) ?? [];
13481380
statements.push([node, declaration]);
1349-
lookup.set(name, statements);
1350-
});
1381+
lookup.set(binding.node.name, statements);
1382+
}
13511383
}
13521384

13531385
/** @type {Array<[string, string]>} */
13541386
const edges = [];
13551387

13561388
for (const [, { assignments, dependencies }] of unsorted_reactive_declarations) {
1357-
for (const { name } of assignments) {
1358-
for (const { node } of dependencies) {
1359-
if (![...assignments].find(({ name }) => node.name === name)) {
1360-
edges.push([name, node.name]);
1389+
for (const assignment of assignments) {
1390+
for (const dependency of dependencies) {
1391+
if (!assignments.has(dependency)) {
1392+
edges.push([assignment.node.name, dependency.node.name]);
13611393
}
13621394
}
13631395
}
@@ -1381,14 +1413,17 @@ function order_reactive_statements(unsorted_reactive_declarations) {
13811413
*/
13821414
const add_declaration = (node, declaration) => {
13831415
if ([...reactive_declarations.values()].includes(declaration)) return;
1384-
declaration.dependencies.forEach(({ node: { name } }) => {
1385-
if ([...declaration.assignments].some((a) => a.name === name)) return;
1386-
for (const [node, earlier] of lookup.get(name) ?? []) {
1416+
1417+
for (const binding of declaration.dependencies) {
1418+
if (declaration.assignments.has(binding)) continue;
1419+
for (const [node, earlier] of lookup.get(binding.node.name) ?? []) {
13871420
add_declaration(node, earlier);
13881421
}
1389-
});
1422+
}
1423+
13901424
reactive_declarations.set(node, declaration);
13911425
};
1426+
13921427
for (const [node, declaration] of unsorted_reactive_declarations) {
13931428
add_declaration(node, declaration);
13941429
}

‎packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ function setup_select_synchronization(value_binding, context) {
233233
context.state.init.push(
234234
b.stmt(
235235
b.call(
236-
'$.pre_effect',
236+
'$.render_effect',
237237
b.thunk(
238238
b.block([
239239
b.stmt(

‎packages/svelte/src/compiler/phases/types.d.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ export interface Template {
2323
}
2424

2525
export interface ReactiveStatement {
26-
assignments: Set<Identifier>;
27-
dependencies: Set<Binding>;
26+
assignments: Set<Binding>;
27+
dependencies: Binding[];
2828
}
2929

3030
export interface RawWarning {

‎packages/svelte/src/internal/client/constants.js‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
export const DERIVED = 1 << 1;
22
export const EFFECT = 1 << 2;
3-
export const PRE_EFFECT = 1 << 3;
4-
export const RENDER_EFFECT = 1 << 4;
5-
export const BLOCK_EFFECT = 1 << 5;
6-
export const BRANCH_EFFECT = 1 << 6;
3+
export const RENDER_EFFECT = 1 << 3;
4+
export const BLOCK_EFFECT = 1 << 4;
5+
export const BRANCH_EFFECT = 1 << 5;
6+
export const ROOT_EFFECT = 1 << 6;
77
export const UNOWNED = 1 << 7;
88
export const CLEAN = 1 << 8;
99
export const DIRTY = 1 << 9;
@@ -12,7 +12,6 @@ export const INERT = 1 << 11;
1212
export const DESTROYED = 1 << 12;
1313
export const IS_ELSEIF = 1 << 13;
1414
export const EFFECT_RAN = 1 << 14;
15-
export const ROOT_EFFECT = 1 << 15;
1615

1716
export const UNINITIALIZED = Symbol();
1817
export const STATE_SYMBOL = Symbol('$state');

‎packages/svelte/src/internal/client/dom/elements/bindings/this.js‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,6 @@ export function bind_this(element_or_component, update, get_value, get_parts) {
4747
});
4848

4949
return () => {
50-
// Defer to the next tick so that all updates can be reconciled first.
51-
// This solves the case where one variable is shared across multiple this-bindings.
5250
effect(() => {
5351
if (parts && is_bound_this(get_value(...parts), element_or_component)) {
5452
update(null, ...parts);

0 commit comments

Comments
 (0)