Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(resizer): autosizeColumns is called too many times on page load #1343

Merged
merged 3 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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