Skip to content

Commit 2910fbf

Browse files
authored
fix(core): keep signal versions monotonic when a batch reverts a value (#947)
Rolling the version back during batch snapshot reconciliation let a later write re-mint a version number that a lazy computed had already observed mid-batch, permanently serving a stale cached value. Fast-forward subscriber nodes that saw the pre-batch version instead.
1 parent d40746b commit 2910fbf

3 files changed

Lines changed: 76 additions & 2 deletions

File tree

.changeset/olive-poems-refuse.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"@preact/signals-core": patch
3+
---
4+
5+
Fix computeds returning stale values after a batch reverts a signal to its original value.
6+
7+
Reconciling a reverted batch write used to roll the signal's version number back, breaking version monotonicity. A lazy computed that read the signal during the batch had already observed the intermediate version, so a later write could re-mint that same version number for a different value and the computed would treat it as unchanged forever. Subscriber nodes that saw the pre-batch version are now fast-forwarded instead, keeping the no-op skip optimization without ever reusing version numbers.

packages/core/src/index.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,8 +182,24 @@ function reconcileBatchSnapshots() {
182182
batchSnapshots = undefined;
183183

184184
while (snapshots !== undefined) {
185-
if (snapshots._source._value === snapshots._value) {
186-
snapshots._source._version = snapshots._version;
185+
const source = snapshots._source;
186+
if (source._value === snapshots._value) {
187+
// The value was reverted to its pre-batch state. Version numbers must
188+
// stay monotonic: a lazy computed may have observed an intermediate
189+
// version during the batch, and rolling the version back would let a
190+
// future write re-mint that observed number for a different value,
191+
// making the computed treat it as unchanged forever. Instead,
192+
// fast-forward subscribers that last saw the pre-batch version so
193+
// they skip recomputing for the no-op change.
194+
for (
195+
let node = source._targets;
196+
node !== undefined;
197+
node = node._nextTarget
198+
) {
199+
if (node._version === snapshots._version) {
200+
node._version = source._version;
201+
}
202+
}
187203
}
188204
snapshots = snapshots._next;
189205
}

packages/core/test/signal.test.tsx

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -893,6 +893,57 @@ describe("effect()", () => {
893893
expect(spy).not.toHaveBeenCalled();
894894
});
895895

896+
it("should not rerun an effect subscribed through a computed for a no-op batch assignment", () => {
897+
const foo = signal(42);
898+
const double = computed(() => foo.value * 2);
899+
const spy = vi.fn(() => {
900+
double.value;
901+
});
902+
903+
effect(spy);
904+
expect(spy).toHaveBeenCalledOnce();
905+
spy.mockClear();
906+
907+
batch(() => {
908+
foo.value = 0;
909+
foo.value = 42;
910+
});
911+
912+
expect(spy).not.toHaveBeenCalled();
913+
});
914+
915+
it("should keep unsubscribed computeds coherent when they are read during a reverted batch", () => {
916+
const foo = signal("A");
917+
const double = computed(() => foo.value + "!");
918+
expect(double.value).toBe("A!");
919+
920+
batch(() => {
921+
foo.value = "B";
922+
// A lazy read inside the batch observes the intermediate version.
923+
expect(double.value).toBe("B!");
924+
foo.value = "A";
925+
});
926+
927+
// The next write must not collide with the version observed mid-batch.
928+
foo.value = "C";
929+
expect(double.value).toBe("C!");
930+
});
931+
932+
it("should keep unsubscribed computeds coherent when they are peeked during a reverted batch", () => {
933+
const foo = signal(1);
934+
const double = computed(() => foo.value * 2);
935+
expect(double.peek()).toBe(2);
936+
937+
batch(() => {
938+
foo.value = 2;
939+
expect(double.peek()).toBe(4);
940+
foo.value = 1;
941+
});
942+
943+
foo.value = 3;
944+
expect(double.peek()).toBe(6);
945+
});
946+
896947
it("should not rerun parent effect if a nested child effect's signal's value changes", () => {
897948
const parentSignal = signal(0);
898949
const childSignal = signal(0);

0 commit comments

Comments
 (0)