Added DEV mode warnings to CellMeasurerCache to warn about improper c…

…onfigurations
bvaughn · Feb 20, 2017 · 1135ad1 · 1135ad1
1 parent da4fbef
commit 1135ad1
Show file tree

Hide file tree

Showing 3 changed files with 154 additions and 18 deletions.
diff --git a/source/CellMeasurer/CellMeasurer.jest.js b/source/CellMeasurer/CellMeasurer.jest.js
@@ -42,7 +42,9 @@ function createParent ({
 }
 
 function renderHelper ({
-  cache = new CellMeasurerCache(),
+  cache = new CellMeasurerCache({
+    fixedWidth: true
+  }),
   children = <div />,
   parent
 } = {}) {
@@ -61,7 +63,9 @@ function renderHelper ({
 
 describe('CellMeasurer', () => {
   it('componentDidMount() should measure content that is not already in the cache', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedWidth: true
+    })
     const parent = createParent({ cache })
 
     mockClientWidthAndHeight({
@@ -87,7 +91,9 @@ describe('CellMeasurer', () => {
   })
 
   it('componentDidMount() should not measure content that is already in the cache', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedWidth: true
+    })
     cache.set(0, 0, 100, 20)
 
     const parent = createParent({ cache })
@@ -110,7 +116,9 @@ describe('CellMeasurer', () => {
   })
 
   it('componentDidUpdate() should measure content that is not already in the cache', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedWidth: true
+    })
     const parent = createParent({ cache })
 
     renderHelper({ cache, parent })
@@ -142,7 +150,9 @@ describe('CellMeasurer', () => {
   })
 
   it('componentDidUpdate() should not measure content that is already in the cache', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedWidth: true
+    })
     cache.set(0, 0, 100, 20)
 
     const parent = createParent({ cache })
@@ -166,7 +176,9 @@ describe('CellMeasurer', () => {
   })
 
   it('componentDidUpdate() should pass a :measure param to a function child', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedWidth: true
+    })
 
     const children = jest.fn()
     children.mockReturnValue(<div />)
@@ -188,7 +200,9 @@ describe('CellMeasurer', () => {
       width: 100
     })
 
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedWidth: true
+    })
 
     renderHelper({ cache }) // No parent Grid
 

diff --git a/source/CellMeasurer/CellMeasurerCache.jest.js b/source/CellMeasurer/CellMeasurerCache.jest.js
@@ -5,6 +5,8 @@ describe('CellMeasurerCache', () => {
     const cache = new CellMeasurerCache({
       defaultHeight: 20,
       defaultWidth: 100,
+      fixedHeight: true,
+      fixedWidth: true,
       minHeight: 30,
       minWidth: 150
     })
@@ -16,12 +18,18 @@ describe('CellMeasurerCache', () => {
   })
 
   it('should correctly report cache status', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true
+    })
     expect(cache.has(0, 0)).toBe(false)
   })
 
   it('should cache cells', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true
+    })
     cache.set(0, 0, 100, 20)
     expect(cache.has(0, 0)).toBe(true)
   })
@@ -30,6 +38,8 @@ describe('CellMeasurerCache', () => {
     const cache = new CellMeasurerCache({
       defaultHeight: 20,
       defaultWidth: 100,
+      fixedHeight: true,
+      fixedWidth: true,
       minHeight: 15,
       minWidth: 80
     })
@@ -41,7 +51,10 @@ describe('CellMeasurerCache', () => {
   })
 
   it('should clear a single cached cell', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true
+    })
     cache.set(0, 0, 100, 20)
     cache.set(1, 0, 100, 20)
     expect(cache.has(0, 0)).toBe(true)
@@ -52,7 +65,10 @@ describe('CellMeasurerCache', () => {
   })
 
   it('should clear all cached cells', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true
+    })
     cache.set(0, 0, 100, 20)
     cache.set(1, 0, 100, 20)
     expect(cache.has(0, 0)).toBe(true)
@@ -66,7 +82,11 @@ describe('CellMeasurerCache', () => {
     const keyMapper = jest.fn()
     keyMapper.mockReturnValue('a')
 
-    const cache = new CellMeasurerCache({ keyMapper })
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true,
+      keyMapper
+    })
     cache.set(0, 0, 100, 20)
     expect(cache.has(0, 0)).toBe(true)
 
@@ -77,7 +97,10 @@ describe('CellMeasurerCache', () => {
   })
 
   it('should provide a Grid-compatible :columnWidth method', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true
+    })
     expect(cache.columnWidth({ index: 0 })).toBe(DEFAULT_WIDTH)
     cache.set(0, 0, 100, 50)
     expect(cache.columnWidth({ index: 0 })).toBe(100)
@@ -89,7 +112,10 @@ describe('CellMeasurerCache', () => {
   })
 
   it('should provide a Grid-compatible :rowHeight method', () => {
-    const cache = new CellMeasurerCache()
+    const cache = new CellMeasurerCache({
+      fixedHeight: true,
+      fixedWidth: true
+    })
     expect(cache.rowHeight({ index: 0 })).toBe(DEFAULT_HEIGHT)
     cache.set(0, 0, 100, 50)
     expect(cache.rowHeight({ index: 0 })).toBe(50)
@@ -102,15 +128,68 @@ describe('CellMeasurerCache', () => {
 
   it('should return the :defaultWidth for :columnWidth if not measured', () => {
     const cache = new CellMeasurerCache({
-      defaultWidth: 25
+      defaultWidth: 25,
+      fixedHeight: true,
+      fixedWidth: true
     })
     expect(cache.columnWidth({ index: 0 })).toBe(25)
   })
 
   it('should return the :defaultHeight for :rowHeight if not measured', () => {
     const cache = new CellMeasurerCache({
-      defaultHeight: 25
+      defaultHeight: 25,
+      fixedHeight: true,
+      fixedWidth: true
     })
     expect(cache.rowHeight({ index: 0 })).toBe(25)
   })
+
+  describe('DEV mode', () => {
+    it('should warn about dynamic width and height configurations', () => {
+      spyOn(console, 'warn')
+
+      const cache = new CellMeasurerCache({
+        fixedHeight: false,
+        fixedWidth: false
+      })
+
+      expect(cache.hasFixedHeight()).toBe(false)
+      expect(cache.hasFixedWidth()).toBe(false)
+      expect(console.warn).toHaveBeenCalledWith(
+        'CellMeasurerCache should only measure a cell\'s width or height. ' +
+        'You have configured CellMeasurerCache to measure both. ' +
+        'This will result in poor performance.'
+      )
+    })
+
+    it('should warn about dynamic width with a defaultWidth of 0', () => {
+      spyOn(console, 'warn')
+
+      const cache = new CellMeasurerCache({
+        defaultWidth: 0,
+        fixedHeight: true
+      })
+
+      expect(cache.getWidth(0, 0)).toBe(0)
+      expect(console.warn).toHaveBeenCalledWith(
+        'Fixed width CellMeasurerCache should specify a :defaultWidth greater than 0. ' +
+        'Failing to do so will lead to unnecessary layout and poor performance.'
+      )
+    })
+
+    it('should warn about dynamic height with a defaultHeight of 0', () => {
+      spyOn(console, 'warn')
+
+      const cache = new CellMeasurerCache({
+        defaultHeight: 0,
+        fixedWidth: true
+      })
+
+      expect(cache.getHeight(0, 0)).toBe(0)
+      expect(console.warn).toHaveBeenCalledWith(
+        'Fixed height CellMeasurerCache should specify a :defaultHeight greater than 0. ' +
+        'Failing to do so will lead to unnecessary layout and poor performance.'
+      )
+    })
+  })
 })
diff --git a/source/CellMeasurer/CellMeasurerCache.js b/source/CellMeasurer/CellMeasurerCache.js
@@ -59,8 +59,51 @@ export default class CellMeasurerCache {
     this._minWidth = minWidth || 0
     this._keyMapper = keyMapper || defaultKeyMapper
 
-    this._defaultHeight = Math.max(this._minHeight, defaultHeight || DEFAULT_HEIGHT)
-    this._defaultWidth = Math.max(this._minWidth, defaultWidth || DEFAULT_WIDTH)
+    this._defaultHeight = Math.max(
+      this._minHeight,
+      typeof defaultHeight === 'number'
+        ? defaultHeight
+        : DEFAULT_HEIGHT
+    )
+    this._defaultWidth = Math.max(
+      this._minWidth,
+      typeof defaultWidth === 'number'
+        ? defaultWidth
+        : DEFAULT_WIDTH
+    )
+
+    if (process.env.NODE_ENV !== 'production') {
+      if (
+        this._hasFixedHeight === false &&
+        this._hasFixedWidth === false
+      ) {
+        console.warn(
+          'CellMeasurerCache should only measure a cell\'s width or height. ' +
+          'You have configured CellMeasurerCache to measure both. ' +
+          'This will result in poor performance.'
+        )
+      }
+
+      if (
+        this._hasFixedHeight === false &&
+        this._defaultHeight === 0
+      ) {
+        console.warn(
+          'Fixed height CellMeasurerCache should specify a :defaultHeight greater than 0. ' +
+          'Failing to do so will lead to unnecessary layout and poor performance.'
+        )
+      }
+
+      if (
+        this._hasFixedWidth === false &&
+        this._defaultWidth === 0
+      ) {
+        console.warn(
+          'Fixed width CellMeasurerCache should specify a :defaultWidth greater than 0. ' +
+          'Failing to do so will lead to unnecessary layout and poor performance.'
+        )
+      }
+    }
 
     this._columnCount = 0
     this._rowCount = 0