diff --git a/.changeset/keep-observed-across-resubscribe.md b/.changeset/keep-observed-across-resubscribe.md index 0bb9a5b..889b57d 100644 --- a/.changeset/keep-observed-across-resubscribe.md +++ b/.changeset/keep-observed-across-resubscribe.md @@ -4,4 +4,4 @@ `useSelect` no longer re-runs `onUnobserved` and `onObserved` on a `ReactiveNode` it keeps reading when a prop moves the selector's other dependencies. Query nodes such as `ConvexQueryNode` no longer close and reopen their backend subscription on those re-renders. -React hooks now release a `ReactiveNode` one microtask after its last listener unsubscribes, so `onUnobserved` runs just after unmount rather than during it. +React hooks now release a `ReactiveNode` one microtask after its last listener unsubscribes, so `onUnobserved` runs just after unmount rather than during it. Tests that fake `queueMicrotask`, as Jest's fake timers do by default, need to flush microtasks (`jest.runAllTicks()`) before asserting on it. diff --git a/.changeset/unsubscribe-after-clear-listeners.md b/.changeset/unsubscribe-after-clear-listeners.md new file mode 100644 index 0000000..c707162 --- /dev/null +++ b/.changeset/unsubscribe-after-clear-listeners.md @@ -0,0 +1,5 @@ +--- +"@retreejs/core": patch +--- + +Calling a `Retree.on` unsubscribe after `Retree.clearListeners` already removed that listener no longer runs the node's `onUnobserved` a second time. diff --git a/.changeset/use-select-tree-version.md b/.changeset/use-select-tree-version.md new file mode 100644 index 0000000..e78caf4 --- /dev/null +++ b/.changeset/use-select-tree-version.md @@ -0,0 +1,8 @@ +--- +"@retreejs/core": patch +"@retreejs/react": patch +--- + +A selector that reads `Retree.treeVersion(node)` now re-runs on every write under `node`. + +Before, a selector-only `useSelect` dropped writes to descendants the selector never read directly, unlike `Retree.select`. And `useSelect`, `Retree.select`, `Retree.effect` and `@select` getters all skipped a write to a descendant's field when the selector also read a different field of that descendant. diff --git a/packages/retree-core/src/ReactiveNode.spec.ts b/packages/retree-core/src/ReactiveNode.spec.ts index f6fc567..a9741e1 100644 --- a/packages/retree-core/src/ReactiveNode.spec.ts +++ b/packages/retree-core/src/ReactiveNode.spec.ts @@ -511,6 +511,16 @@ describe("ReactiveNode", () => { expect(root.unobservedCount).toBe(1); }); + it("runs onUnobserved once when clearListeners precedes an unsubscribe", () => { + const root = trackRoot(Retree.root(new ObservedLifecycleNode())); + const unsubscribe = Retree.on(root, "nodeChanged", vi.fn()); + + Retree.clearListeners(root); + unsubscribe(); + + expect(root.unobservedCount).toBe(1); + }); + it("lazily proxies ReactiveNode object and array fields with parent metadata", () => { const root = trackRoot(Retree.root(new NestedPayloadNode())); diff --git a/packages/retree-core/src/Retree.ts b/packages/retree-core/src/Retree.ts index 0f34782..a80517e 100644 --- a/packages/retree-core/src/Retree.ts +++ b/packages/retree-core/src/Retree.ts @@ -1730,15 +1730,19 @@ export class Retree { if (unsubscribed) return; unsubscribed = true; const _listeners = relevantListenerMap.get(unproxiedNode); - if (_listeners) { - const findIndex = _listeners.findIndex((l) => l === callback); - if (findIndex !== -1) { - _listeners.splice(findIndex, 1); - this.handleListenerCountChanged(relevantListenerMap, -1); - } - if (_listeners.length === 0) { - relevantListenerMap.delete(unproxiedNode); - } + const findIndex = + _listeners === undefined + ? -1 + : _listeners.findIndex((l) => l === callback); + if (_listeners === undefined || findIndex === -1) { + // `clearListeners` already removed it and ran the teardown + // below; running it again would repeat `onUnobserved`. + return; + } + _listeners.splice(findIndex, 1); + this.handleListenerCountChanged(relevantListenerMap, -1); + if (_listeners.length === 0) { + relevantListenerMap.delete(unproxiedNode); } const isReactiveChangeListenerMap = relevantListenerMap !== this.nodeRemovedListeners; diff --git a/packages/retree-core/src/internals/select.ts b/packages/retree-core/src/internals/select.ts index af2b339..37fb7e5 100644 --- a/packages/retree-core/src/internals/select.ts +++ b/packages/retree-core/src/internals/select.ts @@ -725,9 +725,11 @@ export function runTrackedSelection( } /** - * The record that decides a `nodeChanged` from `changedRawNode`: the node's - * own, or else the nearest ancestor record whose tree version the run read, - * which re-reads that version. Undefined when the run read neither. + * The record that decides a `nodeChanged` from `changedRawNode`: the nearest + * record, the node's own included, whose tree version the run read, which + * re-reads that version; else the node's own. A tree version read wins over + * the node's own record, which may skip a key it never read even though the + * write moved the tree version. Undefined when the run read neither. */ export function resolveChangedReadRecord( reads: ReadonlyMap, @@ -735,7 +737,7 @@ export function resolveChangedReadRecord( changedRawNode: TreeNode ): NodeReadRecord | undefined { const record = reads.get(changedRawNode); - if (record !== undefined || subtreeReads.length === 0) { + if (subtreeReads.length === 0 || record?.subtreeRead) { return record; } let handler = getBaseHandlerForUnproxiedNode(changedRawNode)?.parentHandler; @@ -746,7 +748,7 @@ export function resolveChangedReadRecord( } handler = handler.parentHandler; } - return undefined; + return record; } /** diff --git a/packages/retree-core/src/node-version.spec.ts b/packages/retree-core/src/node-version.spec.ts index 1d552cb..5ac2efc 100644 --- a/packages/retree-core/src/node-version.spec.ts +++ b/packages/retree-core/src/node-version.spec.ts @@ -192,6 +192,47 @@ describe("tracked version reads", () => { stop(); }); + it("re-runs on an unread field of a node also read beneath a tree version read", () => { + class Shelf extends ReactiveNode { + public books = [{ title: "a", tags: ["x"] }]; + get dependencies() { + return []; + } + @select + get summary(): string { + return `${Retree.treeVersion(this.books)}:${ + this.books[0].title + }`; + } + } + const root = Retree.root({ rows: [{ title: "a", done: false }] }); + const shelf = Retree.root(new Shelf()); + roots.push(root, shelf); + const selected: string[] = []; + const stopSelect = Retree.select( + () => `${Retree.treeVersion(root.rows)}:${root.rows[0].title}`, + (value) => selected.push(value) + ); + let effectRuns = 0; + const stopEffect = Retree.effect(() => { + effectRuns++; + Retree.treeVersion(root.rows); + void root.rows[0].title; + }); + void shelf.summary; + const changed = vi.fn(); + Retree.on(shelf, "nodeChanged", changed); + + root.rows[0].done = true; + shelf.books[0].tags = ["z"]; + + expect(selected).toHaveLength(1); + expect(effectRuns).toBe(2); + expect(changed).toHaveBeenCalledTimes(1); + stopSelect(); + stopEffect(); + }); + it("re-runs a @select getter on writes under a tree version read", () => { const root = Retree.root({ library: new Library(), diff --git a/packages/retree-react/src/internals/subscriptionHub.spec.ts b/packages/retree-react/src/internals/subscriptionHub.spec.ts index b8b463e..242e5f5 100644 --- a/packages/retree-react/src/internals/subscriptionHub.spec.ts +++ b/packages/retree-react/src/internals/subscriptionHub.spec.ts @@ -4,10 +4,49 @@ */ import { describe, expect, it, vi } from "vitest"; -import { Retree } from "@retreejs/core"; +import { ReactiveNode, Retree } from "@retreejs/core"; import { subscribeToNode } from "./subscriptionHub.js"; +class LifecycleNode extends ReactiveNode { + public count = 0; + public observed = 0; + public unobserved = 0; + get dependencies() { + return []; + } + protected onObserved(): void { + this.observed++; + } + protected onUnobserved(): void { + this.unobserved++; + } +} + describe("subscriptionHub", () => { + it("defers a ReactiveNode teardown so a same-tick resubscribe keeps it observed", async () => { + const node = Retree.root(new LifecycleNode()); + const listener = vi.fn(); + subscribeToNode(node, "nodeChanged", vi.fn())(); + subscribeToNode(node, "nodeChanged", vi.fn())(); + const unsubscribe = subscribeToNode(node, "nodeChanged", listener); + await Promise.resolve(); + + expect(node.observed).toBe(1); + expect(node.unobserved).toBe(0); + node.count = 1; + expect(listener).toHaveBeenCalledTimes(1); + + unsubscribe(); + expect(node.unobserved).toBe(0); + await Promise.resolve(); + expect(node.unobserved).toBe(1); + + subscribeToNode(node, "nodeChanged", vi.fn())(); + await Promise.resolve(); + expect(node.observed).toBe(2); + expect(node.unobserved).toBe(2); + }); + it("does not remove another listener when unsubscribe is called twice", () => { const root = Retree.root({ count: 0 }); const listenerA = vi.fn(); diff --git a/packages/retree-react/src/useSelect.spec.tsx b/packages/retree-react/src/useSelect.spec.tsx index 5a41879..7d8d26f 100644 --- a/packages/retree-react/src/useSelect.spec.tsx +++ b/packages/retree-react/src/useSelect.spec.tsx @@ -282,6 +282,33 @@ describe("useSelect", () => { expect(screen.getByTestId("value").textContent).toBe("1"); }); + it("re-renders a tracked treeVersion read on a descendant write", () => { + const root = trackRoot( + Retree.root({ list: [{ done: false }], label: "one" }) + ); + let renderCount = 0; + + function Probe() { + renderCount += 1; + const version = useSelect(() => Retree.treeVersion(root.list)); + return
{version}
; + } + + render(); + const initial = screen.getByTestId("value").textContent; + + act(() => { + root.label = "two"; + }); + expect(renderCount).toBe(1); + + act(() => { + root.list[0].done = true; + }); + expect(screen.getByTestId("value").textContent).not.toBe(initial); + expect(renderCount).toBe(2); + }); + it("selects with trapped dependencies when only a selector function is passed", () => { const root = trackRoot( Retree.root({ diff --git a/packages/retree-react/src/useSelect.ts b/packages/retree-react/src/useSelect.ts index 4e37774..adedb40 100644 --- a/packages/retree-react/src/useSelect.ts +++ b/packages/retree-react/src/useSelect.ts @@ -22,6 +22,7 @@ import { hasTrackedSelectionChanged, normalizeDependencyEntry, normalizeSelectDependencies, + resolveChangedReadRecord, runTrackedSelection, stabilizeSelectedRetreeReferences, TrackedSelection, @@ -191,7 +192,8 @@ function getTrackedListenerType( /** * True when a moved source can still affect the selection. With the changed - * nodes known, only their records validate (nodes without a record are + * nodes known, only their records validate, or the nearest ancestor whose + * tree version the selector read (other nodes without a record are * descendants of a subtree cover the selector never read). Without them every * record under a moved source re-reads; validation is raw reads, far cheaper * than re-running the selector. @@ -204,7 +206,11 @@ function isTrackedSnapshotChangeRelevant( const changes = snapshot.changes; if (changes !== undefined) { for (const change of changes) { - const record = reads.get(change.rawNode); + const record = resolveChangedReadRecord( + reads, + state.subtreeReads, + change.rawNode + ); if (record === undefined) { continue; } @@ -482,6 +488,7 @@ function recomputeNodeSelectStateForSelector( */ interface TrackedSelectState { reads: TrackedSelection["reads"]; + subtreeReads: TrackedSelection["subtreeReads"]; sources: readonly RetreeExternalStoreSource[]; /** * Swappable for the same reason as {@link NodeSelectState.store}: a @@ -501,6 +508,7 @@ function createTrackedSelectState( const store = createRetreeSwappableCompositeExternalStore(sources); return { reads: selection.reads, + subtreeReads: selection.subtreeReads, sources, store, snapshot: store.getSnapshot(), @@ -524,6 +532,7 @@ function refreshTrackedSelectState( const nextSelection = runTrackedSelection(selector); const previousReads = state.reads; state.reads = nextSelection.reads; + state.subtreeReads = nextSelection.subtreeReads; const stabilizedSelected = stabilizeSelectedRetreeReferences( state.container.selected, nextSelection.selected @@ -616,6 +625,7 @@ function recomputeTrackedSelectStateForSelector( } return { reads: nextSelection.reads, + subtreeReads: nextSelection.subtreeReads, sources, store, snapshot: store.getSnapshot(),