diff --git a/.changeset/fix-array-iteration-proxy.md b/.changeset/fix-array-iteration-proxy.md new file mode 100644 index 000000000..34cdfa11b --- /dev/null +++ b/.changeset/fix-array-iteration-proxy.md @@ -0,0 +1,20 @@ +--- +"@tanstack/db": patch +--- + +Fix change tracking for array items accessed via iteration methods (find, forEach, for...of, etc.) + +Previously, modifications to array items retrieved via iteration methods were not tracked by the change proxy because these methods returned raw array elements instead of proxied versions. This caused `getChanges()` to return an empty object, which in turn caused `createOptimisticAction`'s `mutationFn` to never be called when using patterns like: + +```ts +collection.update(id, (draft) => { + const item = draft.items.find((x) => x.id === targetId) + if (item) { + item.value = newValue // This change was not tracked! + } +}) +``` + +The fix adds proxy handling for array iteration methods similar to how Map/Set iteration is already handled, ensuring that callbacks receive proxied elements and returned elements are properly proxied. + +Also refactors proxy.ts for improved readability by extracting helper functions and hoisting constants to module scope. diff --git a/packages/db/src/proxy.ts b/packages/db/src/proxy.ts index baffc1743..ca5a5d8d1 100644 --- a/packages/db/src/proxy.ts +++ b/packages/db/src/proxy.ts @@ -5,6 +5,453 @@ import { deepEquals, isTemporal } from "./utils" +/** + * Set of array methods that iterate with callbacks and may return elements. + * Hoisted to module scope to avoid creating a new Set on every property access. + */ +const CALLBACK_ITERATION_METHODS = new Set([ + `find`, + `findLast`, + `findIndex`, + `findLastIndex`, + `filter`, + `map`, + `flatMap`, + `forEach`, + `some`, + `every`, + `reduce`, + `reduceRight`, +]) + +/** + * Set of array methods that modify the array in place. + */ +const ARRAY_MODIFYING_METHODS = new Set([ + `pop`, + `push`, + `shift`, + `unshift`, + `splice`, + `sort`, + `reverse`, + `fill`, + `copyWithin`, +]) + +/** + * Set of Map/Set methods that modify the collection in place. + */ +const MAP_SET_MODIFYING_METHODS = new Set([`set`, `delete`, `clear`, `add`]) + +/** + * Set of Map/Set iterator methods. + */ +const MAP_SET_ITERATOR_METHODS = new Set([ + `entries`, + `keys`, + `values`, + `forEach`, +]) + +/** + * Check if a value is a proxiable object (not Date, RegExp, or Temporal) + */ +function isProxiableObject( + value: unknown +): value is Record { + return ( + value !== null && + typeof value === `object` && + !((value as any) instanceof Date) && + !((value as any) instanceof RegExp) && + !isTemporal(value) + ) +} + +/** + * Creates a handler for array iteration methods that ensures proxied elements + * are passed to callbacks and returned from methods like find/filter. + */ +function createArrayIterationHandler( + methodName: string, + methodFn: (...args: Array) => unknown, + changeTracker: ChangeTracker, + memoizedCreateChangeProxy: ( + obj: Record, + parent?: { + tracker: ChangeTracker> + prop: string | symbol + } + ) => { proxy: Record } +): ((...args: Array) => unknown) | undefined { + if (!CALLBACK_ITERATION_METHODS.has(methodName)) { + return undefined + } + + return function (...args: Array) { + const callback = args[0] + if (typeof callback !== `function`) { + return methodFn.apply(changeTracker.copy_, args) + } + + // Create a helper to get proxied version of an array element + const getProxiedElement = (element: unknown, index: number): unknown => { + if (isProxiableObject(element)) { + const nestedParent = { + tracker: changeTracker as unknown as ChangeTracker< + Record + >, + prop: String(index), + } + const { proxy: elementProxy } = memoizedCreateChangeProxy( + element, + nestedParent + ) + return elementProxy + } + return element + } + + // Wrap the callback to pass proxied elements + const wrappedCallback = function ( + this: unknown, + element: unknown, + index: number, + array: unknown + ) { + const proxiedElement = getProxiedElement(element, index) + return callback.call(this, proxiedElement, index, array) + } + + // For reduce/reduceRight, the callback signature is different + if (methodName === `reduce` || methodName === `reduceRight`) { + const reduceCallback = function ( + this: unknown, + accumulator: unknown, + element: unknown, + index: number, + array: unknown + ) { + const proxiedElement = getProxiedElement(element, index) + return callback.call(this, accumulator, proxiedElement, index, array) + } + return methodFn.apply(changeTracker.copy_, [ + reduceCallback, + ...args.slice(1), + ]) + } + + const result = methodFn.apply(changeTracker.copy_, [ + wrappedCallback, + ...args.slice(1), + ]) + + // For find/findLast, proxy the returned element if it's an object + if ( + (methodName === `find` || methodName === `findLast`) && + result && + typeof result === `object` + ) { + const foundIndex = ( + changeTracker.copy_ as unknown as Array + ).indexOf(result) + if (foundIndex !== -1) { + return getProxiedElement(result, foundIndex) + } + } + + // For filter, proxy each element in the result array + if (methodName === `filter` && Array.isArray(result)) { + return result.map((element) => { + const originalIndex = ( + changeTracker.copy_ as unknown as Array + ).indexOf(element) + if (originalIndex !== -1) { + return getProxiedElement(element, originalIndex) + } + return element + }) + } + + return result + } +} + +/** + * Creates a Symbol.iterator handler for arrays that yields proxied elements. + */ +function createArrayIteratorHandler( + changeTracker: ChangeTracker, + memoizedCreateChangeProxy: ( + obj: Record, + parent?: { + tracker: ChangeTracker> + prop: string | symbol + } + ) => { proxy: Record } +): () => Iterator { + return function () { + const array = changeTracker.copy_ as unknown as Array + let index = 0 + + return { + next() { + if (index >= array.length) { + return { done: true, value: undefined } + } + + const element = array[index] + let proxiedElement = element + + if (isProxiableObject(element)) { + const nestedParent = { + tracker: changeTracker as unknown as ChangeTracker< + Record + >, + prop: String(index), + } + const { proxy: elementProxy } = memoizedCreateChangeProxy( + element, + nestedParent + ) + proxiedElement = elementProxy + } + + index++ + return { done: false, value: proxiedElement } + }, + [Symbol.iterator]() { + return this + }, + } + } +} + +/** + * Creates a wrapper for methods that modify a collection (array, Map, Set). + * The wrapper calls the method and marks the change tracker as modified. + */ +function createModifyingMethodHandler( + methodFn: (...args: Array) => unknown, + changeTracker: ChangeTracker, + markChanged: (tracker: ChangeTracker) => void +): (...args: Array) => unknown { + return function (...args: Array) { + const result = methodFn.apply(changeTracker.copy_, args) + markChanged(changeTracker) + return result + } +} + +/** + * Creates handlers for Map/Set iterator methods (entries, keys, values, forEach). + * Returns proxied values for iteration to enable change tracking. + */ +function createMapSetIteratorHandler( + methodName: string, + prop: string | symbol, + methodFn: (...args: Array) => unknown, + target: Map | Set, + changeTracker: ChangeTracker, + memoizedCreateChangeProxy: ( + obj: Record, + parent?: { + tracker: ChangeTracker> + prop: string | symbol + } + ) => { proxy: Record }, + markChanged: (tracker: ChangeTracker) => void +): ((...args: Array) => unknown) | undefined { + const isIteratorMethod = + MAP_SET_ITERATOR_METHODS.has(methodName) || prop === Symbol.iterator + + if (!isIteratorMethod) { + return undefined + } + + return function (this: unknown, ...args: Array) { + const result = methodFn.apply(changeTracker.copy_, args) + + // For forEach, wrap the callback to track changes + if (methodName === `forEach`) { + const callback = args[0] + if (typeof callback === `function`) { + const wrappedCallback = function ( + this: unknown, + value: unknown, + key: unknown, + collection: unknown + ) { + const cbresult = callback.call(this, value, key, collection) + markChanged(changeTracker) + return cbresult + } + return methodFn.apply(target, [wrappedCallback, ...args.slice(1)]) + } + } + + // For iterators (entries, keys, values, Symbol.iterator) + const isValueIterator = + methodName === `entries` || + methodName === `values` || + methodName === Symbol.iterator.toString() || + prop === Symbol.iterator + + if (isValueIterator) { + const originalIterator = result as Iterator + + // For values() iterator on Maps, create a value-to-key mapping + const valueToKeyMap = new Map() + if (methodName === `values` && target instanceof Map) { + for (const [key, mapValue] of ( + changeTracker.copy_ as unknown as Map + ).entries()) { + valueToKeyMap.set(mapValue, key) + } + } + + // For Set iterators, create an original-to-modified mapping + const originalToModifiedMap = new Map() + if (target instanceof Set) { + for (const setValue of ( + changeTracker.copy_ as unknown as Set + ).values()) { + originalToModifiedMap.set(setValue, setValue) + } + } + + // Return a wrapped iterator that proxies values + return { + next() { + const nextResult = originalIterator.next() + + if ( + !nextResult.done && + nextResult.value && + typeof nextResult.value === `object` + ) { + // For entries, the value is a [key, value] pair + if ( + methodName === `entries` && + Array.isArray(nextResult.value) && + nextResult.value.length === 2 + ) { + if ( + nextResult.value[1] && + typeof nextResult.value[1] === `object` + ) { + const mapKey = nextResult.value[0] + const mapParent = { + tracker: changeTracker as unknown as ChangeTracker< + Record + >, + prop: mapKey as string | symbol, + updateMap: (newValue: unknown) => { + if (changeTracker.copy_ instanceof Map) { + ;(changeTracker.copy_ as Map).set( + mapKey, + newValue + ) + } + }, + } + const { proxy: valueProxy } = memoizedCreateChangeProxy( + nextResult.value[1] as Record, + mapParent as unknown as { + tracker: ChangeTracker> + prop: string | symbol + } + ) + nextResult.value[1] = valueProxy + } + } else if ( + methodName === `values` || + methodName === Symbol.iterator.toString() || + prop === Symbol.iterator + ) { + // For Map values(), use the key mapping + if (methodName === `values` && target instanceof Map) { + const mapKey = valueToKeyMap.get(nextResult.value) + if (mapKey !== undefined) { + const mapParent = { + tracker: changeTracker as unknown as ChangeTracker< + Record + >, + prop: mapKey as string | symbol, + updateMap: (newValue: unknown) => { + if (changeTracker.copy_ instanceof Map) { + ;(changeTracker.copy_ as Map).set( + mapKey, + newValue + ) + } + }, + } + const { proxy: valueProxy } = memoizedCreateChangeProxy( + nextResult.value as Record, + mapParent as unknown as { + tracker: ChangeTracker> + prop: string | symbol + } + ) + nextResult.value = valueProxy + } + } else if (target instanceof Set) { + // For Set, track modifications + const setOriginalValue = nextResult.value + const setParent = { + tracker: changeTracker as unknown as ChangeTracker< + Record + >, + prop: setOriginalValue as unknown as string | symbol, + updateSet: (newValue: unknown) => { + if (changeTracker.copy_ instanceof Set) { + ;(changeTracker.copy_ as Set).delete( + setOriginalValue + ) + ;(changeTracker.copy_ as Set).add(newValue) + originalToModifiedMap.set(setOriginalValue, newValue) + } + }, + } + const { proxy: valueProxy } = memoizedCreateChangeProxy( + nextResult.value as Record, + setParent as unknown as { + tracker: ChangeTracker> + prop: string | symbol + } + ) + nextResult.value = valueProxy + } else { + // For other cases, use a symbol placeholder + const tempKey = Symbol(`iterator-value`) + const { proxy: valueProxy } = memoizedCreateChangeProxy( + nextResult.value as Record, + { + tracker: changeTracker as unknown as ChangeTracker< + Record + >, + prop: tempKey, + } + ) + nextResult.value = valueProxy + } + } + } + + return nextResult + }, + [Symbol.iterator]() { + return this + }, + } + } + + return result + } +} + /** * Simple debug utility that only logs when debug mode is enabled * Set DEBUG to true in localStorage to enable debug logging @@ -392,271 +839,66 @@ export function createChangeProxy< // For Array methods that modify the array if (Array.isArray(ptarget)) { const methodName = prop.toString() - const modifyingMethods = new Set([ - `pop`, - `push`, - `shift`, - `unshift`, - `splice`, - `sort`, - `reverse`, - `fill`, - `copyWithin`, - ]) - - if (modifyingMethods.has(methodName)) { - return function (...args: Array) { - const result = value.apply(changeTracker.copy_, args) - markChanged(changeTracker) - return result - } + + if (ARRAY_MODIFYING_METHODS.has(methodName)) { + return createModifyingMethodHandler( + value, + changeTracker, + markChanged + ) + } + + // Handle array iteration methods (find, filter, forEach, etc.) + const iterationHandler = createArrayIterationHandler( + methodName, + value, + changeTracker, + memoizedCreateChangeProxy + ) + if (iterationHandler) { + return iterationHandler + } + + // Handle array Symbol.iterator for for...of loops + if (prop === Symbol.iterator) { + return createArrayIteratorHandler( + changeTracker, + memoizedCreateChangeProxy + ) } } // For Map and Set methods that modify the collection if (ptarget instanceof Map || ptarget instanceof Set) { const methodName = prop.toString() - const modifyingMethods = new Set([ - `set`, - `delete`, - `clear`, - `add`, - `pop`, - `push`, - `shift`, - `unshift`, - `splice`, - `sort`, - `reverse`, - ]) - - if (modifyingMethods.has(methodName)) { - return function (...args: Array) { - const result = value.apply(changeTracker.copy_, args) - markChanged(changeTracker) - return result - } + + if (MAP_SET_MODIFYING_METHODS.has(methodName)) { + return createModifyingMethodHandler( + value, + changeTracker, + markChanged + ) } // Handle iterator methods for Map and Set - const iteratorMethods = new Set([ - `entries`, - `keys`, - `values`, - `forEach`, - Symbol.iterator, - ]) - - if (iteratorMethods.has(methodName) || prop === Symbol.iterator) { - return function (this: unknown, ...args: Array) { - const result = value.apply(changeTracker.copy_, args) - - // For forEach, we need to wrap the callback to track changes - if (methodName === `forEach`) { - const callback = args[0] - if (typeof callback === `function`) { - // Replace the original callback with our wrapped version - const wrappedCallback = function ( - this: unknown, - // eslint-disable-next-line - value: unknown, - key: unknown, - collection: unknown - ) { - // Call the original callback - const cbresult = callback.call( - this, - value, - key, - collection - ) - // Mark as changed since the callback might have modified the value - markChanged(changeTracker) - return cbresult - } - // Call forEach with our wrapped callback - return value.apply(ptarget, [ - wrappedCallback, - ...args.slice(1), - ]) - } - } - - // For iterators (entries, keys, values, Symbol.iterator) - if ( - methodName === `entries` || - methodName === `values` || - methodName === Symbol.iterator.toString() || - prop === Symbol.iterator - ) { - // If it's an iterator, we need to wrap the returned iterator - // to track changes when the values are accessed and potentially modified - const originalIterator = result - - // For values() iterator on Maps, we need to create a value-to-key mapping - const valueToKeyMap = new Map() - if (methodName === `values` && ptarget instanceof Map) { - // Build a mapping from value to key for reverse lookup - // Use the copy_ (which is the current state) to build the mapping - for (const [ - key, - mapValue, - ] of changeTracker.copy_.entries()) { - valueToKeyMap.set(mapValue, key) - } - } - - // For Set iterators, we need to create an original-to-modified mapping - const originalToModifiedMap = new Map() - if (ptarget instanceof Set) { - // Initialize with original values - for (const setValue of changeTracker.copy_.values()) { - originalToModifiedMap.set(setValue, setValue) - } - } - - // Create a proxy for the iterator that will mark changes when next() is called - return { - next() { - const nextResult = originalIterator.next() - - // If we have a value and it's an object, we need to track it - if ( - !nextResult.done && - nextResult.value && - typeof nextResult.value === `object` - ) { - // For entries, the value is a [key, value] pair - if ( - methodName === `entries` && - Array.isArray(nextResult.value) && - nextResult.value.length === 2 - ) { - // The value is at index 1 in the [key, value] pair - if ( - nextResult.value[1] && - typeof nextResult.value[1] === `object` - ) { - const mapKey = nextResult.value[0] - // Create a special parent tracker that knows how to update the Map - const mapParent = { - tracker: changeTracker, - prop: mapKey, - updateMap: (newValue: unknown) => { - // Update the Map in the copy - if (changeTracker.copy_ instanceof Map) { - changeTracker.copy_.set(mapKey, newValue) - } - }, - } - - // Create a proxy for the value and replace it in the result - const { proxy: valueProxy } = - memoizedCreateChangeProxy( - nextResult.value[1], - mapParent - ) - nextResult.value[1] = valueProxy - } - } else if ( - methodName === `values` || - methodName === Symbol.iterator.toString() || - prop === Symbol.iterator - ) { - // If the value is an object, create a proxy for it - if ( - typeof nextResult.value === `object` && - nextResult.value !== null - ) { - // For Map values(), try to find the key using our mapping - if ( - methodName === `values` && - ptarget instanceof Map - ) { - const mapKey = valueToKeyMap.get(nextResult.value) - if (mapKey !== undefined) { - // Create a special parent tracker for this Map value - const mapParent = { - tracker: changeTracker, - prop: mapKey, - updateMap: (newValue: unknown) => { - // Update the Map in the copy - if (changeTracker.copy_ instanceof Map) { - changeTracker.copy_.set(mapKey, newValue) - } - }, - } - - const { proxy: valueProxy } = - memoizedCreateChangeProxy( - nextResult.value, - mapParent - ) - nextResult.value = valueProxy - } - } else if (ptarget instanceof Set) { - // For Set, we need to track modifications and update the Set accordingly - const setOriginalValue = nextResult.value - const setParent = { - tracker: changeTracker, - prop: setOriginalValue, // Use the original value as the prop - updateSet: (newValue: unknown) => { - // Update the Set in the copy by removing old value and adding new one - if (changeTracker.copy_ instanceof Set) { - changeTracker.copy_.delete(setOriginalValue) - changeTracker.copy_.add(newValue) - // Update our mapping for future iterations - originalToModifiedMap.set( - setOriginalValue, - newValue - ) - } - }, - } - - const { proxy: valueProxy } = - memoizedCreateChangeProxy( - nextResult.value, - setParent - ) - nextResult.value = valueProxy - } else { - // For other cases, use a symbol as a placeholder - const tempKey = Symbol(`iterator-value`) - const { proxy: valueProxy } = - memoizedCreateChangeProxy(nextResult.value, { - tracker: changeTracker, - prop: tempKey, - }) - nextResult.value = valueProxy - } - } - } - } - - return nextResult - }, - [Symbol.iterator]() { - return this - }, - } - } - - return result - } + const iteratorHandler = createMapSetIteratorHandler( + methodName, + prop, + value, + ptarget, + changeTracker, + memoizedCreateChangeProxy, + markChanged + ) + if (iteratorHandler) { + return iteratorHandler } } return value.bind(ptarget) } // If the value is an object (but not Date, RegExp, or Temporal), create a proxy for it - if ( - value && - typeof value === `object` && - !((value as any) instanceof Date) && - !((value as any) instanceof RegExp) && - !isTemporal(value) - ) { + if (isProxiableObject(value)) { // Create a parent reference for the nested object const nestedParent = { tracker: changeTracker, diff --git a/packages/db/tests/proxy.test.ts b/packages/db/tests/proxy.test.ts index f063d43a7..41bb0fee5 100644 --- a/packages/db/tests/proxy.test.ts +++ b/packages/db/tests/proxy.test.ts @@ -1653,5 +1653,166 @@ describe(`Proxy Library`, () => { }) expect(obj.date).toEqual(originalDate) }) + + describe(`array iteration methods`, () => { + it(`should track changes when modifying array items retrieved via find()`, () => { + const obj = { + job: { + orders: [ + { orderId: `order-1`, orderBinInt: 1 }, + { orderId: `order-2`, orderBinInt: 2 }, + ], + }, + } + const { proxy, getChanges } = createChangeProxy(obj) + + // Use find() to get an array item and modify it + const order = proxy.job.orders.find( + (order) => order.orderId === `order-1` + ) + if (order) { + order.orderBinInt = 99 + } + + const changes = getChanges() + expect(Object.keys(changes).length).toBeGreaterThan(0) + expect(changes.job?.orders?.[0]?.orderBinInt).toBe(99) + }) + + it(`should track changes when modifying array items via forEach`, () => { + const obj = { + items: [ + { id: 1, value: 10 }, + { id: 2, value: 20 }, + ], + } + const { proxy, getChanges } = createChangeProxy(obj) + + proxy.items.forEach((item) => { + item.value = item.value * 2 + }) + + const changes = getChanges() + expect(Object.keys(changes).length).toBeGreaterThan(0) + }) + + it(`should track changes when modifying array items via for...of`, () => { + const obj = { + items: [ + { id: 1, value: 10 }, + { id: 2, value: 20 }, + ], + } + const { proxy, getChanges } = createChangeProxy(obj) + + for (const item of proxy.items) { + item.value = item.value * 2 + } + + const changes = getChanges() + expect(Object.keys(changes).length).toBeGreaterThan(0) + }) + + it(`should track changes when modifying array items via index access`, () => { + const obj = { + items: [ + { id: 1, value: 10 }, + { id: 2, value: 20 }, + ], + } + const { proxy, getChanges } = createChangeProxy(obj) + + // Direct index access should work + const firstItem = proxy.items[0] + if (firstItem) { + firstItem.value = 100 + } + + const changes = getChanges() + expect(Object.keys(changes).length).toBeGreaterThan(0) + }) + + it(`should track changes when modifying items from filter() result`, () => { + const obj = { + items: [ + { id: 1, value: 10 }, + { id: 2, value: 20 }, + ], + } + const { proxy, getChanges } = createChangeProxy(obj) + + const filtered = proxy.items.filter((item) => item.id === 1) + const first = filtered[0] + if (first) { + first.value = 42 + } + + const changes = getChanges() + expect(changes.items?.[0]?.value).toBe(42) + }) + + it(`should track changes when modifying array items retrieved via findLast()`, () => { + const obj = { + job: { + orders: [ + { orderId: `order-1`, orderBinInt: 1 }, + { orderId: `order-2`, orderBinInt: 2 }, + ], + }, + } + const { proxy, getChanges } = createChangeProxy(obj) + + // Use type assertion to call findLast (ES2023 method) + type Order = { orderId: string; orderBinInt: number } + const orders = proxy.job.orders as unknown as { + findLast: (predicate: (o: Order) => boolean) => Order | undefined + } + const order = orders.findLast((o) => o.orderId.startsWith(`order-`)) + if (order) { + order.orderBinInt = 123 + } + + const changes = getChanges() + expect(changes.job?.orders?.[1]?.orderBinInt).toBe(123) + }) + + it(`should track changes when modifying array items inside some() callback`, () => { + const obj = { + items: [ + { id: 1, value: 10 }, + { id: 2, value: 20 }, + ], + } + const { proxy, getChanges } = createChangeProxy(obj) + + proxy.items.some((item) => { + item.value = item.value * 2 + return false + }) + + const changes = getChanges() + expect(changes.items?.[0]?.value).toBe(20) + expect(changes.items?.[1]?.value).toBe(40) + }) + + it(`should track changes when modifying array items inside reduce() callback`, () => { + const obj = { + items: [ + { id: 1, value: 10 }, + { id: 2, value: 20 }, + ], + } + const { proxy, getChanges } = createChangeProxy(obj) + + proxy.items.reduce((acc, item) => { + item.value = item.value + 1 + return acc + item.value + }, 0) + + const changes = getChanges() + expect(changes.items?.[0]?.value).toBe(11) + expect(changes.items?.[1]?.value).toBe(21) + }) + }) }) })