fix: make effects depend on state created inside them (#16198)

dummdidumm · web-flow · commit 7486eabd54d0 · 2025-06-18T11:11:47.000-04:00
* make effects depend on state created inside them

* fix, add github action

* disable test in async mode
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -43,6 +43,23 @@ jobs:
       - run: pnpm test
         env:
           CI: true
+  TestNoAsync:
+    permissions: {}
+    runs-on: ubuntu-latest
+    timeout-minutes: 10
+    steps:
+      - uses: actions/checkout@v4
+      - uses: pnpm/action-setup@v4
+      - uses: actions/setup-node@v4
+        with:
+          node-version: 22
+          cache: pnpm
+      - run: pnpm install --frozen-lockfile
+      - run: pnpm playwright install chromium
+      - run: pnpm test runtime-runes
+        env:
+          CI: true
+          SVELTE_NO_ASYNC: true
   Lint:
     permissions: {}
     runs-on: ubuntu-latest
diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
@@ -57,7 +57,6 @@ import {
 import * as w from './warnings.js';
 import { current_batch, Batch, batch_deriveds } from './reactivity/batch.js';
 import { handle_error, invoke_error_boundary } from './error-handling.js';
-import { snapshot } from '../shared/clone.js';
 
 /** @type {Effect | null} */
 let last_scheduled_effect = null;
@@ -259,7 +258,12 @@ function schedule_possible_effect_self_invalidation(signal, effect, root = true)
 	for (var i = 0; i < reactions.length; i++) {
 		var reaction = reactions[i];
 
-		if (reaction_sources?.[1].includes(signal) && reaction_sources[0] === active_reaction) continue;
+		if (
+			!async_mode_flag &&
+			reaction_sources?.[1].includes(signal) &&
+			reaction_sources[0] === active_reaction
+		)
+			continue;
 
 		if ((reaction.f & DERIVED) !== 0) {
 			schedule_possible_effect_self_invalidation(/** @type {Derived} */ (reaction), effect, false);
@@ -299,7 +303,9 @@ export function update_reaction(reaction) {
 	untracking = false;
 	read_version++;
 
-	reaction.f |= EFFECT_IS_UPDATING;
+	if (!async_mode_flag || (reaction.f & DERIVED) !== 0) {
+		reaction.f |= EFFECT_IS_UPDATING;
+	}
 
 	if (reaction.ac !== null) {
 		reaction.ac?.abort(STALE_REACTION);
@@ -383,7 +389,9 @@ export function update_reaction(reaction) {
 		set_component_context(previous_component_context);
 		untracking = previous_untracking;
 
-		reaction.f ^= EFFECT_IS_UPDATING;
+		if (!async_mode_flag || (reaction.f & DERIVED) !== 0) {
+			reaction.f ^= EFFECT_IS_UPDATING;
+		}
 	}
 }
 
@@ -776,7 +784,9 @@ export function get(signal) {
 
 		if (
 			!destroyed &&
-			(!reaction_sources?.[1].includes(signal) || reaction_sources[0] !== active_reaction)
+			((async_mode_flag && (active_reaction.f & DERIVED) === 0) ||
+				!reaction_sources?.[1].includes(signal) ||
+				reaction_sources[0] !== active_reaction)
 		) {
 			var deps = active_reaction.deps;
 
diff --git a/packages/svelte/src/internal/flags/index.js b/packages/svelte/src/internal/flags/index.js
@@ -6,6 +6,11 @@ export function enable_async_mode_flag() {
 	async_mode_flag = true;
 }
 
+/** ONLY USE THIS DURING TESTING */
+export function disable_async_mode_flag() {
+	async_mode_flag = false;
+}
+
 export function enable_legacy_mode_flag() {
 	legacy_mode_flag = true;
 }
diff --git a/packages/svelte/tests/helpers.js b/packages/svelte/tests/helpers.js
@@ -194,6 +194,8 @@ if (typeof window !== 'undefined') {
 
 export const fragments = /** @type {'html' | 'tree'} */ (process.env.FRAGMENTS) ?? 'html';
 
+export const async_mode = process.env.SVELTE_NO_ASYNC !== 'true';
+
 /**
  * @param {any[]} logs
  */
diff --git a/packages/svelte/tests/runtime-legacy/shared.ts b/packages/svelte/tests/runtime-legacy/shared.ts
@@ -3,10 +3,10 @@ import { setImmediate } from 'node:timers/promises';
 import { globSync } from 'tinyglobby';
 import { createClassComponent } from 'svelte/legacy';
 import { proxy } from 'svelte/internal/client';
-import { flushSync, hydrate, mount, unmount, untrack } from 'svelte';
+import { flushSync, hydrate, mount, unmount } from 'svelte';
 import { render } from 'svelte/server';
 import { afterAll, assert, beforeAll } from 'vitest';
-import { compile_directory, fragments } from '../helpers.js';
+import { async_mode, compile_directory, fragments } from '../helpers.js';
 import { assert_html_equal, assert_html_equal_with_options } from '../html_equal.js';
 import { raf } from '../animation-helpers.js';
 import type { CompileOptions } from '#compiler';
@@ -45,6 +45,10 @@ export interface RuntimeTest<Props extends Record<string, any> = Record<string,
 	mode?: Array<'server' | 'client' | 'hydrate'>;
 	/** Temporarily skip specific modes, without skipping the entire test */
 	skip_mode?: Array<'server' | 'client' | 'hydrate'>;
+	/** Skip if running with process.env.NO_ASYNC */
+	skip_no_async?: boolean;
+	/** Skip if running without process.env.NO_ASYNC */
+	skip_async?: boolean;
 	html?: string;
 	ssrHtml?: string;
 	compileOptions?: Partial<CompileOptions>;
@@ -121,7 +125,15 @@ let console_error = console.error;
 export function runtime_suite(runes: boolean) {
 	return suite_with_variants<RuntimeTest, 'hydrate' | 'ssr' | 'dom', CompileOptions>(
 		['dom', 'hydrate', 'ssr'],
-		(variant, config) => {
+		(variant, config, test_name) => {
+			if (!async_mode && (config.skip_no_async || test_name.startsWith('async-'))) {
+				return true;
+			}
+
+			if (async_mode && config.skip_async) {
+				return true;
+			}
+
 			if (variant === 'hydrate') {
 				if (config.mode && !config.mode.includes('hydrate')) return 'no-test';
 				if (config.skip_mode?.includes('hydrate')) return true;
@@ -169,7 +181,7 @@ async function common_setup(cwd: string, runes: boolean | undefined, config: Run
 		dev: force_hmr ? true : undefined,
 		hmr: force_hmr ? true : undefined,
 		experimental: {
-			async: runes
+			async: runes && async_mode
 		},
 		fragments,
 		...config.compileOptions,
diff --git a/packages/svelte/tests/runtime-runes/samples/effect-cleanup/_config.js b/packages/svelte/tests/runtime-runes/samples/effect-cleanup/_config.js
@@ -1,3 +1,4 @@
+import { async_mode } from '../../../helpers';
 import { test } from '../../test';
 import { flushSync } from 'svelte';
 
@@ -10,6 +11,12 @@ export default test({
 		flushSync(() => {
 			b1.click();
 		});
-		assert.deepEqual(logs, ['init 0']);
+
+		// With async mode (which is on by default for runtime-runes) this works as expected, without it
+		// it works differently: https://github.com/sveltejs/svelte/pull/15564
+		assert.deepEqual(
+			logs,
+			async_mode ? ['init 0', 'cleanup 2', null, 'init 2', 'cleanup 4', null, 'init 4'] : ['init 0']
+		);
 	}
 });
diff --git a/packages/svelte/tests/runtime-runes/samples/effect-cleanup/main.svelte b/packages/svelte/tests/runtime-runes/samples/effect-cleanup/main.svelte
@@ -14,4 +14,4 @@
 	})
 </script>
 
-<button on:click={() => count++ }>Click</button>
+<button onclick={() => count++ }>Click</button>
diff --git a/packages/svelte/tests/runtime-runes/samples/set-context-after-await/_config.js b/packages/svelte/tests/runtime-runes/samples/set-context-after-await/_config.js
@@ -1,6 +1,7 @@
 import { test } from '../../test';
 
 export default test({
+	skip_no_async: true,
 	async test({ assert, logs }) {
 		await Promise.resolve();
 		await Promise.resolve();
diff --git a/packages/svelte/tests/runtime-runes/samples/set-context-after-mount/_config.js b/packages/svelte/tests/runtime-runes/samples/set-context-after-mount/_config.js
@@ -1,11 +1,16 @@
 import { flushSync } from 'svelte';
 import { test } from '../../test';
+import { async_mode } from '../../../helpers';
 
 export default test({
 	async test({ target, assert, logs }) {
 		const button = target.querySelector('button');
 
 		flushSync(() => button?.click());
-		assert.ok(logs[0].startsWith('set_context_after_init'));
+		assert.ok(
+			async_mode
+				? logs[0].startsWith('set_context_after_init')
+				: logs[0] === 'works without experimental async but really shouldnt'
+		);
 	}
 });
diff --git a/packages/svelte/tests/runtime-runes/samples/set-context-after-mount/main.svelte b/packages/svelte/tests/runtime-runes/samples/set-context-after-mount/main.svelte
@@ -7,6 +7,7 @@
 		if (condition) {
 			try {
 				setContext('potato', {});
+				console.log('works without experimental async but really shouldnt')
 			} catch (e) {
 				console.log(e.message);
 			}
diff --git a/packages/svelte/tests/runtime-runes/samples/untrack-own-deriveds/_config.js b/packages/svelte/tests/runtime-runes/samples/untrack-own-deriveds/_config.js
@@ -2,6 +2,8 @@ import { flushSync } from 'svelte';
 import { test } from '../../test';
 
 export default test({
+	// In async mode we _do_ want to run effects that react to their own state changing
+	skip_async: true,
 	test({ assert, target, logs }) {
 		const button = target.querySelector('button');
 
diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts
@@ -16,6 +16,7 @@ import { snapshot } from '../../src/internal/shared/clone.js';
 import { SvelteSet } from '../../src/reactivity/set';
 import { DESTROYED } from '../../src/internal/client/constants';
 import { noop } from 'svelte/internal/client';
+import { disable_async_mode_flag, enable_async_mode_flag } from '../../src/internal/flags';
 
 /**
  * @param runes runes mode
@@ -1010,14 +1011,39 @@ describe('signals', () => {
 		};
 	});
 
-	test('effects do not depend on state they own', () => {
-		user_effect(() => {
-			const value = state(0);
-			set(value, $.get(value) + 1);
+	test('effects do depend on state they own', (runes) => {
+		// This behavior is important for use cases like a Resource class
+		// which shares its instance between multiple effects and triggers
+		// rerenders by self-invalidating its state.
+		const log: number[] = [];
+
+		let count: any;
+
+		if (runes) {
+			// We will make this the new default behavior once it's stable but until then
+			// we need to keep the old behavior to not break existing code.
+			enable_async_mode_flag();
+		}
+
+		effect(() => {
+			if (!count || $.get<number>(count) < 2) {
+				count ||= state(0);
+				log.push($.get(count));
+				set(count, $.get<number>(count) + 1);
+			}
 		});
 
 		return () => {
-			flushSync();
+			try {
+				flushSync();
+				if (runes) {
+					assert.deepEqual(log, [0, 1]);
+				} else {
+					assert.deepEqual(log, [0]);
+				}
+			} finally {
+				disable_async_mode_flag();
+			}
 		};
 	});
 
diff --git a/packages/svelte/tests/suite.ts b/packages/svelte/tests/suite.ts
@@ -35,7 +35,7 @@ export function suite<Test extends BaseTest>(fn: (config: Test, test_dir: string
 
 export function suite_with_variants<Test extends BaseTest, Variants extends string, Common>(
 	variants: Variants[],
-	should_skip_variant: (variant: Variants, config: Test) => boolean | 'no-test',
+	should_skip_variant: (variant: Variants, config: Test, test_name: string) => boolean | 'no-test',
 	common_setup: (config: Test, test_dir: string) => Promise<Common> | Common,
 	fn: (config: Test, test_dir: string, variant: Variants, common: Common) => void
 ) {
@@ -46,11 +46,11 @@ export function suite_with_variants<Test extends BaseTest, Variants extends stri
 				let called_common = false;
 				let common: any = undefined;
 				for (const variant of variants) {
-					if (should_skip_variant(variant, config) === 'no-test') {
+					if (should_skip_variant(variant, config, dir) === 'no-test') {
 						continue;
 					}
 					// TODO unify test interfaces
-					const skip = config.skip || should_skip_variant(variant, config);
+					const skip = config.skip || should_skip_variant(variant, config, dir);
 					const solo = config.solo;
 					let it_fn = skip ? it.skip : solo ? it.only : it;
 

Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,11 @@ export function enable_async_mode_flag() {`
`6`	`6`	`async_mode_flag = true;`
`7`	`7`	`}`
`8`	`8`
	`9`	`+/** ONLY USE THIS DURING TESTING */`
	`10`	`+export function disable_async_mode_flag() {`
	`11`	`+ async_mode_flag = false;`
	`12`	`+}`
	`13`	`+`
`9`	`14`	`export function enable_legacy_mode_flag() {`
`10`	`15`	`legacy_mode_flag = true;`
`11`	`16`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,6 +7,7 @@`
`7`	`7`	`if (condition) {`
`8`	`8`	`try {`
`9`	`9`	`setContext('potato', {});`
	`10`	`+ console.log('works without experimental async but really shouldnt')`
`10`	`11`	`} catch (e) {`
`11`	`12`	`console.log(e.message);`
`12`	`13`	`}`