From bcb1b8eb98b0150c4b2d392b3b89ee3587fc1019 Mon Sep 17 00:00:00 2001 From: Dominic Gannaway Date: Tue, 20 Feb 2024 15:49:30 +0000 Subject: [PATCH] fix: improve handling of unowned derived signals (#10565) * fix: improve handling of unowned derived signals * lint --- .changeset/green-tigers-judge.md | 5 +++ .../svelte/src/internal/client/runtime.js | 35 +++++++++++++++---- .../svelte/src/internal/client/types.d.ts | 4 +++ packages/svelte/tests/signals/test.ts | 24 ++++++++++++- 4 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 .changeset/green-tigers-judge.md diff --git a/.changeset/green-tigers-judge.md b/.changeset/green-tigers-judge.md new file mode 100644 index 0000000000..b3548bd11f --- /dev/null +++ b/.changeset/green-tigers-judge.md @@ -0,0 +1,5 @@ +--- +"svelte": patch +--- + +fix: improve handling of unowned derived signals diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 67891e84ce..49dfe69448 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -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() }; @@ -179,7 +181,9 @@ function create_source_signal(flags, value) { // flags f: flags, // value - v: value + v: value, + // write version + w: 0 }; } @@ -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 @@ -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 @@ -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; + } } } } @@ -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)) { @@ -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 { @@ -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 diff --git a/packages/svelte/src/internal/client/types.d.ts b/packages/svelte/src/internal/client/types.d.ts index 937028a84e..425824049d 100644 --- a/packages/svelte/src/internal/client/types.d.ts +++ b/packages/svelte/src/internal/client/types.d.ts @@ -77,6 +77,8 @@ export type SourceSignal = { f: SignalFlags; /** value: The latest value for this signal */ v: V; + // write version + w: number; }; export type SourceSignalDebug = { @@ -111,6 +113,8 @@ export type ComputationSignal = { 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 = SourceSignal | ComputationSignal; diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 0f8864ad6d..9cecf3d5ea 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -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] ); }; }); @@ -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> = []; + + $.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 () => {};