diff --git a/.changeset/query-detach-shared-results.md b/.changeset/query-detach-shared-results.md new file mode 100644 index 0000000..5208dfe --- /dev/null +++ b/.changeset/query-detach-shared-results.md @@ -0,0 +1,10 @@ +--- +"@retreejs/query": patch +"@retreejs/convex": patch +--- + +Queries no longer adopt the objects a source emits, so two queries can share one cached result. + +Two queries that received the same cached result threw "Retree cannot assign this node because it already has a structural parent". Rows added during reconciliation were also shared between queries, so editing one query changed the other. Query state now holds its own copies, including of `initialState`. Frozen objects and opaque built-ins are still shared, and reconciliation copies only the rows and fields it writes. + +Custom reconcilers should not write objects from `next` into `current` directly. Use `reconcileArray`, or return a replacement value; it is copied, except for rows reused from `current`. diff --git a/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts b/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts index 66ca57d..a37205e 100644 --- a/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts +++ b/packages/retree-convex/src/ConvexPaginatedQueryNode.optimistic.spec.ts @@ -437,3 +437,23 @@ describe("ConvexPaginatedQueryNode optimistic updates", () => { ); }); }); + +describe("ConvexPaginatedQueryNode shared pages", () => { + it("loads one cached page into two queries without sharing rows", () => { + const client = new FakePaginatedConvexClient(); + const first = subscribePaginatedNode(client); + const second = subscribePaginatedNode(client); + const page: RetreePaginatedQueryResult = { + results: [{ _id: "task-1", text: "Server", isCompleted: false }], + status: "CanLoadMore", + loadMore: () => true, + }; + + client.paginatedSubscriptions[0].callback(page); + client.paginatedSubscriptions[1].callback(page); + first.state!.results[0].text = "Edited"; + + expect(second.state?.results[0].text).toBe("Server"); + expect(page.results[0].text).toBe("Server"); + }); +}); diff --git a/packages/retree-convex/src/ConvexPaginatedQueryNode.ts b/packages/retree-convex/src/ConvexPaginatedQueryNode.ts index 60fec9a..1d32b0d 100644 --- a/packages/retree-convex/src/ConvexPaginatedQueryNode.ts +++ b/packages/retree-convex/src/ConvexPaginatedQueryNode.ts @@ -259,14 +259,14 @@ export class ConvexPaginatedQueryNode< const current = this.state; if (current === undefined) { - this.state = next; + this.state = this.detach(next); return; } if (!tryReconcileConvexDocuments(current.results, next.results)) { // Rows without `_id` cannot be reconciled by identity; replace the // loaded rows wholesale like the non-paginated node does. - current.results = next.results; + current.results = this.detach(next.results); } // Page bookkeeping: compare against the raw view so the function diff --git a/packages/retree-core/src/internals/index.ts b/packages/retree-core/src/internals/index.ts index 40e683a..f4497ea 100644 --- a/packages/retree-core/src/internals/index.ts +++ b/packages/retree-core/src/internals/index.ts @@ -6,6 +6,7 @@ export * from "./TypedEventEmitter.js"; export * from "./dependencies.js"; export * from "./dependency-tracking.js"; +export { isLeafObject } from "./leaves.js"; export * from "./proxy.js"; export * from "./reproxy.js"; export * from "./select.js"; diff --git a/packages/retree-query/src/QueryNode.spec.ts b/packages/retree-query/src/QueryNode.spec.ts index cc3a0eb..d17ec47 100644 --- a/packages/retree-query/src/QueryNode.spec.ts +++ b/packages/retree-query/src/QueryNode.spec.ts @@ -2,7 +2,11 @@ import { describe, expect, it, vi } from "vitest"; import { Retree } from "@retreejs/core"; import { QueryNode } from "./QueryNode.js"; import { reconcileArrayById } from "./reconcile.js"; -import { IQuerySubscriptionSource, IQuerySubscriptionHandle } from "./types.js"; +import { + IQuerySubscriptionSource, + IQuerySubscriptionHandle, + IStateReconciler, +} from "./types.js"; interface ITaskArgs { listId: string; @@ -14,6 +18,11 @@ interface ITask { isCompleted: boolean; } +interface ITaskWithMeta { + id: string; + meta?: { owner: string }; +} + class FakeQuerySource implements IQuerySubscriptionSource { @@ -632,3 +641,190 @@ describe("QueryNode optimistic updates", () => { ); }); }); + +describe("QueryNode shared source values", () => { + function subscribeTwo( + source: FakeQuerySource, + reconcile?: IStateReconciler + ) { + const nodes = [0, 1].map(() => + Retree.root( + new QueryNode(source, { args: { listId: "today" }, reconcile }) + ) + ); + for (const node of nodes) { + Retree.on(node, "nodeChanged", () => undefined); + } + return nodes; + } + + it("observes one cached result from two queries without sharing it", () => { + const source = new FakeQuerySource(); + const cached = [{ id: "task-1", text: "Server", isCompleted: false }]; + source.nextCurrentValue = cached; + const [first, second] = subscribeTwo(source); + + first.state![0].text = "Edited"; + + expect(first.state![0].text).toBe("Edited"); + expect(second.state![0].text).toBe("Server"); + expect(cached[0].text).toBe("Server"); + }); + + it("copies rows that reconciliation inserts", () => { + const source = new FakeQuerySource(); + const [first, second] = subscribeTwo(source, reconcileArrayById("id")); + const initial = { id: "task-1", text: "First", isCompleted: false }; + source.subscriptions[0].onValue([{ ...initial }]); + source.subscriptions[1].onValue([{ ...initial }]); + + const next = [ + initial, + { id: "task-2", text: "Second", isCompleted: false }, + ]; + source.subscriptions[0].onValue(next); + source.subscriptions[1].onValue(next); + first.state![1].text = "Edited"; + + expect(second.state![1].text).toBe("Second"); + expect(next[1].text).toBe("Second"); + }); + + it("copies objects that reconciliation writes into existing rows", () => { + const source = new FakeQuerySource(); + const nodes = [0, 1].map(() => + Retree.root( + new QueryNode(source, { + args: { listId: "today" }, + reconcile: reconcileArrayById("id"), + }) + ) + ); + for (const node of nodes) { + Retree.on(node, "nodeChanged", () => undefined); + } + source.subscriptions[0].onValue([{ id: "task-1" }]); + source.subscriptions[1].onValue([{ id: "task-1" }]); + + const next = [{ id: "task-1", meta: { owner: "Ada" } }]; + source.subscriptions[0].onValue(next); + source.subscriptions[1].onValue(next); + nodes[0].state![0].meta!.owner = "Grace"; + + expect(nodes[1].state![0].meta?.owner).toBe("Ada"); + expect(next[0].meta.owner).toBe("Ada"); + }); + + it("copies a replacement value but keeps rows it reuses", () => { + const source = new FakeQuerySource(); + const replaceById: IStateReconciler = { + reconcile(current, next) { + return next.map( + (task) => current?.find((row) => row.id === task.id) ?? task + ); + }, + }; + const [first, second] = subscribeTwo(source, replaceById); + const initial = { id: "task-1", text: "First", isCompleted: false }; + source.subscriptions[0].onValue([{ ...initial }]); + source.subscriptions[1].onValue([{ ...initial }]); + const row = first.state![0]; + + const next = [ + initial, + { id: "task-2", text: "Second", isCompleted: false }, + ]; + source.subscriptions[0].onValue(next); + source.subscriptions[1].onValue(next); + first.state![1].text = "Edited"; + + expect(first.state![0]).toBe(row); + expect(second.state![1].text).toBe("Second"); + }); + + it("rolls back when a custom reconciler adopts emitted rows", async () => { + const source = new FakeQuerySource(); + const adoptRows: IStateReconciler = { + reconcile(current, next) { + if (current === undefined) return next; + next.forEach((task, index) => { + current[index] = task; + }); + return current; + }, + }; + const node = Retree.root( + new QueryNode(source, { + args: { listId: "today" }, + reconcile: adoptRows, + }) + ); + Retree.on(node, "nodeChanged", () => undefined); + source.subscriptions[0].onValue([ + { id: "task-1", text: "Server", isCompleted: false }, + ]); + source.subscriptions[0].onValue([ + { id: "task-1", text: "Server", isCompleted: true }, + ]); + const mutation = Promise.reject(new Error("Mutation failed")); + + node.optimisticUpdate({ + ctx: { promise: mutation }, + apply(tasks) { + tasks[0].text = "Optimistic"; + }, + }); + await mutation.catch(() => undefined); + + expect(node.state![0].text).toBe("Server"); + }); + + it("gives each query its own copy of a shared initial state", () => { + const source = new FakeQuerySource(); + const empty: ITask[] = []; + const [first, second] = [0, 1].map(() => + Retree.root( + new QueryNode(source, { + args: { listId: "today" }, + initialState: empty, + }) + ) + ); + + first.state!.push({ id: "task-1", text: "Local", isCompleted: false }); + + expect(second.state).toEqual([]); + expect(empty).toEqual([]); + }); + + it("keeps state when the source repeats the emission it mirrors", () => { + const source = new FakeQuerySource(); + const cached = [{ id: "task-1", text: "Server", isCompleted: false }]; + source.nextCurrentValue = cached; + const node = Retree.root( + new QueryNode(source, { args: { listId: "today" } }) + ); + Retree.on(node, "nodeChanged", () => undefined)(); + const state = node.state; + + Retree.on(node, "nodeChanged", () => undefined); + + expect(node.state).toBe(state); + }); + + it("shares frozen leaves instead of copying them", () => { + const source = new FakeQuerySource(); + const task = Object.freeze({ + id: "task-1", + text: "Server", + isCompleted: false, + }); + source.nextCurrentValue = { task }; + const node = Retree.root( + new QueryNode(source, { args: { listId: "today" } }) + ); + Retree.on(node, "nodeChanged", () => undefined); + + expect(node.state?.task).toBe(task); + }); +}); diff --git a/packages/retree-query/src/QueryNode.ts b/packages/retree-query/src/QueryNode.ts index 601a07a..ce6cbc7 100644 --- a/packages/retree-query/src/QueryNode.ts +++ b/packages/retree-query/src/QueryNode.ts @@ -5,6 +5,7 @@ import { ignore, ReactiveNode, Retree, TreeNode } from "@retreejs/core"; import { getUnproxiedNode } from "@retreejs/core/internal"; +import { detach } from "./internals/detach.js"; import { isDevMode } from "./internals/env.js"; import { deepEquals } from "./internals/equality.js"; import { @@ -96,6 +97,12 @@ export class QueryNode extends ReactiveNode { */ @ignore private lastEmittedState: TState | undefined; + /** + * Emission that `state` was last restored from and still mirrors, so a + * repeated emission (such as a cached value on resubscribe) skips the copy. + */ + @ignore + private stateSource: TState | undefined; /** * True while local optimistic state differs from the last clean server * baseline and may still need confirmation or rollback. @@ -151,9 +158,9 @@ export class QueryNode extends ReactiveNode { this.args = getInitialArgs(options, queryOptions); this.reconciler = queryOptions?.reconcile; this.keepPreviousData = queryOptions?.keepPreviousData ?? false; - this.state = queryOptions?.initialState; + this.state = this.detach(queryOptions?.initialState); this.result = getInitialResult(options, this.state); - this.lastEmittedState = this.cloneState(this.state); + this.lastEmittedState = queryOptions?.initialState; } get dependencies() { @@ -392,21 +399,10 @@ export class QueryNode extends ReactiveNode { return; } - // Clone at rejection time so the rollback restores the latest - // clean server baseline — including confirmations of older - // mutations that advanced `lastEmittedState` mid-window — and - // so the baseline never aliases objects written into managed - // state during the restore. - const rollbackState = this.cloneState(this.lastEmittedState); - if ( - this.state !== undefined && - rollbackState !== undefined && - transform.revert !== undefined - ) { - transform.revert(this.state, rollbackState); - } else { - this.restoreState(rollbackState); - } + // Restore the latest clean server baseline, including + // confirmations of older mutations that advanced + // `lastEmittedState` mid-window. + this.rollback(transform.revert); this.clearOptimisticDirty(); this.setResultFromState(); this.error = this.toRollbackError(error); @@ -414,9 +410,28 @@ export class QueryNode extends ReactiveNode { }); } + private rollback( + revert: IOptimisticQueryTransform["revert"] + ): void { + if (revert === undefined || this.state === undefined) { + // restoreState copies whatever it adopts from the baseline. + this.restoreState(this.lastEmittedState); + return; + } + + // The snapshot is the caller's to mutate or adopt. + const snapshot = this.cloneState(this.lastEmittedState); + if (snapshot === undefined) { + this.restoreState(undefined); + return; + } + revert(this.state, snapshot); + } + private setPending(): void { Retree.runTransaction(() => { this.state = undefined; + this.stateSource = undefined; this.error = null; this.result = { status: "pending" }; }); @@ -438,6 +453,7 @@ export class QueryNode extends ReactiveNode { private setSkipped(): void { Retree.runTransaction(() => { this.state = undefined; + this.stateSource = undefined; this.lastEmittedState = undefined; this.clearOptimisticDirty(); this.error = null; @@ -478,10 +494,13 @@ export class QueryNode extends ReactiveNode { } this.clearOptimisticDirty(); - this.restoreState(next); - // Store the emitted value by reference. Reconciliation may alias parts - // of `next` into managed state, so `markOptimisticDirty` detaches the - // baseline with a clone before any optimistic transform can mutate it. + // restoreState copies what it adopts, so `state` never aliases `next` + // and the baseline can keep the emitted value by reference. + const isRepeat = next !== undefined && next === this.stateSource; + if (!isRepeat) { + this.restoreState(next); + this.stateSource = next; + } this.lastEmittedState = next; this.setResultFromState(next); } @@ -509,6 +528,10 @@ export class QueryNode extends ReactiveNode { * replacing the state wholesale. Subclasses with non-plain state shapes * (for example paginated results carrying a `loadMore` function) override * this. + * + * `next` may be shared with other queries and the source's cache, so + * overrides must copy any object they write into `state` with + * {@link QueryNode.detach} or reconcile it with `reconcileArray`. */ protected restoreState(next: TState | undefined): void { if (next === undefined) { @@ -521,7 +544,7 @@ export class QueryNode extends ReactiveNode { return; } - this.state = next; + this.state = this.detach(next); return; } @@ -538,7 +561,15 @@ export class QueryNode extends ReactiveNode { return; } - this.state = reconciled; + this.state = this.detach(reconciled); + } + + /** + * Copy every object Retree would adopt from `value`, sharing leaves and + * functions. Use it in {@link QueryNode.restoreState} overrides. + */ + protected detach(value: T): T { + return detach(value); } /** @@ -610,13 +641,14 @@ export class QueryNode extends ReactiveNode { } private markOptimisticDirty(): number { - if (!this.isOptimisticDirty) { - this.isOptimisticDirty = true; - // The emitted baseline may alias objects that reconciliation wrote - // into managed state. Detach it now — before the first optimistic - // transform mutates state — so the rollback baseline stays clean. + if (!this.isOptimisticDirty && this.reconciler !== undefined) { + // A custom reconciler may have written baseline objects into + // `state`; detach the baseline before the transform mutates it. this.lastEmittedState = this.cloneState(this.lastEmittedState); } + // The transform is about to make `state` diverge from its source. + this.stateSource = undefined; + this.isOptimisticDirty = true; this.optimisticGeneration++; // The first generation in a dirty window is the oldest optimistic write diff --git a/packages/retree-query/src/internals/detach.ts b/packages/retree-query/src/internals/detach.ts new file mode 100644 index 0000000..528c465 --- /dev/null +++ b/packages/retree-query/src/internals/detach.ts @@ -0,0 +1,77 @@ +/*! + * Copyright (c) Ryan Bliss. All rights reserved. + * Licensed under the MIT License. + */ + +import { getUnproxiedNode, isLeafObject } from "@retreejs/core/internal"; + +/** + * Copy every object Retree would adopt as a structural child. + * + * @remarks + * Sources may hand the same object to several queries and keep it in their + * own cache, so query state must never adopt it. Leaves (frozen objects and + * opaque built-ins), functions, and nodes the tree already manages are + * shared. Copies keep own enumerable string keys, like the source data + * queries carry. + */ +export function detach(value: T): T { + return detachValue(value) as T; +} + +function detachValue(value: unknown): unknown { + if (value === null) return value; + if (typeof value !== "object") return value; + if (isLeafObject(value)) return value; + if (getUnproxiedNode(value) !== value) return value; + if (Array.isArray(value)) return value.map(detachValue); + if (isPlainRecord(value)) { + // Bracket access is ~3x faster than Reflect for payload-sized copies. + const copy: Record = {}; + for (const key of Object.keys(value)) { + setField(copy, key, detachValue(value[key])); + } + return copy; + } + if (value instanceof Date) return new Date(value.getTime()); + if (value instanceof Map) { + const copy = new Map(); + for (const [key, item] of value) copy.set(key, detachValue(item)); + return copy; + } + if (value instanceof Set) { + const copy = new Set(); + for (const item of value) copy.add(detachValue(item)); + return copy; + } + + const copy: Record = Object.create( + Object.getPrototypeOf(value) + ); + for (const key of Object.keys(value)) { + setField(copy, key, detachValue(Reflect.get(value, key))); + } + return copy; +} + +function isPlainRecord(value: object): value is Record { + return Object.getPrototypeOf(value) === Object.prototype; +} + +function setField( + copy: Record, + key: string, + value: unknown +): void { + if (key === "__proto__") { + // Assignment would invoke the prototype setter. + Object.defineProperty(copy, key, { + value, + writable: true, + enumerable: true, + configurable: true, + }); + return; + } + copy[key] = value; +} diff --git a/packages/retree-query/src/internals/reconcile.ts b/packages/retree-query/src/internals/reconcile.ts index 48d7b31..ae1f528 100644 --- a/packages/retree-query/src/internals/reconcile.ts +++ b/packages/retree-query/src/internals/reconcile.ts @@ -5,6 +5,7 @@ import { getUnproxiedNode } from "@retreejs/core/internal"; import { TreeNode } from "@retreejs/core"; +import { detach } from "./detach.js"; import { deepEquals } from "./equality.js"; function unwrapRaw(value: T): T { @@ -42,6 +43,13 @@ export function tryReconcileDocumentsById( return true; } +/** + * Reconcile `next` into `current` by item id, writing only the diffs. + * + * @remarks + * Objects written into `current` are copies, so `next` stays free for other + * queries and the source's cache. + */ export function reconcileArray( current: TItem[], next: TItem[], @@ -96,7 +104,7 @@ export function reconcileArray( const nextId = getId(nextItem); const currentItem = currentById.get(nextId); if (currentItem === undefined) { - current[index] = nextItem; + current[index] = detach(nextItem); continue; } @@ -243,7 +251,7 @@ function reconcileObject( if (deepEquals(rawValue, value) && Object.hasOwn(target.raw, key)) { continue; } - Reflect.set(getManagedTarget(target), key, value); + Reflect.set(getManagedTarget(target), key, detach(value)); } } diff --git a/packages/retree-query/src/types.ts b/packages/retree-query/src/types.ts index fdc82fd..ca3611e 100644 --- a/packages/retree-query/src/types.ts +++ b/packages/retree-query/src/types.ts @@ -105,6 +105,11 @@ export interface IStateReconciler { * changed rows emit `nodeChanged` and item identity stays stable for * `useNode` rows; writes to `rawCurrent` skip emission entirely. * + * `next` may be shared with other queries and the source's cache, so + * never write its objects into `current` directly. `reconcileArray` + * copies what it writes, and a returned replacement value is copied + * except for rows it reuses from `current`. + * * @param current Current query state, if any. Write surface. * @param next Newly emitted query state (raw server data). * @param rawCurrent Raw view of `current` for fast reads; `undefined`