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 <[email protected]>

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

Co-authored-by: Rich Harris <[email protected]>

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

Co-authored-by: Rich Harris <[email protected]>

* 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 <[email protected]>

Author: trueadm

.changeset/brave-walls-flow.md

@@ -0,0 +1,5 @@
+---
+"svelte": patch
+---
+
+fix: improved effect sequencing and execution order

.changeset/cool-peas-lick.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+breaking: onDestroy functions run child-first

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

@@ -608,20 +608,38 @@ const legacy_scope_tweaker = {
 		/** @type {import('../types.js').ReactiveStatement} */
 		const reactive_statement = {
 			assignments: new Set(),
-			dependencies: new Set()
+			dependencies: []
 		};
 
 		next({ ...state, reactive_statement, function_depth: state.scope.function_depth + 1 });
 
+		// Every referenced binding becomes a dependency, unless it's on
+		// the left-hand side of an `=` assignment
 		for (const [name, nodes] of state.scope.references) {
 			const binding = state.scope.get(name);
 			if (binding === null) continue;
 
-			// Include bindings that have references other than assignments and their own declarations
-			if (
-				nodes.some((n) => n.node !== binding.node && !reactive_statement.assignments.has(n.node))
-			) {
-				reactive_statement.dependencies.add(binding);
+			for (const { node, path } of nodes) {
+				/** @type {import('estree').Expression} */
+				let left = node;
+
+				let i = path.length - 1;
+				let parent = /** @type {import('estree').Expression} */ (path.at(i));
+				while (parent.type === 'MemberExpression') {
+					left = parent;
+					parent = /** @type {import('estree').Expression} */ (path.at(--i));
+				}
+
+				if (
+					parent.type === 'AssignmentExpression' &&
+					parent.operator === '=' &&
+					parent.left === left
+				) {
+					continue;
+				}
+
+				reactive_statement.dependencies.push(binding);
+				break;
 			}
 		}
 
@@ -630,8 +648,8 @@ const legacy_scope_tweaker = {
 		// Ideally this would be in the validation file, but that isn't possible because this visitor
 		// calls "next" before setting the reactive statements.
 		if (
-			reactive_statement.dependencies.size &&
-			[...reactive_statement.dependencies].every(
+			reactive_statement.dependencies.length &&
+			reactive_statement.dependencies.every(
 				(d) => d.scope === state.analysis.module.scope && d.declaration_kind !== 'const'
 			)
 		) {
@@ -660,15 +678,29 @@ const legacy_scope_tweaker = {
 		}
 	},
 	AssignmentExpression(node, { state, next }) {
-		if (state.reactive_statement && node.operator === '=') {
-			if (node.left.type === 'MemberExpression') {
-				const id = object(node.left);
-				if (id !== null) {
-					state.reactive_statement.assignments.add(id);
-				}
-			} else {
+		if (state.reactive_statement) {
+			const id = node.left.type === 'MemberExpression' ? object(node.left) : node.left;
+			if (id !== null) {
 				for (const id of extract_identifiers(node.left)) {
-					state.reactive_statement.assignments.add(id);
+					const binding = state.scope.get(id.name);
+
+					if (binding) {
+						state.reactive_statement.assignments.add(binding);
+					}
+				}
+			}
+		}
+
+		next();
+	},
+	UpdateExpression(node, { state, next }) {
+		if (state.reactive_statement) {
+			const id = node.argument.type === 'MemberExpression' ? object(node.argument) : node.argument;
+			if (id?.type === 'Identifier') {
+				const binding = state.scope.get(id.name);
+
+				if (binding) {
+					state.reactive_statement.assignments.add(binding);
 				}
 			}
 		}
@@ -1343,21 +1375,21 @@ function order_reactive_statements(unsorted_reactive_declarations) {
 	const lookup = new Map();
 
 	for (const [node, declaration] of unsorted_reactive_declarations) {
-		declaration.assignments.forEach(({ name }) => {
-			const statements = lookup.get(name) ?? [];
+		for (const binding of declaration.assignments) {
+			const statements = lookup.get(binding.node.name) ?? [];
 			statements.push([node, declaration]);
-			lookup.set(name, statements);
-		});
+			lookup.set(binding.node.name, statements);
+		}
 	}
 
 	/** @type {Array<[string, string]>} */
 	const edges = [];
 
 	for (const [, { assignments, dependencies }] of unsorted_reactive_declarations) {
-		for (const { name } of assignments) {
-			for (const { node } of dependencies) {
-				if (![...assignments].find(({ name }) => node.name === name)) {
-					edges.push([name, node.name]);
+		for (const assignment of assignments) {
+			for (const dependency of dependencies) {
+				if (!assignments.has(dependency)) {
+					edges.push([assignment.node.name, dependency.node.name]);
 				}
 			}
 		}
@@ -1381,14 +1413,17 @@ function order_reactive_statements(unsorted_reactive_declarations) {
 	 */
 	const add_declaration = (node, declaration) => {
 		if ([...reactive_declarations.values()].includes(declaration)) return;
-		declaration.dependencies.forEach(({ node: { name } }) => {
-			if ([...declaration.assignments].some((a) => a.name === name)) return;
-			for (const [node, earlier] of lookup.get(name) ?? []) {
+
+		for (const binding of declaration.dependencies) {
+			if (declaration.assignments.has(binding)) continue;
+			for (const [node, earlier] of lookup.get(binding.node.name) ?? []) {
 				add_declaration(node, earlier);
 			}
-		});
+		}
+
 		reactive_declarations.set(node, declaration);
 	};
+
 	for (const [node, declaration] of unsorted_reactive_declarations) {
 		add_declaration(node, declaration);
 	}

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

@@ -233,7 +233,7 @@ function setup_select_synchronization(value_binding, context) {
 	context.state.init.push(
 		b.stmt(
 			b.call(
-				'$.pre_effect',
+				'$.render_effect',
 				b.thunk(
 					b.block([
 						b.stmt(

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

@@ -23,8 +23,8 @@ export interface Template {
 }
 
 export interface ReactiveStatement {
-	assignments: Set<Identifier>;
-	dependencies: Set<Binding>;
+	assignments: Set<Binding>;
+	dependencies: Binding[];
 }
 
 export interface RawWarning {

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

@@ -1,9 +1,9 @@
 export const DERIVED = 1 << 1;
 export const EFFECT = 1 << 2;
-export const PRE_EFFECT = 1 << 3;
-export const RENDER_EFFECT = 1 << 4;
-export const BLOCK_EFFECT = 1 << 5;
-export const BRANCH_EFFECT = 1 << 6;
+export const RENDER_EFFECT = 1 << 3;
+export const BLOCK_EFFECT = 1 << 4;
+export const BRANCH_EFFECT = 1 << 5;
+export const ROOT_EFFECT = 1 << 6;
 export const UNOWNED = 1 << 7;
 export const CLEAN = 1 << 8;
 export const DIRTY = 1 << 9;
@@ -12,7 +12,6 @@ export const INERT = 1 << 11;
 export const DESTROYED = 1 << 12;
 export const IS_ELSEIF = 1 << 13;
 export const EFFECT_RAN = 1 << 14;
-export const ROOT_EFFECT = 1 << 15;
 
 export const UNINITIALIZED = Symbol();
 export const STATE_SYMBOL = Symbol('$state');

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

@@ -47,8 +47,6 @@ export function bind_this(element_or_component, update, get_value, get_parts) {
 		});
 
 		return () => {
-			// Defer to the next tick so that all updates can be reconciled first.
-			// This solves the case where one variable is shared across multiple this-bindings.
 			effect(() => {
 				if (parts && is_bound_this(get_value(...parts), element_or_component)) {
 					update(null, ...parts);