Skip to content

fix: improve handling of unowned derived signals #10565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/green-tigers-judge.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: improve handling of unowned derived signals
35 changes: 29 additions & 6 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ function create_source_signal(flags, value) {
f: flags,
// value
v: value,
// write version
w: 0,
// this is for DEV only
inspect: new Set()
};
Expand All @@ -179,7 +181,9 @@ function create_source_signal(flags, value) {
// flags
f: flags,
// value
v: value
v: value,
// write version
w: 0
};
}

Expand Down Expand Up @@ -211,6 +215,8 @@ function create_computation_signal(flags, value, block) {
r: null,
// value
v: value,
// write version
w: 0,
// context: We can remove this if we get rid of beforeUpdate/afterUpdate
x: null,
// destroy
Expand Down Expand Up @@ -239,6 +245,8 @@ function create_computation_signal(flags, value, block) {
r: null,
// value
v: value,
// write version
w: 0,
// context: We can remove this if we get rid of beforeUpdate/afterUpdate
x: null,
// destroy
Expand Down Expand Up @@ -296,6 +304,17 @@ function is_signal_dirty(signal) {
return true;
}
}
// If we're workig with an unowned derived signal, then we need to check
// if our dependency write version is higher. If is is then we can assume
// that state has changed to a newer version and thus this unowned signal
// is also dirty.
const is_unowned = (flags & UNOWNED) !== 0;
const write_version = signal.w;
const dep_write_version = dependency.w;
if (is_unowned && dep_write_version > write_version) {
signal.w = dep_write_version;
return true;
}
}
}
}
Expand Down Expand Up @@ -825,7 +844,9 @@ function update_derived(signal, force_schedule) {
const value = execute_signal_fn(signal);
updating_derived = previous_updating_derived;
const status =
(current_skip_consumer || (signal.f & UNOWNED) !== 0) && signal.d !== null ? DIRTY : CLEAN;
(current_skip_consumer || (signal.f & UNOWNED) !== 0) && signal.d !== null
? MAYBE_DIRTY
: CLEAN;
set_signal_status(signal, status);
const equals = /** @type {import('./types.js').EqualsFunctions} */ (signal.e);
if (!equals(value, signal.v)) {
Expand Down Expand Up @@ -1153,18 +1174,18 @@ function mark_signal_consumers(signal, to_status, force_schedule) {
const consumer = consumers[i];
const flags = consumer.f;
const unowned = (flags & UNOWNED) !== 0;
const dirty = (flags & DIRTY) !== 0;
// We skip any effects that are already dirty (but not unowned). Additionally, we also
// skip if the consumer is the same as the current effect (except if we're not in runes or we
// are in force schedule mode).
if ((dirty && !unowned) || ((!force_schedule || !runes) && consumer === current_effect)) {
if ((!force_schedule || !runes) && consumer === current_effect) {
continue;
}
set_signal_status(consumer, to_status);
// If the signal is not clean, then skip over it – with the exception of unowned signals that
// are already dirty. Unowned signals might be dirty because they are not captured as part of an
// are already maybe dirty. Unowned signals might be dirty because they are not captured as part of an
// effect.
if ((flags & CLEAN) !== 0 || (dirty && unowned)) {
const maybe_dirty = (flags & MAYBE_DIRTY) !== 0;
if ((flags & CLEAN) !== 0 || (maybe_dirty && unowned)) {
if ((consumer.f & IS_EFFECT) !== 0) {
schedule_effect(/** @type {import('./types.js').EffectSignal} */ (consumer), false);
} else {
Expand Down Expand Up @@ -1203,6 +1224,8 @@ export function set_signal_value(signal, value) {
!(/** @type {import('./types.js').EqualsFunctions} */ (signal.e)(value, signal.v))
) {
signal.v = value;
// Increment write version so that unowned signals can properly track dirtyness
signal.w++;
// If the current signal is running for the first time, it won't have any
// consumers as we only allocate and assign the consumers after the signal
// has fully executed. So in the case of ensuring it registers the consumer
Expand Down
4 changes: 4 additions & 0 deletions packages/svelte/src/internal/client/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ export type SourceSignal<V = unknown> = {
f: SignalFlags;
/** value: The latest value for this signal */
v: V;
// write version
w: number;
};

export type SourceSignalDebug = {
Expand Down Expand Up @@ -111,6 +113,8 @@ export type ComputationSignal<V = unknown> = {
v: V;
/** level: the depth from the root signal, used for ordering render/pre-effects topologically **/
l: number;
/** write version: used for unowned signals to track if their depdendencies are dirty or not **/
w: number;
};

export type Signal<V = unknown> = SourceSignal<V> | ComputationSignal<V>;
Expand Down
24 changes: 23 additions & 1 deletion packages/svelte/tests/signals/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ describe('signals', () => {
// Ensure we're not leaking dependencies
assert.deepEqual(
nested.slice(0, -2).map((s) => s.d),
[null, null, null, null]
[null, null]
);
};
});
Expand Down Expand Up @@ -284,6 +284,28 @@ describe('signals', () => {
};
});

let some_state = $.source({});
let some_deps = $.derived(() => {
return [$.get(some_state)];
});

test('two effects with an unowned derived that has some depedencies', () => {
const log: Array<Array<any>> = [];

$.render_effect(() => {
log.push($.get(some_deps));
});

$.render_effect(() => {
log.push($.get(some_deps));
});

return () => {
$.flushSync();
assert.deepEqual(log, [[{}], [{}]]);
};
});

test('schedules rerun when writing to signal before reading it', (runes) => {
if (!runes) return () => {};

Expand Down