fix: improve derived connection to ownership graph (#15137)

* fix: improve derived connection to ownership graph * revised * revised * revised * revised * feedback * feedback * invasive change * fix bugs * fix other bug
sveltejs · Jan 29, 2025 · 13a6d55 · 13a6d55
1 parent 9410ad0
commit 13a6d55
Show file tree

Hide file tree

Showing 6 changed files with 199 additions and 123 deletions.
diff --git a/.changeset/blue-rocks-play.md b/.changeset/blue-rocks-play.md
@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: improve derived connection to ownership graph
diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js
@@ -35,25 +35,24 @@ import { component_context } from '../context.js';
 /*#__NO_SIDE_EFFECTS__*/
 export function derived(fn) {
 	var flags = DERIVED | DIRTY;
+	var parent_derived =
+		active_reaction !== null && (active_reaction.f & DERIVED) !== 0
+			? /** @type {Derived} */ (active_reaction)
+			: null;
 
-	if (active_effect === null) {
+	if (active_effect === null || (parent_derived !== null && (parent_derived.f & UNOWNED) !== 0)) {
 		flags |= UNOWNED;
 	} else {
 		// Since deriveds are evaluated lazily, any effects created inside them are
 		// created too late to ensure that the parent effect is added to the tree
 		active_effect.f |= EFFECT_HAS_DERIVED;
 	}
 
-	var parent_derived =
-		active_reaction !== null && (active_reaction.f & DERIVED) !== 0
-			? /** @type {Derived} */ (active_reaction)
-			: null;
-
 	/** @type {Derived<V>} */
 	const signal = {
-		children: null,
 		ctx: component_context,
 		deps: null,
+		effects: null,
 		equals,
 		f: flags,
 		fn,
@@ -87,19 +86,14 @@ export function derived_safe_equal(fn) {
  * @param {Derived} derived
  * @returns {void}
  */
-function destroy_derived_children(derived) {
-	var children = derived.children;
-
-	if (children !== null) {
-		derived.children = null;
-
-		for (var i = 0; i < children.length; i += 1) {
-			var child = children[i];
-			if ((child.f & DERIVED) !== 0) {
-				destroy_derived(/** @type {Derived} */ (child));
-			} else {
-				destroy_effect(/** @type {Effect} */ (child));
-			}
+export function destroy_derived_effects(derived) {
+	var effects = derived.effects;
+
+	if (effects !== null) {
+		derived.effects = null;
+
+		for (var i = 0; i < effects.length; i += 1) {
+			destroy_effect(/** @type {Effect} */ (effects[i]));
 		}
 	}
 }
@@ -147,7 +141,7 @@ export function execute_derived(derived) {
 
 			stack.push(derived);
 
-			destroy_derived_children(derived);
+			destroy_derived_effects(derived);
 			value = update_reaction(derived);
 		} finally {
 			set_active_effect(prev_active_effect);
@@ -156,7 +150,7 @@ export function execute_derived(derived) {
 		}
 	} else {
 		try {
-			destroy_derived_children(derived);
+			destroy_derived_effects(derived);
 			value = update_reaction(derived);
 		} finally {
 			set_active_effect(prev_active_effect);
@@ -188,9 +182,9 @@ export function update_derived(derived) {
  * @returns {void}
  */
 export function destroy_derived(derived) {
-	destroy_derived_children(derived);
+	destroy_derived_effects(derived);
 	remove_reactions(derived, 0);
 	set_signal_status(derived, DESTROYED);
 
-	derived.v = derived.children = derived.deps = derived.ctx = derived.reactions = null;
+	derived.v = derived.deps = derived.ctx = derived.reactions = null;
 }
diff --git a/packages/svelte/src/internal/client/reactivity/effects.js b/packages/svelte/src/internal/client/reactivity/effects.js
@@ -53,7 +53,7 @@ export function validate_effect(rune) {
 		e.effect_orphan(rune);
 	}
 
-	if (active_reaction !== null && (active_reaction.f & UNOWNED) !== 0) {
+	if (active_reaction !== null && (active_reaction.f & UNOWNED) !== 0 && active_effect === null) {
 		e.effect_in_unowned_derived();
 	}
 
@@ -99,7 +99,6 @@ 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,
@@ -153,7 +152,7 @@ function create_effect(type, fn, sync, push = true) {
 		// if we're in a derived, add the effect there too
 		if (active_reaction !== null && (active_reaction.f & DERIVED) !== 0) {
 			var derived = /** @type {Derived} */ (active_reaction);
-			(derived.children ??= []).push(effect);
+			(derived.effects ??= []).push(effect);
 		}
 	}
 
@@ -395,22 +394,6 @@ export function execute_effect_teardown(effect) {
 	}
 }
 
-/**
- * @param {Effect} signal
- * @returns {void}
- */
-export function destroy_effect_deriveds(signal) {
-	var deriveds = signal.deriveds;
-
-	if (deriveds !== null) {
-		signal.deriveds = null;
-
-		for (var i = 0; i < deriveds.length; i += 1) {
-			destroy_derived(deriveds[i]);
-		}
-	}
-}
-
 /**
  * @param {Effect} signal
  * @param {boolean} remove_dom
@@ -468,7 +451,6 @@ export function destroy_effect(effect, remove_dom = true) {
 	}
 
 	destroy_effect_children(effect, remove_dom && !removed);
-	destroy_effect_deriveds(effect);
 	remove_reactions(effect, 0);
 	set_signal_status(effect, DESTROYED);
 

diff --git a/packages/svelte/src/internal/client/reactivity/types.d.ts b/packages/svelte/src/internal/client/reactivity/types.d.ts
@@ -36,8 +36,8 @@ export interface Reaction extends Signal {
 export interface Derived<V = unknown> extends Value<V>, Reaction {
 	/** The derived function */
 	fn: () => V;
-	/** Reactions created inside this signal */
-	children: null | Reaction[];
+	/** Effects created inside this signal */
+	effects: null | Effect[];
 	/** Parent effect or derived */
 	parent: Effect | Derived | null;
 }
@@ -51,8 +51,6 @@ export interface Effect extends Reaction {
 	 */
 	nodes_start: null | TemplateNode;
 	nodes_end: null | TemplateNode;
-	/** Reactions created inside this signal */
-	deriveds: null | Derived[];
 	/** The effect function */
 	fn: null | (() => void | (() => void));
 	/** The teardown function returned from the effect function */

diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js
@@ -4,7 +4,6 @@ import { define_property, get_descriptors, get_prototype_of, index_of } from '..
 import {
 	destroy_block_effect_children,
 	destroy_effect_children,
-	destroy_effect_deriveds,
 	execute_effect_teardown,
 	unlink_effect
 } from './reactivity/effects.js';
@@ -28,7 +27,12 @@ import {
 } from './constants.js';
 import { flush_tasks } from './dom/task.js';
 import { internal_set, set } from './reactivity/sources.js';
-import { destroy_derived, execute_derived, update_derived } from './reactivity/deriveds.js';
+import {
+	destroy_derived,
+	destroy_derived_effects,
+	execute_derived,
+	update_derived
+} from './reactivity/deriveds.js';
 import * as e from './errors.js';
 import { FILENAME } from '../../constants.js';
 import { legacy_mode_flag, tracing_mode_flag } from '../flags/index.js';
@@ -409,7 +413,16 @@ export function update_reaction(reaction) {
 	skipped_deps = 0;
 	untracked_writes = null;
 	active_reaction = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 ? reaction : null;
-	skip_reaction = !is_flushing_effect && (flags & UNOWNED) !== 0;
+	// prettier-ignore
+	skip_reaction =
+		(flags & UNOWNED) !== 0 &&
+		(!is_flushing_effect ||
+			// If we were previously not in a reactive context and we're reading an unowned derived
+			// that was created inside another reaction, then we don't fully know the real owner and thus
+			// we need to skip adding any reactions for this unowned
+				((previous_reaction === null || previous_untracking) &&
+				/** @type {Derived} */ (reaction).parent !== null));
+
 	derived_sources = null;
 	set_component_context(reaction.ctx);
 	untracking = false;
@@ -517,6 +530,8 @@ function remove_reaction(signal, dependency) {
 		if ((dependency.f & (UNOWNED | DISCONNECTED)) === 0) {
 			dependency.f ^= DISCONNECTED;
 		}
+		// Disconnect any reactions owned by this reaction
+		destroy_derived_effects(/** @type {Derived} **/ (dependency));
 		remove_reactions(/** @type {Derived} **/ (dependency), 0);
 	}
 }
@@ -564,7 +579,6 @@ export function update_effect(effect) {
 		} else {
 			destroy_effect_children(effect);
 		}
-		destroy_effect_deriveds(effect);
 
 		execute_effect_teardown(effect);
 		var teardown = update_reaction(effect);
@@ -934,30 +948,20 @@ export function get(signal) {
 				new_deps.push(signal);
 			}
 		}
-	}
-
-	if (
+	} else if (
 		is_derived &&
 		/** @type {Derived} */ (signal).deps === null &&
-		(active_reaction === null || untracking || (active_reaction.f & DERIVED) !== 0)
+		/** @type {Derived} */ (signal).effects === null
 	) {
 		var derived = /** @type {Derived} */ (signal);
 		var parent = derived.parent;
 
 		if (parent !== null) {
-			// Attach the derived to the nearest parent effect or derived
-			if ((parent.f & DERIVED) !== 0) {
-				var parent_derived = /** @type {Derived} */ (parent);
-
-				if (!parent_derived.children?.includes(derived)) {
-					(parent_derived.children ??= []).push(derived);
-				}
-			} else {
-				var parent_effect = /** @type {Effect} */ (parent);
-
-				if (!parent_effect.deriveds?.includes(derived)) {
-					(parent_effect.deriveds ??= []).push(derived);
-				}
+			// If the derived is owned by another derived then mark it as unowned
+			// as the derived value might have been referenced in a different context
+			// since and thus its parent might not be its true owner anymore
+			if ((parent.f & UNOWNED) === 0) {
+				derived.f ^= UNOWNED;
 			}
 		}
 	}