CellMeasurerCache.keyMapper is not used for List with rowHeights

[offsky]

It appears that CellMeasurerCache is always using the row index to store and retrieve its cached heights for rows when using it with a List that has dynamic heights, even if keyMapper is implemented.

If the rows get reordered, then the indexes stay the same even though the data and heights have changed. This results in the List getting the wrong heights. Even if you call List.recomputeRowHeights(), when the List asks the cache for the new height, the cache will use the row index to fetch the old wrong height.

A nuclear solution is to call CellMeasurerCache.clearAll() or you can do CellMeasurerCache.clear(2) for each row that that changed height, if you know this, which you may not.

The keyMapper option is supposed to fix this issue, by allowing the app to specify a better key than the row index, but this appears to only be implemented for a Grid. The rowHeight accessor and underlying setter are still using the row index.

Here is my naive solution, although this will certainly break the Grid.
https://github.com/offsky/react-virtualized/commit/086f2bf06c00b83807fc01443c5170e42f1bf401

I am not exactly sure what the right solution is for all use cases since I am very new to this library.

[bvaughn]

This is kind of tricky for Grid, since columns or rows might move around. I think the approach you prototyped in the diff you linked is pretty reasonable.

I've made this change locally (expanding on what you built):

diff --git a/source/CellMeasurer/CellMeasurerCache.jest.js b/source/CellMeasurer/CellMeasurerCache.jest.js
index 7701788..4a5f976 100644
--- a/source/CellMeasurer/CellMeasurerCache.jest.js
+++ b/source/CellMeasurer/CellMeasurerCache.jest.js
@@ -83,6 +83,8 @@ describe('CellMeasurerCache', () => {
     keyMapper.mockReturnValue('a')
 
     const cache = new CellMeasurerCache({
+      defaultHeight: 30,
+      defaultWidth: 50,
       fixedHeight: true,
       fixedWidth: true,
       keyMapper
@@ -90,10 +92,19 @@ describe('CellMeasurerCache', () => {
     cache.set(0, 0, 100, 20)
     expect(cache.has(0, 0)).toBe(true)
 
-    keyMapper.mock.calls.splice(0)
+    keyMapper.mockReset()
     keyMapper.mockReturnValue('b')
     expect(cache.has(0, 0)).toBe(false)
-    expect(keyMapper.mock.calls).toHaveLength(1)
+    expect(cache.columnWidth(0)).toBe(50)
+    expect(cache.rowHeight(0)).toBe(30)
+    expect(keyMapper.mock.calls).toHaveLength(3)
+
+    keyMapper.mockReset()
+    keyMapper.mockReturnValue('a')
+    expect(cache.has(0, 0)).toBe(true)
+    expect(cache.columnWidth(0)).toBe(100)
+    expect(cache.rowHeight(0)).toBe(20)
+    expect(keyMapper.mock.calls).toHaveLength(3)
   })
 
   it('should provide a Grid-compatible :columnWidth method', () => {
diff --git a/source/CellMeasurer/CellMeasurerCache.js b/source/CellMeasurer/CellMeasurerCache.js
index 8537822..20a75b2 100644
--- a/source/CellMeasurer/CellMeasurerCache.js
+++ b/source/CellMeasurer/CellMeasurerCache.js
@@ -134,8 +134,10 @@ export default class CellMeasurerCache {
   }
 
   columnWidth = ({ index } : IndexParam) => {
-    return this._columnWidthCache.hasOwnProperty(index)
-      ? this._columnWidthCache[index]
+    const key = this._keyMapper(0, index)
+
+    return this._columnWidthCache.hasOwnProperty(key)
+      ? this._columnWidthCache[key]
       : this._defaultWidth
   }
 
@@ -179,8 +181,10 @@ export default class CellMeasurerCache {
   }
 
   rowHeight = ({ index } : IndexParam) => {
-    return this._rowHeightCache.hasOwnProperty(index)
-      ? this._rowHeightCache[index]
+    const key = this._keyMapper(index, 0)
+
+    return this._rowHeightCache.hasOwnProperty(key)
+      ? this._rowHeightCache[key]
       : this._defaultHeight
   }
 
@@ -221,8 +225,12 @@ export default class CellMeasurerCache {
     for (let i = 0; i < this._columnCount; i++) {
       rowHeight = Math.max(rowHeight, this.getHeight(rowIndex, i))
     }
-    this._columnWidthCache[columnIndex] = columnWidth
-    this._rowHeightCache[rowIndex] = rowHeight
+
+    const columnKey = this._keyMapper(0, columnIndex)
+    const rowKey = this._keyMapper(rowIndex, 0)
+
+    this._columnWidthCache[columnKey] = columnWidth
+    this._rowHeightCache[rowKey] = rowHeight
   }
 }

[bvaughn]

Let's roll with this. Maybe it's not perfect, but it's better than it was.

Committed with ed887bb. Thanks for bringing this to my attention!