more effect ordering stuff (#10958)

* WIP

* i guess this change makes sense?

* simplify

Author: Rich-Harris

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

@@ -22,7 +22,7 @@ function is_bound_this(bound_value, element_or_component) {
  * @returns {void}
  */
 export function bind_this(element_or_component, update, get_value, get_parts) {
-	render_effect(() => {
+	effect(() => {
 		/** @type {unknown[]} */
 		var old_parts;
 

packages/svelte/src/internal/client/reactivity/effects.js

@@ -34,10 +34,9 @@ import { remove } from '../dom/reconciler.js';
  * @param {import('./types.js').EffectType} type
  * @param {(() => void | (() => void))} fn
  * @param {boolean} sync
- * @param {boolean} init
  * @returns {import('#client').Effect}
  */
-function create_effect(type, fn, sync, init = true) {
+function create_effect(type, fn, sync) {
 	var is_root = (type & ROOT_EFFECT) !== 0;
 	/** @type {import('#client').Effect} */
 	var effect = {
@@ -61,20 +60,18 @@ function create_effect(type, fn, sync, init = true) {
 		}
 	}
 
-	if (init) {
-		if (sync) {
-			var previously_flushing_effect = is_flushing_effect;
+	if (sync) {
+		var previously_flushing_effect = is_flushing_effect;
 
-			try {
-				set_is_flushing_effect(true);
-				execute_effect(effect);
-				effect.f |= EFFECT_RAN;
-			} finally {
-				set_is_flushing_effect(previously_flushing_effect);
-			}
-		} else {
-			schedule_effect(effect);
+		try {
+			set_is_flushing_effect(true);
+			execute_effect(effect);
+			effect.f |= EFFECT_RAN;
+		} finally {
+			set_is_flushing_effect(previously_flushing_effect);
 		}
+	} else {
+		schedule_effect(effect);
 	}
 
 	return effect;
@@ -91,7 +88,6 @@ export function effect_active() {
 /**
  * Internal representation of `$effect(...)`
  * @param {() => void | (() => void)} fn
- * @returns {import('#client').Effect}
  */
 export function user_effect(fn) {
 	if (current_effect === null) {
@@ -101,7 +97,20 @@ export function user_effect(fn) {
 		);
 	}
 
-	return create_effect(EFFECT, fn, false, true);
+	// Non-nested `$effect(...)` in a component should be deferred
+	// until the component is mounted
+	const defer =
+		current_effect.f & RENDER_EFFECT &&
+		// TODO do we actually need this? removing them changes nothing
+		current_component_context !== null &&
+		!current_component_context.m;
+
+	if (defer) {
+		const context = /** @type {import('#client').ComponentContext} */ (current_component_context);
+		(context.e ??= []).push(fn);
+	} else {
+		effect(fn);
+	}
 }
 
 /**

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

@@ -8,7 +8,7 @@ import {
 	object_prototype
 } from './utils.js';
 import { unstate } from './proxy.js';
-import { destroy_effect, user_pre_effect } from './reactivity/effects.js';
+import { destroy_effect, effect, user_pre_effect } from './reactivity/effects.js';
 import {
 	EFFECT,
 	RENDER_EFFECT,
@@ -1094,6 +1094,8 @@ export function push(props, runes = false, fn) {
 		x: null,
 		// context
 		c: null,
+		// effects
+		e: null,
 		// mounted
 		m: false,
 		// parent
@@ -1129,6 +1131,13 @@ export function pop(component) {
 		if (component !== undefined) {
 			context_stack_item.x = component;
 		}
+		const effects = context_stack_item.e;
+		if (effects !== null) {
+			context_stack_item.e = null;
+			for (let i = 0; i < effects.length; i++) {
+				effect(effects[i]);
+			}
+		}
 		current_component_context = context_stack_item.p;
 		context_stack_item.m = true;
 	}

packages/svelte/src/internal/client/types.d.ts

@@ -20,6 +20,8 @@ export type ComponentContext = {
 	s: Record<string, unknown>;
 	/** exports (and props, if `accessors: true`) */
 	x: Record<string, any> | null;
+	/** deferred effects */
+	e: null | Array<() => void | (() => void)>;
 	/** mounted */
 	m: boolean;
 	/** parent */

packages/svelte/tests/runtime-legacy/samples/ondestroy-deep/_config.js

@@ -2,14 +2,13 @@ import { test } from '../../test';
 import { destroyed, reset } from './destroyed.js';
 
 export default test({
-	test({ assert, component }) {
-		// for hydration, ssr may have pushed to `destroyed`
+	before_test() {
 		reset();
+	},
 
+	test({ assert, component }) {
 		component.visible = false;
 		assert.deepEqual(destroyed, ['A', 'B', 'C']);
-
-		reset();
 	},
 
 	test_ssr({ assert }) {

packages/svelte/tests/signals/test.ts

@@ -1,8 +1,8 @@
 import { describe, assert, it } from 'vitest';
 import * as $ from '../../src/internal/client/runtime';
 import {
-	destroy_effect,
 	effect,
+	effect_root,
 	render_effect,
 	user_effect
 } from '../../src/internal/client/reactivity/effects';
@@ -22,12 +22,12 @@ function run_test(runes: boolean, fn: (runes: boolean) => () => void) {
 		$.push({}, runes);
 		// Create a render context so that effect validations etc don't fail
 		let execute: any;
-		const signal = render_effect(() => {
+		const destroy = effect_root(() => {
 			execute = fn(runes);
 		});
 		$.pop();
 		execute();
-		destroy_effect(signal);
+		destroy();
 	};
 }
 
@@ -248,8 +248,10 @@ describe('signals', () => {
 	test('effect with derived using unowned derived every time', () => {
 		const log: Array<number | string> = [];
 
-		const effect = user_effect(() => {
-			log.push($.get(calc));
+		const destroy = effect_root(() => {
+			user_effect(() => {
+				log.push($.get(calc));
+			});
 		});
 
 		return () => {
@@ -261,7 +263,7 @@ describe('signals', () => {
 			// Ensure we're not leaking consumers
 			assert.deepEqual(count.reactions?.length, 1);
 			assert.deepEqual(log, [0, 2, 'limit', 0]);
-			destroy_effect(effect);
+			destroy();
 			// Ensure we're not leaking consumers
 			assert.deepEqual(count.reactions, null);
 		};