fix: avoid false positives for reactivity loss warning (#18088)

Avoids two categories of false positives for reactivity loss warning:
1. if you have synchronously read signals already as part of invoking
the async_derived function, then it shouldn't warn when these signals
are read after an await if we know they haven't changed (we check the
write version for that)
2. `track_reactivity_loss` kept the `reactivity_loss_tracker` around
indefinitely, both when invoking the async operation as well as when
it's finished. The former is buggy because while the async operation
happens unrelated reads as part of other reactivity work can happen, the
latter is buggy because if it's the last in a chain of awaits it's kept
around until the next async work starts.

Fixes https://github.com/sveltejs/kit/issues/15654

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
pull/18085/head
Simon H 3 months ago committed by GitHub
parent 0e9e76f292
commit 15588f5fbf
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: avoid false positives for reactivity loss warning

@ -164,10 +164,26 @@ export async function save(promise) {
*/
export async function track_reactivity_loss(promise) {
var previous_async_effect = reactivity_loss_tracker;
// Ensure that unrelated reads after an async operation is kicked off don't cause false positives
queueMicrotask(() => {
if (reactivity_loss_tracker === previous_async_effect) {
set_reactivity_loss_tracker(null);
}
});
var value = await promise;
return () => {
set_reactivity_loss_tracker(previous_async_effect);
// While this can result in false negatives it also guards against the more important
// false positives that would occur if this is the last in a chain of async operations,
// and the reactivity_loss_tracker would then stay around until the next async operation happens.
queueMicrotask(() => {
if (reactivity_loss_tracker === previous_async_effect) {
set_reactivity_loss_tracker(null);
}
});
return value;
};
}
@ -206,7 +222,9 @@ export async function* for_await_track_reactivity_loss(iterable) {
normal_completion = true;
break;
}
var prev = reactivity_loss_tracker;
yield value;
set_reactivity_loss_tracker(prev);
}
} finally {
// If the iterator had an abrupt completion and `return` is defined on the iterator, call it and return the value

@ -1,4 +1,4 @@
/** @import { Derived, Effect, Source } from '#client' */
/** @import { Derived, Effect, Reaction, Source, Value } from '#client' */
/** @import { Batch } from './batch.js'; */
/** @import { Boundary } from '../dom/blocks/boundary.js'; */
import { DEV } from 'esm-env';
@ -24,7 +24,9 @@ import {
push_reaction_value,
is_destroying_effect,
update_effect,
remove_reactions
remove_reactions,
skipped_deps,
new_deps
} from '../runtime.js';
import { equals, safe_equals } from './equality.js';
import * as e from '../errors.js';
@ -49,11 +51,11 @@ import { set_signal_status, update_derived_status } from './status.js';
/**
* 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}
* @type {{ effect: Effect, effect_deps: Set<Value>, warned: boolean } | null}
*/
export let reactivity_loss_tracker = null;
/** @param {{ effect: Effect, warned: boolean } | null} v */
/** @param {{ effect: Effect, effect_deps: Set<Value>, warned: boolean } | null} v */
export function set_reactivity_loss_tracker(v) {
reactivity_loss_tracker = v;
}
@ -125,15 +127,12 @@ export function async_derived(fn, label, location) {
var deferreds = new Map();
async_effect(() => {
var effect = /** @type {Effect} */ (active_effect);
if (DEV) {
reactivity_loss_tracker = {
effect: /** @type {Effect} */ (active_effect),
warned: false
};
reactivity_loss_tracker = { effect, effect_deps: new Set(), warned: false };
}
var effect = /** @type {Effect} */ (active_effect);
/** @type {ReturnType<typeof deferred<V>>} */
var d = deferred();
promise = d.promise;
@ -149,6 +148,24 @@ export function async_derived(fn, label, location) {
}
if (DEV) {
if (reactivity_loss_tracker) {
// Reused deps from previous run (indices 0 to skipped_deps-1)
// We deliberately only track direct dependencies of the async expression to encourage
// dependencies being directly visible at the point of the expression
if (effect.deps !== null) {
for (let i = 0; i < skipped_deps; i += 1) {
reactivity_loss_tracker.effect_deps.add(effect.deps[i]);
}
}
// New deps discovered this run
if (new_deps !== null) {
for (let i = 0; i < new_deps.length; i += 1) {
reactivity_loss_tracker.effect_deps.add(new_deps[i]);
}
}
}
reactivity_loss_tracker = null;
}

@ -111,9 +111,9 @@ export function push_reaction_value(value) {
* and until a new dependency is accessed we track this via `skipped_deps`
* @type {null | Value[]}
*/
let new_deps = null;
export let new_deps = null;
let skipped_deps = 0;
export let skipped_deps = 0;
/**
* Tracks writes that the effect it's executed in doesn't listen to yet,
@ -580,7 +580,8 @@ export function get(signal) {
!untracking &&
reactivity_loss_tracker &&
!reactivity_loss_tracker.warned &&
(reactivity_loss_tracker.effect.f & REACTION_IS_UPDATING) === 0
(reactivity_loss_tracker.effect.f & REACTION_IS_UPDATING) === 0 &&
!reactivity_loss_tracker.effect_deps.has(signal)
) {
reactivity_loss_tracker.warned = true;

@ -0,0 +1,27 @@
import { tick } from 'svelte';
import { test } from '../../test';
import { normalise_trace_logs } from '../../../helpers.js';
export default test({
compileOptions: {
dev: true
},
async test({ assert, target, warnings }) {
await new Promise((r) => setTimeout(r, 25));
const [count] = target.querySelectorAll('button');
count.click();
await new Promise((r) => setTimeout(r, 25));
assert.deepEqual(normalise_trace_logs(warnings), [
{
log: 'Detected reactivity loss when reading `other`. This happens when state is read in an async function after an earlier `await`'
},
{
log: 'Detected reactivity loss when reading `other`. This happens when state is read in an async function after an earlier `await`'
}
]);
}
});

@ -0,0 +1,27 @@
<script>
let count = $state(0);
let other = $state(0);
function delayed(value, ms = 1000) {
return new Promise((f) => setTimeout(() => f(value), ms))
}
async function foo() {
await new Promise(r => setTimeout(r, 10));
}
async function bar() {
const value = await delayed(count, 10);
other; // should trigger warning
return value;
}
async function get() {
foo();
return await bar();
}
</script>
<button onclick={() => count++}>{count}</button>
<button onclick={() => other++}>{other}</button>
{await get()}

@ -14,7 +14,7 @@ export default test({
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`'
log: 'Detected reactivity loss when reading `values[1]`. This happens when state is read in an async function after an earlier `await`'
}
]);
}

@ -15,7 +15,7 @@ export default test({
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`'
log: 'Detected reactivity loss when reading `values[1]`. This happens when state is read in an async function after an earlier `await`'
}
]);
}

@ -0,0 +1,21 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
compileOptions: {
dev: true
},
async test({ assert, target, warnings }) {
await tick();
const [x, y] = target.querySelectorAll('button');
y.click();
await tick();
x.click();
await new Promise((r) => setTimeout(r, 15));
assert.equal(warnings.length, 0);
}
});

@ -0,0 +1,16 @@
<script>
let x = $state(0);
let y = $state(0);
async function foo(x) {
if (x) {
await 1; // restores reactivity loss warning context
await new Promise(r => setTimeout(r, 10)) // saves reactivity loss warning context; should not keep it while running
}
return x
}
</script>
{x} {await foo(y)}
<button onclick={() => x += 1}>x++</button>
<button onclick={() => y += 1}>y++</button>

@ -0,0 +1,19 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
compileOptions: {
dev: true
},
async test({ assert, target, warnings }) {
await tick();
const [count] = target.querySelectorAll('button');
count.click();
await tick();
assert.equal(warnings.length, 0);
}
});

@ -0,0 +1,14 @@
<script>
let count = $state(0);
async function delay(c) {
if (c) {
await new Promise(r => setTimeout(r));
count; // count already read synchronously; should not result in reacitive loss warning
}
return c;
}
</script>
{await delay(count)}
<button onclick={() => count += 1}>count++</button>

@ -0,0 +1,19 @@
import { tick } from 'svelte';
import { test } from '../../test';
export default test({
compileOptions: {
dev: true
},
async test({ assert, target, warnings }) {
await new Promise((r) => setTimeout(r, 5));
const [count] = target.querySelectorAll('button');
count.click();
await tick();
assert.equal(warnings.length, 0);
}
});

@ -0,0 +1,14 @@
<script>
let count = $state(0);
async function run() {
await new Promise(r => setTimeout(r));
}
async function get() {
run();
return 1;
}
</script>
<button onclick={() => count++}>{count}</button>
{await get()}
Loading…
Cancel
Save