From f5f38796baa1c36310c3f388c3a10fc872ca4e37 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Mon, 17 Jun 2024 01:03:25 -0700 Subject: [PATCH] fix: increment derived versions when updating (#12047) We need to ensure that if derived a depends on derived b, and a is reconnecting to the dependency graph, that if b was updated more recently than a, a also updates. Fixes #11988 Fixes #12044 --- .changeset/brave-pigs-obey.md | 5 + .../internal/client/reactivity/deriveds.js | 4 +- .../src/internal/client/reactivity/props.js | 2 - .../src/internal/client/reactivity/sources.js | 5 +- .../svelte/src/internal/client/runtime.js | 10 +- packages/svelte/tests/signals/test.ts | 93 ++++++++++++++++++- 6 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 .changeset/brave-pigs-obey.md diff --git a/.changeset/brave-pigs-obey.md b/.changeset/brave-pigs-obey.md new file mode 100644 index 0000000000..312ead8ade --- /dev/null +++ b/.changeset/brave-pigs-obey.md @@ -0,0 +1,5 @@ +--- +'svelte': patch +--- + +fix: increment derived versions when updating diff --git a/packages/svelte/src/internal/client/reactivity/deriveds.js b/packages/svelte/src/internal/client/reactivity/deriveds.js index 6ebd945301..d50b335a7b 100644 --- a/packages/svelte/src/internal/client/reactivity/deriveds.js +++ b/packages/svelte/src/internal/client/reactivity/deriveds.js @@ -8,7 +8,8 @@ import { mark_reactions, current_skip_reaction, execute_reaction_fn, - destroy_effect_children + destroy_effect_children, + increment_version } from '../runtime.js'; import { equals, safe_equals } from './equality.js'; @@ -104,6 +105,7 @@ export function update_derived(derived, force_schedule) { var is_equal = derived.equals(value); if (!is_equal) { + derived.version = increment_version(); derived.v = value; mark_reactions(derived, DIRTY, force_schedule); diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index a12da16090..665d7fdc1f 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -323,8 +323,6 @@ export function prop(props, key, flags, fallback) { from_child = true; set(inner_current_value, value); get(current_value); // force a synchronisation immediately - // Increment the value so that unowned/disconnected nodes can validate dirtiness again. - current_value.version++; } return value; diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 9004e02ca3..f366ae5fe8 100644 --- a/packages/svelte/src/internal/client/reactivity/sources.js +++ b/packages/svelte/src/internal/client/reactivity/sources.js @@ -14,7 +14,8 @@ import { set_current_untracked_writes, set_last_inspected_signal, set_signal_status, - untrack + untrack, + increment_version } from '../runtime.js'; import { equals, safe_equals } from './equality.js'; import { CLEAN, DERIVED, DIRTY, BRANCH_EFFECT } from '../constants.js'; @@ -99,7 +100,7 @@ export function set(signal, value) { signal.v = value; // Increment write version so that unowned signals can properly track dirtiness - signal.version++; + signal.version = increment_version(); // If the current signal is running for the first time, it won't have any // reactions as we only allocate and assign the reactions after the signal diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 8b9ca1c64b..0b8c522140 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -122,6 +122,9 @@ export function set_last_inspected_signal(signal) { /** If `true`, `get`ting the signal should not register it as a dependency */ export let current_untracking = false; +/** @type {number} */ +let current_version = 0; + // If we are working with a get() chain that has no active container, // to prevent memory leaks, we skip adding the reaction. export let current_skip_reaction = false; @@ -155,6 +158,10 @@ export function set_dev_current_component_function(fn) { dev_current_component_function = fn; } +export function increment_version() { + return current_version++; +} + /** @returns {boolean} */ export function is_runes() { return current_component_context !== null && current_component_context.l === null; @@ -234,7 +241,6 @@ export function check_dirtiness(reaction) { // is also dirty. if (version > /** @type {import('#client').Derived} */ (reaction).version) { - /** @type {import('#client').Derived} */ (reaction).version = version; return !is_equal; } @@ -257,9 +263,9 @@ export function check_dirtiness(reaction) { // In thise case, we need to re-attach it to the graph and mark it dirty if any of its dependencies have // changed since. if (version > /** @type {import('#client').Derived} */ (reaction).version) { - /** @type {import('#client').Derived} */ (reaction).version = version; is_dirty = true; } + reactions = dependency.reactions; if (reactions === null) { dependency.reactions = [reaction]; diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 2c681fd41f..f8e8e54b82 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -232,7 +232,7 @@ describe('signals', () => { // Ensure we're not leaking dependencies assert.deepEqual( nested.slice(0, -2).map((s) => s.deps), - [null, null] + [null, null, null, null] ); }; }); @@ -446,4 +446,95 @@ describe('signals', () => { assert.equal(state?.reactions, null); }; }); + + test('deriveds update upon reconnection #1', () => { + let a = source(false); + let b = source(false); + + let c = derived(() => $.get(a)); + let d = derived(() => $.get(c)); + + let last: Record = {}; + + render_effect(() => { + last = { + a: $.get(a), + b: $.get(b), + c: $.get(c), + d: $.get(a) || $.get(b) ? $.get(d) : null + }; + }); + + return () => { + assert.deepEqual(last, { a: false, b: false, c: false, d: null }); + + flushSync(() => set(a, true)); + flushSync(() => set(b, true)); + assert.deepEqual(last, { a: true, b: true, c: true, d: true }); + + flushSync(() => set(a, false)); + flushSync(() => set(b, false)); + assert.deepEqual(last, { a: false, b: false, c: false, d: null }); + + flushSync(() => set(a, true)); + flushSync(() => set(b, true)); + assert.deepEqual(last, { a: true, b: true, c: true, d: true }); + + flushSync(() => set(a, false)); + flushSync(() => set(b, false)); + assert.deepEqual(last, { a: false, b: false, c: false, d: null }); + + flushSync(() => set(b, true)); + assert.deepEqual(last, { a: false, b: true, c: false, d: false }); + }; + }); + + test('deriveds update upon reconnection #2', () => { + let a = source(false); + let b = source(false); + let c = source(false); + + let d = derived(() => $.get(a) || $.get(b)); + + let branch = ''; + + render_effect(() => { + if ($.get(c) && !$.get(d)) { + branch = 'if'; + } else { + branch = 'else'; + } + }); + + return () => { + assert.deepEqual(branch, 'else'); + + flushSync(() => set(c, true)); + assert.deepEqual(branch, 'if'); + + flushSync(() => set(a, true)); + assert.deepEqual(branch, 'else'); + + set(a, false); + set(b, false); + set(c, false); + flushSync(); + assert.deepEqual(branch, 'else'); + + flushSync(() => set(c, true)); + assert.deepEqual(branch, 'if'); + + flushSync(() => set(b, true)); + assert.deepEqual(branch, 'else'); + + set(a, false); + set(b, false); + set(c, false); + flushSync(); + assert.deepEqual(branch, 'else'); + + flushSync(() => set(c, true)); + assert.deepEqual(branch, 'if'); + }; + }); });