fix: ensure effects destroy owned deriveds upon teardown (#13563)

* fix: ensure effects destroy owned deriveds upon teardown

* add test

* make old test work

* tune

* tune

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

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
pull/13545/head
Dominic Gannaway 11 months ago committed by GitHub
parent 6534f507ce
commit 7e6d93d1c3
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: ensure effects destroy owned deriveds upon teardown

@ -58,6 +58,9 @@ export function derived(fn) {
var derived = /** @type {Derived} */ (active_reaction);
(derived.children ??= []).push(signal);
}
if (active_effect !== null) {
(active_effect.deriveds ??= []).push(signal);
}
return signal;
}
@ -103,10 +106,11 @@ function destroy_derived_children(derived) {
let stack = [];
/**
* @template T
* @param {Derived} derived
* @returns {void}
* @returns {T}
*/
export function update_derived(derived) {
export function execute_derived(derived) {
var value;
var prev_active_effect = active_effect;
@ -138,6 +142,15 @@ export function update_derived(derived) {
}
}
return value;
}
/**
* @param {Derived} derived
* @returns {void}
*/
export function update_derived(derived) {
var value = execute_derived(derived);
var status =
(skip_reaction || (derived.f & UNOWNED) !== 0) && derived.deps !== null ? MAYBE_DIRTY : CLEAN;
@ -153,17 +166,11 @@ export function update_derived(derived) {
* @param {Derived} signal
* @returns {void}
*/
function destroy_derived(signal) {
export function destroy_derived(signal) {
destroy_derived_children(signal);
remove_reactions(signal, 0);
set_signal_status(signal, DESTROYED);
// TODO we need to ensure we remove the derived from any parent derives
signal.children =
signal.deps =
signal.reactions =
// @ts-expect-error `signal.fn` cannot be `null` while the signal is alive
signal.fn =
null;
signal.v = signal.children = signal.deps = signal.reactions = null;
}

@ -97,6 +97,7 @@ function create_effect(type, fn, sync, push = true) {
var effect = {
ctx: component_context,
deps: null,
deriveds: null,
nodes_start: null,
nodes_end: null,
f: type | DIRTY,

@ -10,10 +10,17 @@ import {
import { get_descriptor, is_function } from '../../shared/utils.js';
import { mutable_source, set, source } from './sources.js';
import { derived, derived_safe_equal } from './deriveds.js';
import { get, is_signals_recorded, untrack, update } from '../runtime.js';
import {
active_effect,
get,
is_signals_recorded,
set_active_effect,
untrack,
update
} from '../runtime.js';
import { safe_equals } from './equality.js';
import * as e from '../errors.js';
import { LEGACY_DERIVED_PROP } from '../constants.js';
import { BRANCH_EFFECT, LEGACY_DERIVED_PROP, ROOT_EFFECT } from '../constants.js';
import { proxy } from '../proxy.js';
/**
@ -217,6 +224,26 @@ export function spread_props(...props) {
return new Proxy({ props }, spread_props_handler);
}
/**
* @template T
* @param {() => T} fn
* @returns {T}
*/
function with_parent_branch(fn) {
var effect = active_effect;
var previous_effect = active_effect;
while (effect !== null && (effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0) {
effect = effect.parent;
}
try {
set_active_effect(effect);
return fn();
} finally {
set_active_effect(previous_effect);
}
}
/**
* This function is responsible for synchronizing a possibly bound prop with the inner component state.
* It is used whenever the compiler sees that the component writes to the prop, or when it has a default prop_value.
@ -276,8 +303,8 @@ export function prop(props, key, flags, fallback) {
} else {
// Svelte 4 did not trigger updates when a primitive value was updated to the same value.
// Replicate that behavior through using a derived
var derived_getter = (immutable ? derived : derived_safe_equal)(
() => /** @type {V} */ (props[key])
var derived_getter = with_parent_branch(() =>
(immutable ? derived : derived_safe_equal)(() => /** @type {V} */ (props[key]))
);
derived_getter.f |= LEGACY_DERIVED_PROP;
getter = () => {
@ -321,19 +348,21 @@ export function prop(props, key, flags, fallback) {
// The derived returns the current value. The underlying mutable
// source is written to from various places to persist this value.
var inner_current_value = mutable_source(prop_value);
var current_value = derived(() => {
var parent_value = getter();
var child_value = get(inner_current_value);
if (from_child) {
from_child = false;
was_from_child = true;
return child_value;
}
var current_value = with_parent_branch(() =>
derived(() => {
var parent_value = getter();
var child_value = get(inner_current_value);
if (from_child) {
from_child = false;
was_from_child = true;
return child_value;
}
was_from_child = false;
return (inner_current_value.v = parent_value);
});
was_from_child = false;
return (inner_current_value.v = parent_value);
})
);
if (!immutable) current_value.equals = safe_equals;

@ -42,6 +42,8 @@ export interface Effect extends Reaction {
nodes_end: null | TemplateNode;
/** The associated component context */
ctx: null | ComponentContext;
/** Reactions created inside this signal */
deriveds: null | Derived[];
/** The effect function */
fn: null | (() => void | (() => void));
/** The teardown function returned from the effect function */

@ -27,7 +27,7 @@ import {
import { flush_tasks } from './dom/task.js';
import { add_owner } from './dev/ownership.js';
import { mutate, set, source } from './reactivity/sources.js';
import { update_derived } from './reactivity/deriveds.js';
import { destroy_derived, execute_derived, update_derived } from './reactivity/deriveds.js';
import * as e from './errors.js';
import { lifecycle_outside_component } from '../shared/errors.js';
import { FILENAME } from '../../constants.js';
@ -300,12 +300,13 @@ export function update_reaction(reaction) {
var previous_reaction = active_reaction;
var previous_skip_reaction = skip_reaction;
var prev_derived_sources = derived_sources;
var flags = reaction.f;
new_deps = /** @type {null | Value[]} */ (null);
skipped_deps = 0;
untracked_writes = null;
active_reaction = (reaction.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null;
skip_reaction = !is_flushing_effect && (reaction.f & UNOWNED) !== 0;
active_reaction = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null;
skip_reaction = !is_flushing_effect && (flags & UNOWNED) !== 0;
derived_sources = null;
try {
@ -408,6 +409,16 @@ export function remove_reactions(signal, start_index) {
* @returns {void}
*/
export function destroy_effect_children(signal, remove_dom = false) {
var deriveds = signal.deriveds;
if (deriveds !== null) {
signal.deriveds = null;
for (var i = 0; i < deriveds.length; i += 1) {
destroy_derived(deriveds[i]);
}
}
var effect = signal.first;
signal.first = signal.last = null;
@ -729,9 +740,15 @@ export async function tick() {
*/
export function get(signal) {
var flags = signal.f;
if ((flags & DESTROYED) !== 0) {
return signal.v;
var is_derived = (flags & DERIVED) !== 0;
// If the derived is destroyed, just execute it again without retaining
// its memoisation properties as the derived is stale
if (is_derived && (flags & DESTROYED) !== 0) {
var value = execute_derived(/** @type {Derived} */ (signal));
// Ensure the derived remains destroyed
destroy_derived(/** @type {Derived} */ (signal));
return value;
}
if (is_signals_recorded) {
@ -768,7 +785,7 @@ export function get(signal) {
}
}
if ((flags & DERIVED) !== 0) {
if (is_derived) {
var derived = /** @type {Derived} */ (signal);
if (check_dirtiness(derived)) {

@ -1,6 +1,4 @@
<script>
import { untrack } from 'svelte';
class Model {
data = $state();
@ -28,9 +26,7 @@
$effect(() => {
if(needsSet) {
setModel('effect');
untrack(() => {
needsSet = false;
});
needsSet = false;
}
});

@ -488,7 +488,7 @@ describe('signals', () => {
assert.equal(a?.deps?.length, 1);
assert.equal(s?.reactions?.length, 1);
destroy();
assert.equal(a?.deps?.length, 1);
assert.equal(a?.deps, null);
assert.equal(s?.reactions, null);
};
});
@ -725,4 +725,19 @@ describe('signals', () => {
assert.equal($.get(d), true);
};
});
test('deriveds read inside the root/branches are cleaned up', () => {
return () => {
const a = state(0);
const destroy = effect_root(() => {
const b = derived(() => $.get(a));
$.get(b);
});
destroy();
assert.deepEqual(a.reactions, null);
};
});
});

Loading…
Cancel
Save