msb-public
/
svelte
mirror of https://github.com/sveltejs/svelte
1
0
Code Issues Projects Releases Wiki Activity

fix: improve derived connection to ownership graph ()

Browse Source 
* fix: improve derived connection to ownership graph

* revised

* revised

* revised

* revised

* feedback

* feedback

* invasive change

* fix bugs

* fix other bug
pull/15139/head
Dominic Gannaway 3 months ago committed by GitHub
parent 9410ad0318
commit 13a6d555c0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 199 additions and 123 deletions

5
.changeset/blue-rocks-play.md
Unescape View File

@ -0,0 +1,5 @@
---
'svelte': patch
---
fix: improve derived connection to ownership graph

42
packages/svelte/src/internal/client/reactivity/deriveds.js
Unescape View File

@ -35,8 +35,12 @@ 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
@ -44,16 +48,11 @@ export function derived(fn) {
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;
}

22
packages/svelte/src/internal/client/reactivity/effects.js vendored
Unescape View File

@ -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);

6
packages/svelte/src/internal/client/reactivity/types.d.ts vendored
Unescape View File

@ -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 */

46
packages/svelte/src/internal/client/runtime.js
Unescape View File

@ -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;
}
}
}

201
packages/svelte/tests/signals/test.ts
Unescape View File

@ -9,11 +9,12 @@ import {
user_effect
} from '../../src/internal/client/reactivity/effects';
import { state, set } from '../../src/internal/client/reactivity/sources';
import type { Derived, Value } from '../../src/internal/client/types';
import type { Derived, Effect, Value } from '../../src/internal/client/types';
import { proxy } from '../../src/internal/client/proxy';
import { derived } from '../../src/internal/client/reactivity/deriveds';
import { snapshot } from '../../src/internal/shared/clone.js';
import { SvelteSet } from '../../src/reactivity/set';
import { DESTROYED } from '../../src/internal/client/constants';
/**
* @param runes runes mode
@ -68,7 +69,7 @@ describe('signals', () => {
};
});
test('multiple effects with state and derived in it#1', () => {
test('multiple effects with state and derived in it #1', () => {
const log: string[] = [];
let count = state(0);
@ -89,7 +90,7 @@ describe('signals', () => {
};
});
test('multiple effects with state and derived in it#2', () => {
test('multiple effects with state and derived in it #2', () => {
const log: string[] = [];
let count = state(0);
@ -256,12 +257,16 @@ describe('signals', () => {
const a = state(0);
const b = state(0);
const c = derived(() => {
const a_2 = derived(() => $.get(a) + '!');
const b_2 = derived(() => $.get(b) + '?');
nested.push(a_2, b_2);
let c: any;
const destroy = effect_root(() => {
c = derived(() => {
const a_2 = derived(() => $.get(a) + '!');
const b_2 = derived(() => $.get(b) + '?');
nested.push(a_2, b_2);
return { a: $.get(a_2), b: $.get(b_2) };
return { a: $.get(a_2), b: $.get(b_2) };
});
});
$.get(c);
@ -274,11 +279,10 @@ describe('signals', () => {
$.get(c);
// Ensure we're not leaking dependencies
assert.deepEqual(
nested.slice(0, -2).map((s) => s.deps),
[null, null, null, null]
);
destroy();
assert.equal(a.reactions, null);
assert.equal(b.reactions, null);
};
});
@ -477,6 +481,7 @@ describe('signals', () => {
effect(() => {
log.push('inner', $.get(inner));
});
return $.get(outer);
});
});
});
@ -530,6 +535,103 @@ describe('signals', () => {
};
});
test('mixed nested deriveds correctly cleanup when no longer connected to graph #1', () => {
let a: Derived<unknown>;
let b: Derived<unknown>;
let s = state(0);
const destroy = effect_root(() => {
render_effect(() => {
a = derived(() => {
b = derived(() => {
$.get(s);
});
$.untrack(() => {
$.get(b);
});
$.get(s);
});
$.get(a);
});
});
return () => {
flushSync();
assert.equal(a?.deps?.length, 1);
assert.equal(s?.reactions?.length, 1);
destroy();
assert.equal(s?.reactions, null);
};
});
test('mixed nested deriveds correctly cleanup when no longer connected to graph #2', () => {
let a: Derived<unknown>;
let b: Derived<unknown>;
let s = state(0);
const destroy = effect_root(() => {
render_effect(() => {
a = derived(() => {
b = derived(() => {
$.get(s);
});
effect_root(() => {
$.get(b);
});
$.get(s);
});
$.get(a);
});
});
return () => {
flushSync();
assert.equal(a?.deps?.length, 1);
assert.equal(s?.reactions?.length, 1);
destroy();
assert.equal(s?.reactions, null);
};
});
test('mixed nested deriveds correctly cleanup when no longer connected to graph #3', () => {
let a: Derived<unknown>;
let b: Derived<unknown>;
let s = state(0);
let logs: any[] = [];
const destroy = effect_root(() => {
render_effect(() => {
a = derived(() => {
b = derived(() => {
return $.get(s);
});
effect_root(() => {
$.get(b);
});
render_effect(() => {
logs.push($.get(b));
});
$.get(s);
});
$.get(a);
});
});
return () => {
flushSync();
assert.equal(a?.deps?.length, 1);
assert.equal(s?.reactions?.length, 2);
set(s, 1);
flushSync();
assert.deepEqual(logs, [0, 1]);
destroy();
assert.equal(s?.reactions, null);
};
});
test('deriveds update upon reconnection #1', () => {
let a = state(false);
let b = state(false);
@ -778,56 +880,44 @@ describe('signals', () => {
};
});
test('nested deriveds clean up the relationships when used with untrack', () => {
test('deriveds containing effects work correctly', () => {
return () => {
let a = render_effect(() => {});
let b = state(0);
let c;
let effects: Effect[] = [];
const destroy = effect_root(() => {
a = render_effect(() => {
$.untrack(() => {
const b = derived(() => {
const c = derived(() => {});
$.untrack(() => {
$.get(c);
});
});
c = derived(() => {
effects.push(
effect(() => {
$.get(b);
})
);
$.get(b);
});
$.get(c);
});
});
assert.deepEqual(a.deriveds?.length, 1);
destroy();
assert.equal(c!.effects?.length, 1);
assert.equal(a.first, a.last);
assert.deepEqual(a.deriveds, null);
};
});
test('nested deriveds do not connect inside parent deriveds if unused', () => {
return () => {
let a = render_effect(() => {});
let b: Derived<void> | undefined;
set(b, 1);
const destroy = effect_root(() => {
a = render_effect(() => {
$.untrack(() => {
b = derived(() => {
derived(() => {});
derived(() => {});
derived(() => {});
});
$.get(b);
});
});
});
flushSync();
assert.deepEqual(a.deriveds?.length, 1);
assert.deepEqual(b?.children, null);
assert.equal(c!.effects?.length, 1);
assert.equal(a.first, a.last);
destroy();
assert.deepEqual(a.deriveds, null);
assert.equal(a.first, null);
assert.equal(effects.length, 2);
assert.equal((effects[0].f & DESTROYED) !== 0, true);
assert.equal((effects[1].f & DESTROYED) !== 0, true);
};
});
@ -836,7 +926,7 @@ describe('signals', () => {
let a = render_effect(() => {});
let b = state(0);
let c;
let effects = [];
let effects: Effect[] = [];
const destroy = effect_root(() => {
a = render_effect(() => {
@ -854,20 +944,23 @@ describe('signals', () => {
});
});
assert.deepEqual(c!.children?.length, 1);
assert.deepEqual(a.first, a.last);
assert.equal(c!.effects?.length, 1);
assert.equal(a.first, a.last);
set(b, 1);
flushSync();
assert.deepEqual(c!.children?.length, 1);
assert.deepEqual(a.first, a.last);
assert.equal(c!.effects?.length, 1);
assert.equal(a.first, a.last);
destroy();
assert.deepEqual(a.deriveds, null);
assert.deepEqual(a.first, null);
assert.equal(a.first, null);
assert.equal(effects.length, 2);
assert.equal((effects[0].f & DESTROYED) !== 0, true);
assert.equal((effects[1].f & DESTROYED) !== 0, true);
};
});

Write Preview
Loading…
Cancel
Save