Skip to content

Commit

Permalink
fix: increment derived versions when updating (#12047)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
Rich-Harris authored Jun 17, 2024
1 parent 95d07de commit f5f3879
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 8 deletions.
5 changes: 5 additions & 0 deletions .changeset/brave-pigs-obey.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: increment derived versions when updating
4 changes: 3 additions & 1 deletion packages/svelte/src/internal/client/reactivity/deriveds.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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);

Expand Down
2 changes: 0 additions & 2 deletions packages/svelte/src/internal/client/reactivity/props.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions packages/svelte/src/internal/client/reactivity/sources.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions packages/svelte/src/internal/client/runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand All @@ -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];
Expand Down
93 changes: 92 additions & 1 deletion packages/svelte/tests/signals/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]
);
};
});
Expand Down Expand Up @@ -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<string, boolean | null> = {};

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');
};
});
});

0 comments on commit f5f3879

Please sign in to comment.