diff --git a/.changeset/tidy-pandas-invite.md b/.changeset/tidy-pandas-invite.md new file mode 100644 index 0000000000..0fadb2f508 --- /dev/null +++ b/.changeset/tidy-pandas-invite.md @@ -0,0 +1,5 @@ +--- +'@tanstack/table-core': patch +--- + +fix: invalidate cached row values when a column's `accessorFn` changes. Rows are memoized on `data`, so replacing column definitions kept serving stale `getValue()` results. The cache now records the accessor identity and recomputes when it changes. diff --git a/packages/table-core/src/core/rows/constructRow.ts b/packages/table-core/src/core/rows/constructRow.ts index 9fecbb23ed..aa328d868d 100644 --- a/packages/table-core/src/core/rows/constructRow.ts +++ b/packages/table-core/src/core/rows/constructRow.ts @@ -49,6 +49,7 @@ export const constructRow = < // Only assign instance-specific properties row._displayIndexCache = -1 + row._accessorFnsCache = makeObjectMap() row._uniqueValuesCache = makeObjectMap() row._valuesCache = makeObjectMap() row.depth = depth diff --git a/packages/table-core/src/core/rows/coreRowsFeature.types.ts b/packages/table-core/src/core/rows/coreRowsFeature.types.ts index 8a38bed8c9..2c3766b637 100644 --- a/packages/table-core/src/core/rows/coreRowsFeature.types.ts +++ b/packages/table-core/src/core/rows/coreRowsFeature.types.ts @@ -25,6 +25,18 @@ export interface Row_CoreProperties< _displayIndexCache: number _uniqueValuesCache: Record _valuesCache: Record + /** + * Records the column `accessorFn` identity that produced each entry in + * `_valuesCache`. + * + * Rows are rebuilt when `data` changes, but column definitions can be + * replaced independently. A cached value is only served while the column's + * current accessor is the same function that produced it, so updating + * column defs invalidates stale values instead of returning them. + * + * @internal + */ + _accessorFnsCache: Record /** * The depth of the row (if nested or grouped) relative to the root row array. */ diff --git a/packages/table-core/src/core/rows/coreRowsFeature.utils.ts b/packages/table-core/src/core/rows/coreRowsFeature.utils.ts index f37ce79828..348cf859bf 100644 --- a/packages/table-core/src/core/rows/coreRowsFeature.utils.ts +++ b/packages/table-core/src/core/rows/coreRowsFeature.utils.ts @@ -68,7 +68,9 @@ export function table_getRowsInDisplayOrder< * Reads and caches this row's value for a column. * * The value is produced by the column accessor. Missing columns or display - * columns without an accessor return `undefined`. + * columns without an accessor return `undefined`. The cached value is only + * reused while the column's accessor function is unchanged; replacing the + * column definitions with a new accessor invalidates the cache. * * @example * ```ts @@ -79,19 +81,30 @@ export function row_getValue< TFeatures extends TableFeatures, TData extends RowData, >(row: Row, columnId: string) { - if (hasOwn(row._valuesCache, columnId)) { - return row._valuesCache[columnId] - } - const column = row.table.getColumn(columnId) if (!column?.accessorFn) { return undefined } - row._valuesCache[columnId] = column.accessorFn(row.original, row.index) + // Rows are rebuilt when `data` changes, but column definitions can be + // replaced independently. Compare the accessor identity so a value cached + // under a previous accessor is recomputed instead of served stale. + if ( + hasOwn(row._valuesCache, columnId) && + row._accessorFnsCache[columnId] === column.accessorFn + ) { + return row._valuesCache[columnId] + } + + // Evaluate the accessor before touching either cache entry: if it throws, + // both entries keep their previous state so a later call retries the + // accessor instead of serving a stale value. + const value = column.accessorFn(row.original, row.index) + row._accessorFnsCache[columnId] = column.accessorFn + row._valuesCache[columnId] = value - return row._valuesCache[columnId] + return value } /** diff --git a/packages/table-core/tests/unit/core/rows/coreRowsFeature.utils.test.ts b/packages/table-core/tests/unit/core/rows/coreRowsFeature.utils.test.ts index 05d2971736..e240c66884 100644 --- a/packages/table-core/tests/unit/core/rows/coreRowsFeature.utils.test.ts +++ b/packages/table-core/tests/unit/core/rows/coreRowsFeature.utils.test.ts @@ -113,6 +113,79 @@ describe('row_getValue', () => { expect(row_getValue(row, 'not-a-column')).toBeUndefined() }) + + it('should invalidate the cached value when the column accessor changes', () => { + const data = generateTestData(1) + const table = constructTable({ + features, + data, + columns: [{ id: 'derived', accessorFn: () => 'a' }], + }) + const row = table.getRowModel().rows[0]! + + expect(row_getValue(row, 'derived')).toBe('a') + + // Rows are memoized on `data`, so replacing the column defs keeps the + // same row instances; the value cache must still pick up the new accessor. + table.setOptions((old) => ({ + ...old, + columns: [{ id: 'derived', accessorFn: () => 'b' }], + })) + + expect(row_getValue(row, 'derived')).toBe('b') + }) + + it('should not recompute the value while the accessor is unchanged', () => { + const data = generateTestData(1) + let calls = 0 + const table = constructTable({ + features, + data, + columns: [ + { + id: 'derived', + accessorFn: () => { + calls++ + return 'a' + }, + }, + ], + }) + const row = table.getRowModel().rows[0]! + + expect(row_getValue(row, 'derived')).toBe('a') + expect(row_getValue(row, 'derived')).toBe('a') + expect(calls).toBe(1) + }) + + it('should retry the accessor instead of serving a stale value after it throws', () => { + const data = generateTestData(1) + const table = constructTable({ + features, + data, + columns: [{ id: 'derived', accessorFn: () => 'a' }], + }) + const row = table.getRowModel().rows[0]! + + expect(row_getValue(row, 'derived')).toBe('a') + + let calls = 0 + const flaky = () => { + calls++ + if (calls === 1) throw new Error('boom') + return 'b' + } + table.setOptions((old) => ({ + ...old, + columns: [{ id: 'derived', accessorFn: flaky }], + })) + + expect(() => row_getValue(row, 'derived')).toThrow('boom') + // The failed attempt must not poison the cache: the next call retries + // the accessor instead of returning the stale 'a'. + expect(row_getValue(row, 'derived')).toBe('b') + expect(calls).toBe(2) + }) }) describe('row_getUniqueValues', () => {