From e02ac550d9195ede2df58060fecc81b72c5011f9 Mon Sep 17 00:00:00 2001 From: Ghislain B Date: Thu, 18 Jan 2024 12:59:59 -0500 Subject: [PATCH] perf(resizer): `autosizeColumns` is called too many times on page load (#1343) * perf(resizer): don't call `autosizeColumns` too many times - when loading a new grid, we are calling `grid.autosizeColumns()` to resize the columns and while adding tests for that method I incidently discovered that it was being called multiple times on a page load (up to 6-8x times) but in reality there is no need to call it more than once or twice. - there is also no need to call the `autosizeColumns` if the grid dimension calculated by the Resizer Service are the same as the previously calculated dimensions - the updated code will never call `autosizeColumns` more than twice on a page load --- .../src/core/__tests__/slickGrid.spec.ts | 3 +- .../common/src/services/resizer.service.ts | 7 +++- .../__tests__/slick-vanilla-grid.spec.ts | 16 +++++++- .../components/slick-vanilla-grid-bundle.ts | 38 +++++++++++-------- 4 files changed, 44 insertions(+), 20 deletions(-) diff --git a/packages/common/src/core/__tests__/slickGrid.spec.ts b/packages/common/src/core/__tests__/slickGrid.spec.ts index 07f03f69e..ec41bb9cb 100644 --- a/packages/common/src/core/__tests__/slickGrid.spec.ts +++ b/packages/common/src/core/__tests__/slickGrid.spec.ts @@ -1,11 +1,12 @@ import { BasePubSubService } from '@slickgrid-universal/event-pub-sub'; +import { createDomElement } from '@slickgrid-universal/utils'; + import { CheckboxEditor, InputEditor, LongTextEditor } from '../../editors'; import { SlickCellSelectionModel, SlickRowSelectionModel } from '../../extensions'; import { Column, Editor, FormatterResultWithHtml, FormatterResultWithText, GridOption, type EditCommand } from '../../interfaces'; import { SlickEventData, SlickGlobalEditorLock } from '../slickCore'; import { SlickDataView } from '../slickDataview'; import { SlickGrid } from '../slickGrid'; -import { createDomElement } from '@slickgrid-universal/utils'; jest.useFakeTimers(); diff --git a/packages/common/src/services/resizer.service.ts b/packages/common/src/services/resizer.service.ts index 7d0bf21c4..21cada095 100644 --- a/packages/common/src/services/resizer.service.ts +++ b/packages/common/src/services/resizer.service.ts @@ -348,10 +348,13 @@ export class ResizerService { } // also call the grid auto-size columns so that it takes available space when going bigger - if (this.gridOptions?.enableAutoSizeColumns && this._grid.autosizeColumns) { + if (this._grid && this.gridOptions?.enableAutoSizeColumns) { // make sure that the grid still exist (by looking if the Grid UID is found in the DOM tree) to avoid SlickGrid error "missing stylesheet" if (this.gridUid && document.querySelector(this.gridUidSelector)) { - this._grid.autosizeColumns(); + // don't call autosize unless dimension really changed + if (!this._lastDimensions || this._lastDimensions.height !== newHeight || this._lastDimensions.width !== newWidth) { + this._grid.autosizeColumns(); + } } } else if (this.gridOptions.enableAutoResizeColumnsByCellContent && (!this._lastDimensions?.width || newWidth !== this._lastDimensions?.width)) { // we can call our resize by content here (when enabled) diff --git a/packages/vanilla-bundle/src/components/__tests__/slick-vanilla-grid.spec.ts b/packages/vanilla-bundle/src/components/__tests__/slick-vanilla-grid.spec.ts index 4f432815e..d95674f62 100644 --- a/packages/vanilla-bundle/src/components/__tests__/slick-vanilla-grid.spec.ts +++ b/packages/vanilla-bundle/src/components/__tests__/slick-vanilla-grid.spec.ts @@ -508,6 +508,20 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', () sharedService.slickGrid = mockGrid as unknown as SlickGrid; }); + it('should expect "autosizeColumns" being called when "autoFitColumnsOnFirstLoad" is set we udpated the dataset', () => { + const autosizeSpy = jest.spyOn(mockGrid, 'autosizeColumns'); + const refreshSpy = jest.spyOn(component, 'refreshGridData'); + const mockData = [{ firstName: 'John', lastName: 'Doe' }, { firstName: 'Jane', lastName: 'Smith' }]; + jest.spyOn(mockDataView, 'getLength').mockReturnValueOnce(0).mockReturnValueOnce(0).mockReturnValueOnce(mockData.length); + + component.gridOptions = { autoFitColumnsOnFirstLoad: true }; + component.initialization(divContainer, slickEventHandler); + component.setData(mockData, true); // manually force an autoresize + + expect(autosizeSpy).toHaveBeenCalledTimes(2); // 1x by datasetChanged and 1x by bindResizeHook + expect(refreshSpy).toHaveBeenCalledWith(mockData); + }); + it('should expect "autosizeColumns" being called when "autoFitColumnsOnFirstLoad" is set and we are on first page load', () => { const autosizeSpy = jest.spyOn(mockGrid, 'autosizeColumns'); const refreshSpy = jest.spyOn(component, 'refreshGridData'); @@ -518,7 +532,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', () component.initialization(divContainer, slickEventHandler); component.dataset = mockData; - expect(autosizeSpy).toHaveBeenCalledTimes(3); // 1x by datasetChanged and 2x by bindResizeHook + expect(autosizeSpy).toHaveBeenCalledTimes(1); expect(refreshSpy).toHaveBeenCalledWith(mockData); }); diff --git a/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts b/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts index 01b2b365a..a658ccb8a 100644 --- a/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts +++ b/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts @@ -69,6 +69,7 @@ export class SlickVanillaGridBundle { protected _gridContainerElm!: HTMLElement; protected _gridParentContainerElm!: HTMLElement; protected _hideHeaderRowAfterPageLoad = false; + protected _isAutosizeColsCalled = false; protected _isDatasetInitialized = false; protected _isDatasetHierarchicalInitialized = false; protected _isGridInitialized = false; @@ -140,10 +141,10 @@ export class SlickVanillaGridBundle { } } - get dataset(): any[] { + get dataset(): TData[] { return this.dataView?.getItems() || []; } - set dataset(newDataset: any[]) { + set dataset(newDataset: TData[]) { const prevDatasetLn = this._currentDatasetLength; const isDatasetEqual = dequal(newDataset, this.dataset || []); const isDeepCopyDataOnPageLoadEnabled = !!(this._gridOptions?.enableDeepCopyDatasetOnPageLoad); @@ -160,8 +161,9 @@ export class SlickVanillaGridBundle { // expand/autofit columns on first page load // we can assume that if the prevDataset was empty then we are on first load - if (this.slickGrid && this.gridOptions.autoFitColumnsOnFirstLoad && prevDatasetLn === 0) { + if (this.slickGrid && this.gridOptions.autoFitColumnsOnFirstLoad && prevDatasetLn === 0 && !this._isAutosizeColsCalled) { this.slickGrid.autosizeColumns(); + this._isAutosizeColsCalled = true; } } @@ -269,7 +271,7 @@ export class SlickVanillaGridBundle { gridParentContainerElm: HTMLElement, columnDefs?: Column[], options?: Partial, - dataset?: any[], + dataset?: TData[], hierarchicalDataset?: any[], services?: { backendUtilityService?: BackendUtilityService, @@ -381,7 +383,7 @@ export class SlickVanillaGridBundle { this.initialization(this._gridContainerElm, eventHandler); if (!hierarchicalDataset && !this.gridOptions.backendServiceApi) { - this.dataset = dataset || []; + this.setData(dataset || []); this._currentDatasetLength = this.dataset.length; } } @@ -478,6 +480,7 @@ export class SlickVanillaGridBundle { this._gridContainerElm = gridContainerElm; this._eventPubSubService.publish('onBeforeGridCreate', true); + this._isAutosizeColsCalled = false; this._eventHandler = eventHandler; this._gridOptions = this.mergeGridOptions(this._gridOptions || {} as GridOption); this.backendServiceApi = this._gridOptions?.backendServiceApi; @@ -904,11 +907,6 @@ export class SlickVanillaGridBundle { throw new Error(`[Slickgrid-Universal] You cannot enable both autosize/fit viewport & resize by content, you must choose which resize technique to use. You can enable these 2 options ("autoFitColumnsOnFirstLoad" and "enableAutoSizeColumns") OR these other 2 options ("autosizeColumnsByCellContentOnFirstLoad" and "enableAutoResizeColumnsByCellContent").`); } - if (grid && options.autoFitColumnsOnFirstLoad && options.enableAutoSizeColumns && typeof grid.autosizeColumns === 'function') { - // expand/autofit columns on first page load - grid.autosizeColumns(); - } - // auto-resize grid on browser resize (optionally provide grid height or width) if (options.gridHeight || options.gridWidth) { this.resizerService.resizeGrid(0, { height: options.gridHeight, width: options.gridWidth }); @@ -916,10 +914,10 @@ export class SlickVanillaGridBundle { this.resizerService.resizeGrid(); } - if (grid && options?.enableAutoResize) { - if (options.autoFitColumnsOnFirstLoad && options.enableAutoSizeColumns && typeof grid.autosizeColumns === 'function') { - grid.autosizeColumns(); - } + // expand/autofit columns on first page load + if (grid && options?.enableAutoResize && options.autoFitColumnsOnFirstLoad && options.enableAutoSizeColumns && !this._isAutosizeColsCalled) { + grid.autosizeColumns(); + this._isAutosizeColsCalled = true; } } @@ -959,7 +957,7 @@ export class SlickVanillaGridBundle { * When dataset changes, we need to refresh the entire grid UI & possibly resize it as well * @param dataset */ - refreshGridData(dataset: any[], totalCount?: number) { + refreshGridData(dataset: TData[], totalCount?: number) { // local grid, check if we need to show the Pagination // if so then also check if there's any presets and finally initialize the PaginationService // a local grid with Pagination presets will potentially have a different total of items, we'll need to get it from the DataView and update our total @@ -1066,6 +1064,14 @@ export class SlickVanillaGridBundle { return showing; } + setData(data: TData[], shouldAutosizeColumns = false) { + if (shouldAutosizeColumns) { + this._isAutosizeColsCalled = false; + this._currentDatasetLength = 0; + } + this.dataset = data || []; + } + /** * Check if there's any Pagination Presets defined in the Grid Options, * if there are then load them in the paginationOptions object @@ -1254,7 +1260,7 @@ export class SlickVanillaGridBundle { * if so then also check if there's any presets and finally initialize the PaginationService * a local grid with Pagination presets will potentially have a different total of items, we'll need to get it from the DataView and update our total */ - protected loadLocalGridPagination(dataset?: any[]) { + protected loadLocalGridPagination(dataset?: TData[]) { if (this.gridOptions && this._paginationOptions) { this.totalItems = Array.isArray(dataset) ? dataset.length : 0; if (this._paginationOptions && this.dataView?.getPagingInfo) {