fix: freeze effects-inside-deriveds when disconnecting, unfreeze on reconnect (#17682)

This supersedes #16595, and fixes the issue by 'freezing' effects inside
deriveds when those deriveds are disconnected, and unfreezing them when
they reconnect. This is preferable to the current asymmetric behaviour
on `main` (in which effects are destroyed when the derived is
disconnected, and never recreated) and #16595, which causes the derived
itself to be re-evaluated.

### 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.
- [x] 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`
pull/17690/head
Rich Harris 3 days ago committed by GitHub
parent 9400689e1c
commit 4eee2f7c92
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: freeze effects-inside-deriveds when disconnecting, unfreeze on reconnect

@ -27,10 +27,10 @@ export const DIRTY = 1 << 11;
export const MAYBE_DIRTY = 1 << 12;
export const INERT = 1 << 13;
export const DESTROYED = 1 << 14;
/** Set once a reaction has run for the first time */
export const REACTION_RAN = 1 << 15;
// Flags exclusive to effects
/** Set once an effect that should run synchronously has run */
export const EFFECT_RAN = 1 << 15;
/**
* 'Transparent' effects do not create a transition boundary.
* This is on a block effect 99% of the time but may also be on a branch effect if its parent block effect was pruned
@ -48,7 +48,7 @@ export const EFFECT_OFFSCREEN = 1 << 25;
* Will be lifted during execution of the derived and during checking its dirty state (both are necessary
* because a derived might be checked but not executed).
*/
export const WAS_MARKED = 1 << 15;
export const WAS_MARKED = 1 << 16;
// Flags used for async
export const REACTION_IS_UPDATING = 1 << 21;

@ -5,7 +5,7 @@ import { active_effect, active_reaction } from './runtime.js';
import { create_user_effect } from './reactivity/effects.js';
import { async_mode_flag, legacy_mode_flag } from '../flags/index.js';
import { FILENAME } from '../../constants.js';
import { BRANCH_EFFECT, EFFECT_RAN } from './constants.js';
import { BRANCH_EFFECT, REACTION_RAN } from './constants.js';
/** @type {ComponentContext | null} */
export let component_context = null;

@ -5,7 +5,7 @@ import { active_effect, untrack } from '../../runtime.js';
import { loop } from '../../loop.js';
import { should_intro } from '../../render.js';
import { TRANSITION_GLOBAL, TRANSITION_IN, TRANSITION_OUT } from '../../../../constants.js';
import { BLOCK_EFFECT, EFFECT_RAN, EFFECT_TRANSPARENT } from '#client/constants';
import { BLOCK_EFFECT, REACTION_RAN, EFFECT_TRANSPARENT } from '#client/constants';
import { queue_micro_task } from '../task.js';
import { without_reactive_context } from './bindings/shared.js';
@ -289,7 +289,7 @@ export function transition(flags, element, get_fn, get_params) {
}
}
run = !block || (block.f & EFFECT_RAN) !== 0;
run = !block || (block.f & REACTION_RAN) !== 0;
}
if (run) {

@ -5,7 +5,7 @@ import { init_array_prototype_warnings } from '../dev/equality.js';
import { get_descriptor, is_extensible } from '../../shared/utils.js';
import { active_effect } from '../runtime.js';
import { async_mode_flag } from '../../flags/index.js';
import { TEXT_NODE, EFFECT_RAN } from '#client/constants';
import { TEXT_NODE, REACTION_RAN } from '#client/constants';
import { eager_block_effects } from '../reactivity/batch.js';
import { NAMESPACE_HTML } from '../../../constants.js';
@ -228,7 +228,7 @@ export function should_defer_append() {
if (eager_block_effects !== null) return false;
var flags = /** @type {Effect} */ (active_effect).f;
return (flags & EFFECT_RAN) !== 0;
return (flags & REACTION_RAN) !== 0;
}
/**

@ -22,7 +22,7 @@ import {
TEMPLATE_USE_MATHML,
TEMPLATE_USE_SVG
} from '../../../constants.js';
import { COMMENT_NODE, DOCUMENT_FRAGMENT_NODE, EFFECT_RAN, TEXT_NODE } from '#client/constants';
import { COMMENT_NODE, DOCUMENT_FRAGMENT_NODE, REACTION_RAN, TEXT_NODE } from '#client/constants';
/**
* @param {TemplateNode} start
@ -353,7 +353,7 @@ export function append(anchor, dom) {
// When hydrating and outer component and an inner component is async, i.e. blocked on a promise,
// then by the time the inner resolves we have already advanced to the end of the hydrated nodes
// of the parent component. Check for defined for that reason to avoid rewinding the parent's end marker.
if ((effect.f & EFFECT_RAN) === 0 || effect.nodes.end === null) {
if ((effect.f & REACTION_RAN) === 0 || effect.nodes.end === null) {
effect.nodes.end = hydrate_node;
}

@ -3,7 +3,7 @@
import { DEV } from 'esm-env';
import { FILENAME } from '../../constants.js';
import { is_firefox } from './dom/operations.js';
import { ERROR_VALUE, BOUNDARY_EFFECT, EFFECT_RAN } from './constants.js';
import { ERROR_VALUE, BOUNDARY_EFFECT, REACTION_RAN } from './constants.js';
import { define_property, get_descriptor } from '../shared/utils.js';
import { active_effect, active_reaction } from './runtime.js';
@ -25,7 +25,7 @@ export function handle_error(error) {
adjustments.set(error, get_adjustments(error, effect));
}
if ((effect.f & EFFECT_RAN) === 0) {
if ((effect.f & REACTION_RAN) === 0) {
// if the error occurred while creating this subtree, we let it
// bubble up until it hits a boundary that can handle it
if ((effect.f & BOUNDARY_EFFECT) === 0) {

@ -701,16 +701,15 @@ function flush_queued_effects(effects) {
// don't know if we need to keep them until they are executed. Doing the check
// here (rather than in `update_effect`) allows us to skip the work for
// immediate effects.
if (effect.deps === null && effect.first === null && effect.nodes === null) {
// if there's no teardown or abort controller we completely unlink
// the effect from the graph
if (effect.teardown === null && effect.ac === null) {
// remove this effect from the graph
unlink_effect(effect);
} else {
// keep the effect in the graph, but free up some memory
effect.fn = null;
}
if (
effect.deps === null &&
effect.first === null &&
effect.nodes === null &&
effect.teardown === null &&
effect.ac === null
) {
// remove this effect from the graph
unlink_effect(effect);
}
// If update_effect() has a flushSync() in it, we may have flushed another flush_queued_effects(),

@ -19,12 +19,20 @@ import {
increment_write_version,
set_active_effect,
push_reaction_value,
is_destroying_effect
is_destroying_effect,
update_effect,
remove_reactions
} from '../runtime.js';
import { equals, safe_equals } from './equality.js';
import * as e from '../errors.js';
import * as w from '../warnings.js';
import { async_effect, destroy_effect, effect_tracking, teardown } from './effects.js';
import {
async_effect,
destroy_effect,
destroy_effect_children,
effect_tracking,
teardown
} from './effects.js';
import { eager_effects, internal_set, set_eager_effects, source } from './sources.js';
import { get_error } from '../../shared/dev.js';
import { async_mode_flag, tracing_mode_flag } from '../../flags/index.js';
@ -33,7 +41,7 @@ import { component_context } from '../context.js';
import { UNINITIALIZED } from '../../../constants.js';
import { batch_values, current_batch } from './batch.js';
import { unset_context } from './async.js';
import { deferred, includes } from '../../shared/utils.js';
import { deferred, includes, noop } from '../../shared/utils.js';
import { set_signal_status, update_derived_status } from './status.js';
/** @type {Effect | null} */
@ -392,3 +400,43 @@ export function update_derived(derived) {
update_derived_status(derived);
}
}
/**
* @param {Derived} derived
*/
export function freeze_derived_effects(derived) {
if (derived.effects === null) return;
for (const e of derived.effects) {
// if the effect has a teardown function or abort signal, call it
if (e.teardown || e.ac) {
e.teardown?.();
e.ac?.abort(STALE_REACTION);
// make it a noop so it doesn't get called again if the derived
// is unfrozen. we don't set it to `null`, because the existence
// of a teardown function is what determines whether the
// effect runs again during unfreezing
e.teardown = noop;
e.ac = null;
remove_reactions(e, 0);
destroy_effect_children(e);
}
}
}
/**
* @param {Derived} derived
*/
export function unfreeze_derived_effects(derived) {
if (derived.effects === null) return;
for (const e of derived.effects) {
// if the effect was previously frozen — indicated by the presence
// of a teardown function — unfreeze it
if (e.teardown) {
update_effect(e);
}
}
}

@ -19,7 +19,7 @@ import {
EFFECT,
DESTROYED,
INERT,
EFFECT_RAN,
REACTION_RAN,
BLOCK_EFFECT,
ROOT_EFFECT,
EFFECT_TRANSPARENT,
@ -122,7 +122,6 @@ function create_effect(type, fn, sync) {
if (sync) {
try {
update_effect(effect);
effect.f |= EFFECT_RAN;
} catch (e) {
destroy_effect(effect);
throw e;
@ -206,7 +205,7 @@ export function user_effect(fn) {
// Non-nested `$effect(...)` in a component should be deferred
// until the component is mounted
var flags = /** @type {Effect} */ (active_effect).f;
var defer = !active_reaction && (flags & BRANCH_EFFECT) !== 0 && (flags & EFFECT_RAN) === 0;
var defer = !active_reaction && (flags & BRANCH_EFFECT) !== 0 && (flags & REACTION_RAN) === 0;
if (defer) {
// Top-level `$effect(...)` in an unmounted component — defer until mount

@ -22,13 +22,16 @@ import {
STALE_REACTION,
ERROR_VALUE,
WAS_MARKED,
MANAGED_EFFECT
MANAGED_EFFECT,
REACTION_RAN
} from './constants.js';
import { old_values } from './reactivity/sources.js';
import {
destroy_derived_effects,
execute_derived,
freeze_derived_effects,
recent_async_deriveds,
unfreeze_derived_effects,
update_derived
} from './reactivity/deriveds.js';
import { async_mode_flag, tracing_mode_flag } from '../flags/index.js';
@ -253,6 +256,7 @@ export function update_reaction(reaction) {
reaction.f |= REACTION_IS_UPDATING;
var fn = /** @type {Function} */ (reaction.fn);
var result = fn();
reaction.f |= REACTION_RAN;
var deps = reaction.deps;
// Don't remove reactions during fork;
@ -396,8 +400,10 @@ function remove_reaction(signal, dependency) {
update_derived_status(derived);
// freeze any effects inside this derived
freeze_derived_effects(derived);
// Disconnect any reactions owned by this reaction
destroy_derived_effects(derived);
remove_reactions(derived, 0);
}
}
@ -643,7 +649,7 @@ export function get(signal) {
active_reaction !== null &&
(is_updating_effect || (active_reaction.f & CONNECTED) !== 0);
var is_new = derived.deps === null;
var is_new = (derived.f & REACTION_RAN) === 0;
if (is_dirty(derived)) {
if (should_connect) {
@ -656,6 +662,7 @@ export function get(signal) {
}
if (should_connect && !is_new) {
unfreeze_derived_effects(derived);
reconnect(derived);
}
}
@ -677,14 +684,15 @@ export function get(signal) {
* @param {Derived} derived
*/
function reconnect(derived) {
if (derived.deps === null) return;
derived.f |= CONNECTED;
if (derived.deps === null) return;
for (const dep of derived.deps) {
(dep.reactions ??= []).push(derived);
if ((dep.f & DERIVED) !== 0 && (dep.f & CONNECTED) === 0) {
unfreeze_derived_effects(/** @type {Derived} */ (dep));
reconnect(/** @type {Derived} */ (dep));
}
}

@ -0,0 +1,42 @@
import { flushSync } from 'svelte';
import { test } from '../../test';
export default test({
test({ assert, target, logs }) {
const [toggle, run] = target.querySelectorAll('button');
assert.htmlEqual(
target.innerHTML,
'<button>toggle</button><button>run</button><p>hello: 0</p>'
);
flushSync(() => run.click());
assert.deepEqual(logs, []);
assert.htmlEqual(
target.innerHTML,
'<button>toggle</button><button>run</button><p>hello: 1</p>'
);
flushSync(() => toggle.click());
assert.deepEqual(logs, ['aborted']);
assert.htmlEqual(target.innerHTML, '<button>toggle</button><button>run</button>');
flushSync(() => run.click());
flushSync(() => run.click());
flushSync(() => run.click());
flushSync(() => toggle.click());
assert.htmlEqual(
target.innerHTML,
'<button>toggle</button><button>run</button><p>hello: 1</p>'
);
flushSync(() => run.click());
assert.htmlEqual(
target.innerHTML,
'<button>toggle</button><button>run</button><p>hello: 2</p>'
);
assert.deepEqual(logs, ['aborted']);
}
});

@ -0,0 +1,53 @@
<script>
import { getAbortSignal } from 'svelte';
const callbacks = new Map();
// similar semantics to setInterval, but simpler to test
function add(fn) {
const id = crypto.randomUUID();
callbacks.set(id, fn);
return id;
}
function remove(id) {
callbacks.delete(id);
}
function run() {
for (const fn of callbacks.values()) {
fn();
}
}
class Timer {
constructor(text) {
this.elapsed = $state(0);
this.text = $derived(text + ': ' + this.elapsed);
$effect(() => {
const id = add(() => {
this.elapsed += 1;
});
getAbortSignal().onabort = () => {
console.log('aborted');
};
return () => remove(id);
});
}
}
let timer = $derived(new Timer('hello'));
let visible = $state(true);
</script>
<button onclick={() => visible = !visible}>toggle</button>
<button onclick={run}>run</button>
{#if visible}
<p>{timer.text}</p>
{/if}
Loading…
Cancel
Save