fix: reinstate reactivity loss tracking (#17801)

We commented out this code in #17038 because it was broken. I suspect it
was broken because we weren't correctly calling `unset_context` inside
`run`, leading to false positives — this is now fixed, and as such I
_think_ we can safely reinstate it.

One small change — I got rid of the `was_read` check. I assume this
existed to prevent duplicate warnings, but it actually causes false
negatives in the case where you read a signal while the reaction is
being tracked then again while it's untracked.

### Before submitting the PR, please make sure you do the following

- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [ ] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`

---------

Co-authored-by: Tee Ming <chewteeming01@gmail.com>

.changeset/twelve-stars-serve.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: reinstate reactivity loss tracking

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

@@ -19,10 +19,10 @@ import {
 import { Batch, current_batch } from './batch.js';
 import {
 	async_derived,
-	current_async_effect,
+	reactivity_loss_tracker,
 	derived,
 	derived_safe_equal,
-	set_from_async_derived
+	set_reactivity_loss_tracker
 } from './deriveds.js';
 import { aborted } from './effects.js';
 
@@ -131,7 +131,7 @@ export function capture() {
 		}
 
 		if (DEV) {
-			set_from_async_derived(null);
+			set_reactivity_loss_tracker(null);
 			set_dev_stack(previous_dev_stack);
 		}
 	};
@@ -163,11 +163,11 @@ export async function save(promise) {
  * @returns {Promise<() => T>}
  */
 export async function track_reactivity_loss(promise) {
-	var previous_async_effect = current_async_effect;
+	var previous_async_effect = reactivity_loss_tracker;
 	var value = await promise;
 
 	return () => {
-		set_from_async_derived(previous_async_effect);
+		set_reactivity_loss_tracker(previous_async_effect);
 		return value;
 	};
 }
@@ -224,7 +224,7 @@ export function unset_context(deactivate_batch = true) {
 	if (deactivate_batch) current_batch?.deactivate();
 
 	if (DEV) {
-		set_from_async_derived(null);
+		set_reactivity_loss_tracker(null);
 		set_dev_stack(null);
 	}
 }

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

@@ -45,12 +45,16 @@ import { increment_pending, unset_context } from './async.js';
 import { deferred, includes, noop } from '../../shared/utils.js';
 import { set_signal_status, update_derived_status } from './status.js';
 
-/** @type {Effect | null} */
-export let current_async_effect = null;
+/**
+ * This allows us to track 'reactivity loss' that occurs when signals
+ * are read after a non-context-restoring `await`. Dev-only
+ * @type {{ effect: Effect, warned: boolean } | null}
+ */
+export let reactivity_loss_tracker = null;
 
-/** @param {Effect | null} v */
-export function set_from_async_derived(v) {
-	current_async_effect = v;
+/** @param {{ effect: Effect, warned: boolean } | null} v */
+export function set_reactivity_loss_tracker(v) {
+	reactivity_loss_tracker = v;
 }
 
 export const recent_async_deriveds = new Set();
@@ -124,7 +128,12 @@ export function async_derived(fn, label, location) {
 	var deferreds = new Map();
 
 	async_effect(() => {
-		if (DEV) current_async_effect = active_effect;
+		if (DEV) {
+			reactivity_loss_tracker = {
+				effect: /** @type {Effect} */ (active_effect),
+				warned: false
+			};
+		}
 
 		var effect = /** @type {Effect} */ (active_effect);
 
@@ -142,7 +151,9 @@ export function async_derived(fn, label, location) {
 			unset_context();
 		}
 
-		if (DEV) current_async_effect = null;
+		if (DEV) {
+			reactivity_loss_tracker = null;
+		}
 
 		var batch = /** @type {Batch} */ (current_batch);
 
@@ -174,7 +185,9 @@ export function async_derived(fn, label, location) {
 		 * @param {unknown} error
 		 */
 		const handler = (value, error = undefined) => {
-			if (DEV) current_async_effect = null;
+			if (DEV) {
+				reactivity_loss_tracker = null;
+			}
 
 			if (decrement_pending) {
 				// don't trigger an update if we're only here because

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

@@ -27,7 +27,7 @@ import {
 } from './constants.js';
 import { old_values } from './reactivity/sources.js';
 import {
-	destroy_derived_effects,
+	reactivity_loss_tracker,
 	execute_derived,
 	freeze_derived_effects,
 	recent_async_deriveds,
@@ -58,6 +58,7 @@ import { UNINITIALIZED } from '../../constants.js';
 import { captured_signals } from './legacy.js';
 import { without_reactive_context } from './dom/elements/bindings/shared.js';
 import { set_signal_status, update_derived_status } from './reactivity/status.js';
+import * as w from './warnings.js';
 
 let is_updating_effect = false;
 
@@ -568,19 +569,20 @@ export function get(signal) {
 	}
 
 	if (DEV) {
-		// TODO reinstate this, but make it actually work
-		// if (current_async_effect) {
-		// 	var tracking = (current_async_effect.f & REACTION_IS_UPDATING) !== 0;
-		// 	var was_read = current_async_effect.deps?.includes(signal);
+		if (
+			!untracking &&
+			reactivity_loss_tracker &&
+			!reactivity_loss_tracker.warned &&
+			(reactivity_loss_tracker.effect.f & REACTION_IS_UPDATING) === 0
+		) {
+			reactivity_loss_tracker.warned = true;
 
-		// 	if (!tracking && !untracking && !was_read) {
-		// 		w.await_reactivity_loss(/** @type {string} */ (signal.label));
+			w.await_reactivity_loss(/** @type {string} */ (signal.label));
 
-		// 		var trace = get_error('traced at');
-		// 		// eslint-disable-next-line no-console
-		// 		if (trace) console.warn(trace);
-		// 	}
-		// }
+			var trace = get_error('traced at');
+			// eslint-disable-next-line no-console
+			if (trace) console.warn(trace);
+		}
 
 		recent_async_deriveds.delete(signal);
 
@@ -595,7 +597,7 @@ export function get(signal) {
 			if (signal.trace) {
 				signal.trace();
 			} else {
-				var trace = get_error('traced at');
+				trace = get_error('traced at');
 
 				if (trace) {
 					var entry = tracing_expressions.entries.get(signal);

packages/svelte/tests/runtime-runes/samples/async-reactivity-loss-for-await/_config.js

@@ -1,10 +1,8 @@
 import { tick } from 'svelte';
 import { test } from '../../test';
+import { normalise_trace_logs } from '../../../helpers.js';
 
 export default test({
-	// TODO reinstate
-	skip: true,
-
 	compileOptions: {
 		dev: true
 	},
@@ -15,13 +13,10 @@ export default test({
 		await tick();
 		assert.htmlEqual(target.innerHTML, '<button>a</button><button>b</button><h1>3</h1>');
 
-		assert.equal(
-			warnings[0],
-			'Detected reactivity loss when reading `values[1]`. This happens when state is read in an async function after an earlier `await`'
-		);
-
-		assert.equal(warnings[1].name, 'traced at');
-
-		assert.equal(warnings.length, 2);
+		assert.deepEqual(normalise_trace_logs(warnings), [
+			{
+				log: 'Detected reactivity loss when reading `values.length`. This happens when state is read in an async function after an earlier `await`'
+			}
+		]);
 	}
 });

packages/svelte/tests/runtime-runes/samples/async-reactivity-loss/_config.js

@@ -2,9 +2,6 @@ import { tick } from 'svelte';
 import { test } from '../../test';
 
 export default test({
-	// TODO reinstate this
-	skip: true,
-
 	compileOptions: {
 		dev: true
 	},