Skip to content

Commit

Permalink
perf(resizer): autosizeColumns is called too many times on page load (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
ghiscoding authored Jan 18, 2024
1 parent 03cad4a commit e02ac55
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 20 deletions.
3 changes: 2 additions & 1 deletion packages/common/src/core/__tests__/slickGrid.spec.ts
Original file line number Diff line number Diff line change
@@ -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();

Expand Down
7 changes: 5 additions & 2 deletions packages/common/src/services/resizer.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);
});

Expand Down
38 changes: 22 additions & 16 deletions packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export class SlickVanillaGridBundle<TData = any> {
protected _gridContainerElm!: HTMLElement;
protected _gridParentContainerElm!: HTMLElement;
protected _hideHeaderRowAfterPageLoad = false;
protected _isAutosizeColsCalled = false;
protected _isDatasetInitialized = false;
protected _isDatasetHierarchicalInitialized = false;
protected _isGridInitialized = false;
Expand Down Expand Up @@ -140,10 +141,10 @@ export class SlickVanillaGridBundle<TData = any> {
}
}

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);
Expand All @@ -160,8 +161,9 @@ export class SlickVanillaGridBundle<TData = any> {

// 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;
}
}

Expand Down Expand Up @@ -269,7 +271,7 @@ export class SlickVanillaGridBundle<TData = any> {
gridParentContainerElm: HTMLElement,
columnDefs?: Column<TData>[],
options?: Partial<GridOption>,
dataset?: any[],
dataset?: TData[],
hierarchicalDataset?: any[],
services?: {
backendUtilityService?: BackendUtilityService,
Expand Down Expand Up @@ -381,7 +383,7 @@ export class SlickVanillaGridBundle<TData = any> {

this.initialization(this._gridContainerElm, eventHandler);
if (!hierarchicalDataset && !this.gridOptions.backendServiceApi) {
this.dataset = dataset || [];
this.setData(dataset || []);
this._currentDatasetLength = this.dataset.length;
}
}
Expand Down Expand Up @@ -478,6 +480,7 @@ export class SlickVanillaGridBundle<TData = any> {
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;
Expand Down Expand Up @@ -904,22 +907,17 @@ export class SlickVanillaGridBundle<TData = any> {
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 });
} else {
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;
}
}

Expand Down Expand Up @@ -959,7 +957,7 @@ export class SlickVanillaGridBundle<TData = any> {
* 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
Expand Down Expand Up @@ -1066,6 +1064,14 @@ export class SlickVanillaGridBundle<TData = any> {
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
Expand Down Expand Up @@ -1254,7 +1260,7 @@ export class SlickVanillaGridBundle<TData = any> {
* 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) {
Expand Down

0 comments on commit e02ac55

Please sign in to comment.