From 1135ad16815694506ac1c13f869deab22d2a80f0 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 20 Feb 2017 11:38:17 -0800 Subject: [PATCH] Added DEV mode warnings to CellMeasurerCache to warn about improper configurations --- source/CellMeasurer/CellMeasurer.jest.js | 28 ++++-- source/CellMeasurer/CellMeasurerCache.jest.js | 97 +++++++++++++++++-- source/CellMeasurer/CellMeasurerCache.js | 47 ++++++++- 3 files changed, 154 insertions(+), 18 deletions(-) diff --git a/source/CellMeasurer/CellMeasurer.jest.js b/source/CellMeasurer/CellMeasurer.jest.js index 49371e5cd..1e891531e 100644 --- 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 =
, 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(
) @@ -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 index 7ea12d3a1..546bd7314 100644 --- 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 index f8289e6eb..031bca516 100644 --- 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