From 92a9b6fcda9f938257d3890af445589e661b1fca Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sun, 16 Jun 2024 13:09:23 -0400 Subject: [PATCH 1/4] fix: increment derived versions when updating --- .changeset/brave-pigs-obey.md | 5 +++ .../internal/client/reactivity/deriveds.js | 4 +- .../src/internal/client/reactivity/props.js | 4 +- .../src/internal/client/reactivity/sources.js | 5 ++- .../svelte/src/internal/client/runtime.js | 12 +++++- packages/svelte/tests/signals/test.ts | 37 ++++++++++++++++++- 6 files changed, 59 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 000000000000..312ead8adee9 --- /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 6ebd945301d1..d50b335a7b52 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 a12da1609074..58e79597c6f1 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -8,7 +8,7 @@ import { import { get_descriptor, is_function } from '../utils.js'; import { mutable_source, set, source } from './sources.js'; import { derived, derived_safe_equal } from './deriveds.js'; -import { get, is_signals_recorded, untrack, update } from '../runtime.js'; +import { get, increment_version, is_signals_recorded, untrack, update } from '../runtime.js'; import { safe_equals } from './equality.js'; import { inspect_fn } from '../dev/inspect.js'; import * as e from '../errors.js'; @@ -324,7 +324,7 @@ export function prop(props, key, flags, fallback) { 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++; + current_value.version = increment_version(); } return value; diff --git a/packages/svelte/src/internal/client/reactivity/sources.js b/packages/svelte/src/internal/client/reactivity/sources.js index 9004e02ca3fc..f366ae5fe85d 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 8b9ca1c64b14..69b0ccf0e3a4 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,7 @@ export function check_dirtiness(reaction) { // is also dirty. if (version > /** @type {import('#client').Derived} */ (reaction).version) { - /** @type {import('#client').Derived} */ (reaction).version = version; + /** @type {import('#client').Derived} */ (reaction).version = increment_version(); return !is_equal; } @@ -257,9 +264,10 @@ 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; + /** @type {import('#client').Derived} */ (reaction).version = increment_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 2c681fd41f1d..0fd5924330d4 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,39 @@ describe('signals', () => { assert.equal(state?.reactions, null); }; }); + + test('deriveds update upon reconnection', () => { + 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 }); + }; + }); }); From 0d4c77abd250c621a008dcda24e19408f6511095 Mon Sep 17 00:00:00 2001 From: Rich Harris Date: Sun, 16 Jun 2024 18:43:56 -0400 Subject: [PATCH 2/4] we only need to increment version when setting sources and updating deriveds --- packages/svelte/src/internal/client/reactivity/props.js | 4 +--- packages/svelte/src/internal/client/runtime.js | 2 -- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/packages/svelte/src/internal/client/reactivity/props.js b/packages/svelte/src/internal/client/reactivity/props.js index 58e79597c6f1..665d7fdc1f10 100644 --- a/packages/svelte/src/internal/client/reactivity/props.js +++ b/packages/svelte/src/internal/client/reactivity/props.js @@ -8,7 +8,7 @@ import { import { get_descriptor, is_function } from '../utils.js'; import { mutable_source, set, source } from './sources.js'; import { derived, derived_safe_equal } from './deriveds.js'; -import { get, increment_version, is_signals_recorded, untrack, update } from '../runtime.js'; +import { get, is_signals_recorded, untrack, update } from '../runtime.js'; import { safe_equals } from './equality.js'; import { inspect_fn } from '../dev/inspect.js'; import * as e from '../errors.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 = increment_version(); } return value; diff --git a/packages/svelte/src/internal/client/runtime.js b/packages/svelte/src/internal/client/runtime.js index 69b0ccf0e3a4..0b8c522140c9 100644 --- a/packages/svelte/src/internal/client/runtime.js +++ b/packages/svelte/src/internal/client/runtime.js @@ -241,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 = increment_version(); return !is_equal; } @@ -264,7 +263,6 @@ 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 = increment_version(); is_dirty = true; } From 722552a5f936d5bc1f8a437c99cd262aeac90ea3 Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 17 Jun 2024 09:38:41 +0200 Subject: [PATCH 3/4] enhance test --- packages/svelte/tests/signals/test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index 0fd5924330d4..afaf95c85de0 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -479,6 +479,13 @@ describe('signals', () => { 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 }); }; }); }); From 2a9da583654d08066573e3cf367021a1a97bc97a Mon Sep 17 00:00:00 2001 From: Simon Holthausen Date: Mon, 17 Jun 2024 09:58:28 +0200 Subject: [PATCH 4/4] another test --- packages/svelte/tests/signals/test.ts | 51 ++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/packages/svelte/tests/signals/test.ts b/packages/svelte/tests/signals/test.ts index afaf95c85de0..f8e8e54b825a 100644 --- a/packages/svelte/tests/signals/test.ts +++ b/packages/svelte/tests/signals/test.ts @@ -447,7 +447,7 @@ describe('signals', () => { }; }); - test('deriveds update upon reconnection', () => { + test('deriveds update upon reconnection #1', () => { let a = source(false); let b = source(false); @@ -488,4 +488,53 @@ describe('signals', () => { 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'); + }; + }); });