Skip to content

Commit

Permalink
perf: decrease calls to setItems & grid invalidate (#1363)
Browse files Browse the repository at this point in the history 
* perf: decrease calls to setItems & grid invalidate
- with the previous implementation when dataset was provided through the constructor, it was internally calling `dataView.setItems(data)` twice, this PR will now only call it once and is much closer to the repos implementation which didn't have this issue (like Angular-Slickgrid)
- remove unnecessary calls to `grid.invalidate()`
- remove unnecessary calls to `grid.render()` when `grid.invalidate()` is used since that one already calls the render, so no need to do the same thing twice
  • Loading branch information
@ghiscoding
ghiscoding committed Jan 24, 2024
1 parent 4ce140c commit cab6898
Show file tree
Hide file tree
Showing 9 changed files with 44 additions and 37 deletions.
2 changes: 1 addition & 1 deletion packages/common/src/editors/__tests__/autocompleterEditor.spec.ts
View file
Expand Up @@ -108,7 +108,7 @@ describe('AutocompleterEditor', () => {
gridOptionMock.translater = translateService;
gridOptionMock.enableTranslate = true;
const mockCollection = ['male', 'female'];
const promise = new Promise(resolve => resolve(mockCollection));
const promise = Promise.resolve(mockCollection);
(mockColumn.internalColumnEditor as ColumnEditor).collection = null as any;
(mockColumn.internalColumnEditor as ColumnEditor).collectionAsync = promise;

Expand Down
2 changes: 1 addition & 1 deletion packages/common/src/editors/__tests__/selectEditor.spec.ts
View file
Expand Up @@ -155,7 +155,7 @@ describe('SelectEditor', () => {

it('should initialize the editor with element being disabled in the DOM when passing a collectionAsync and an empty collection property', () => {
const mockCollection = ['male', 'female'];
const promise = new Promise(resolve => resolve(mockCollection));
const promise = Promise.resolve(mockCollection);
(mockColumn.internalColumnEditor as ColumnEditor).collection = null as any;
(mockColumn.internalColumnEditor as ColumnEditor).collectionAsync = promise;
gridOptionMock.translater = translateService;
Expand Down
4 changes: 1 addition & 3 deletions packages/common/src/services/__tests__/grid.service.spec.ts
View file
Expand Up @@ -1673,13 +1673,11 @@ describe('Grid Service', () => {
describe('renderGrid method', () => {
it('should invalidate the grid and call render after', () => {
const invalidateSpy = jest.spyOn(gridStub, 'invalidate');
const renderSpy = jest.spyOn(gridStub, 'render');

service.renderGrid();

expect(invalidateSpy).toHaveBeenCalled();
expect(renderSpy).toHaveBeenCalled();
expect(gridStub.invalidate).toHaveBeenCalledBefore(gridStub.render as any);
expect(gridStub.invalidate).toHaveBeenCalled();
});
});

Expand Down
3 changes: 1 addition & 2 deletions packages/common/src/services/grid.service.ts
View file
Expand Up @@ -290,9 +290,8 @@ export class GridService {

/** Re-Render the Grid */
renderGrid() {
if (this._grid && typeof this._grid.invalidate === 'function') {
if (typeof this._grid?.invalidate === 'function') {
this._grid.invalidate();
this._grid.render();
}
}

Expand Down
20 changes: 10 additions & 10 deletions packages/vanilla-bundle/src/components/__tests__/slick-vanilla-grid.spec.ts
View file
Expand Up @@ -714,7 +714,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()

it('should be able to load async editors with a regular Promise', (done) => {
const mockCollection = ['male', 'female'];
const promise = new Promise(resolve => resolve(mockCollection));
const promise = Promise.resolve(mockCollection);
const mockColDefs = [{ id: 'gender', field: 'gender', editor: { model: Editors.text, collectionAsync: promise } }] as Column[];

component.columnDefinitions = mockColDefs;
Expand All @@ -730,7 +730,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()

it('should be able to load collectionAsync and expect Editor to be destroyed and re-render when receiving new collection from await', (done) => {
const mockCollection = ['male', 'female'];
const promise = new Promise(resolve => resolve(mockCollection));
const promise = Promise.resolve(mockCollection);
const mockEditor = {
disable: jest.fn(),
destroy: jest.fn(),
Expand Down Expand Up @@ -758,7 +758,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()

it('should be able to load async editors with as a Promise with content to simulate http-client', (done) => {
const mockCollection = ['male', 'female'];
const promise = new Promise(resolve => resolve({ content: mockCollection }));
const promise = Promise.resolve({ content: mockCollection });
const mockColDefs = [{ id: 'gender', field: 'gender', editor: { model: Editors.text, collectionAsync: promise } }] as Column[];

component.columnDefinitions = mockColDefs;
Expand Down Expand Up @@ -1064,6 +1064,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
const refreshSpy = jest.spyOn(component, 'refreshGridData');

const mockData = [{ firstName: 'John', lastName: 'Doe' }, { firstName: 'Jane', lastName: 'Smith' }];
jest.spyOn(mockDataView, 'getItems').mockReturnValueOnce(mockData);
component.gridOptions = {
enablePagination: true,
presets: { pagination: { pageSize: 2, pageNumber: expectedPageNumber } }
Expand Down Expand Up @@ -1529,7 +1530,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
service: mockGraphqlService2,
options: mockGraphqlOptions,
preProcess: () => jest.fn(),
process: () => new Promise(resolve => resolve({ data: { users: { nodes: [], totalCount: 100 } } })),
process: () => Promise.resolve({ data: { users: { nodes: [], totalCount: 100 } } }),
} as GraphqlServiceApi,
pagination: mockPagination,
} as unknown as GridOption;
Expand All @@ -1547,7 +1548,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
backendServiceApi: {
service: mockGraphqlService,
preProcess: () => jest.fn(),
process: () => new Promise(resolve => resolve('process resolved')),
process: () => Promise.resolve('process resolved'),
}
} as unknown as GridOption;
component.initialization(divContainer, slickEventHandler);
Expand All @@ -1564,7 +1565,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
service: mockGraphqlService,
useLocalSorting: true,
preProcess: () => jest.fn(),
process: () => new Promise(resolve => resolve('process resolved')),
process: () => Promise.resolve('process resolved'),
}
} as unknown as GridOption;
component.initialization(divContainer, slickEventHandler);
Expand Down Expand Up @@ -1594,7 +1595,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
service: mockGraphqlService,
useLocalFiltering: true,
preProcess: () => jest.fn(),
process: () => new Promise(resolve => resolve('process resolved')),
process: () => Promise.resolve('process resolved'),
}
} as unknown as GridOption;
component.initialization(divContainer, slickEventHandler);
Expand All @@ -1612,7 +1613,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
backendServiceApi: {
service: mockGraphqlService,
preProcess: () => jest.fn(),
process: () => new Promise(resolve => resolve('process resolved')),
process: () => Promise.resolve('process resolved'),
}
} as unknown as GridOption;
component.initialization(divContainer, slickEventHandler);
Expand Down Expand Up @@ -1904,7 +1905,6 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
});

it('should have custom footer with metrics when the DataView "onSetItemsCalled" event is triggered', () => {
const invalidateSpy = jest.spyOn(mockGrid, 'invalidate');
const expectation = {
startTime: expect.toBeDate(),
endTime: expect.toBeDate(),
Expand All @@ -1918,7 +1918,6 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
const footerSpy = jest.spyOn(component.slickFooter!, 'metrics', 'set');
mockDataView.onSetItemsCalled.notify({ idProperty: 'id', itemCount: 0 });

expect(invalidateSpy).toHaveBeenCalled();
expect(component.metrics).toEqual(expectation);
expect(footerSpy).toHaveBeenCalledWith(expectation);
});
Expand Down Expand Up @@ -2119,6 +2118,7 @@ describe('Slick-Vanilla-Grid-Bundle Component instantiated via Constructor', ()
component.gridOptions = { enableTreeData: true, treeDataOptions: { columnId: 'file', initialSort: { columndId: 'file', direction: 'ASC' } } } as unknown as GridOption;
component.datasetHierarchical = mockHierarchical;
component.eventPubSubService = new EventPubSubService(divContainer);
component.isDatasetHierarchicalInitialized = true;
component.initialization(divContainer, slickEventHandler);

expect(hierarchicalSpy).toHaveBeenCalledWith(mockHierarchical);
Expand Down
43 changes: 26 additions & 17 deletions packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts
View file
Expand Up @@ -201,6 +201,10 @@ export class SlickVanillaGridBundle<TData = any> {
this._eventPubSubService = pubSub;
}

set isDatasetHierarchicalInitialized(isInitialized: boolean) {
this._isDatasetHierarchicalInitialized = isInitialized;
}

get gridOptions(): GridOption {
return this._gridOptions || {} as GridOption;
}
Expand Down Expand Up @@ -382,11 +386,7 @@ export class SlickVanillaGridBundle<TData = any> {
this.universalContainerService.registerInstance('TranslaterService', this.translaterService);
this.universalContainerService.registerInstance('TreeDataService', this.treeDataService);

this.initialization(this._gridContainerElm, eventHandler);
if (!hierarchicalDataset && !this.gridOptions.backendServiceApi) {
this.setData(dataset || []);
this._currentDatasetLength = this.dataset.length;
}
this.initialization(this._gridContainerElm, eventHandler, dataset);
}

emptyGridContainerElm() {
Expand Down Expand Up @@ -471,7 +471,7 @@ export class SlickVanillaGridBundle<TData = any> {
this._registeredResources = [];
}

initialization(gridContainerElm: HTMLElement, eventHandler: SlickEventHandler) {
initialization(gridContainerElm: HTMLElement, eventHandler: SlickEventHandler, inputDataset?: TData[]) {
// when detecting a frozen grid, we'll automatically enable the mousewheel scroll handler so that we can scroll from both left/right frozen containers
if (this.gridOptions && ((this.gridOptions.frozenRow !== undefined && this.gridOptions.frozenRow >= 0) || this.gridOptions.frozenColumn !== undefined && this.gridOptions.frozenColumn >= 0) && this.gridOptions.enableMouseWheelScrollHandler === undefined) {
this.gridOptions.enableMouseWheelScrollHandler = true;
Expand Down Expand Up @@ -576,9 +576,13 @@ export class SlickVanillaGridBundle<TData = any> {
}

// load the data in the DataView (unless it's a hierarchical dataset, if so it will be loaded after the initial tree sort)
if (Array.isArray(this.dataset)) {
const initialDataset = this.gridOptions?.enableTreeData ? this.sortTreeDataset(this.dataset) : this.dataset;
this.dataView?.setItems(initialDataset, this._gridOptions.datasetIdPropertyName);
if (this.dataView) {
inputDataset = inputDataset || [];
const initialDataset = this.gridOptions?.enableTreeData ? this.sortTreeDataset(inputDataset) : inputDataset;
this.dataView.beginUpdate();
this.dataView.setItems(initialDataset, this._gridOptions.datasetIdPropertyName);
this._currentDatasetLength = inputDataset.length;
this.dataView.endUpdate();
}

// if you don't want the items that are not visible (due to being filtered out or being on a different page)
Expand All @@ -604,14 +608,14 @@ export class SlickVanillaGridBundle<TData = any> {
}
}

this.slickGrid.invalidate();

if ((this.dataView?.getLength() ?? 0) > 0) {
if (!this._isDatasetInitialized && (this._gridOptions.enableCheckboxSelector || this._gridOptions.enableRowSelection)) {
this.loadRowSelectionPresetWhenExists();
}
this.loadFilterPresetsWhenDatasetInitialized();
this._isDatasetInitialized = true;
} else {
this.displayEmptyDataWarning(true);
}

// user might want to hide the header row on page load but still have `enableFiltering: true`
Expand Down Expand Up @@ -643,6 +647,14 @@ export class SlickVanillaGridBundle<TData = any> {
// bind resize ONLY after the dataView is ready
this.bindResizeHook(this.slickGrid, this.gridOptions);

// 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
if (this.gridOptions?.enablePagination && this._isLocalGrid) {
this.showPagination = true;
this.loadLocalGridPagination(this.dataset);
}

// once the grid is created, we'll return its instance (we do this to return Transient Services from DI)
this._slickerGridInstances = {
// Slick Grid & DataView objects
Expand Down Expand Up @@ -959,11 +971,6 @@ 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
if (this.slickGrid && this._gridOptions) {
if (this._gridOptions.enablePagination && this._isLocalGrid) {
this.showPagination = true;
this.loadLocalGridPagination(dataset);
}

if (this._gridOptions.enableEmptyDataWarningMessage && Array.isArray(dataset)) {
const finalTotalCount = totalCount || dataset.length;
this.displayEmptyDataWarning(finalTotalCount < 1);
Expand Down Expand Up @@ -1094,7 +1101,9 @@ export class SlickVanillaGridBundle<TData = any> {
}

protected displayEmptyDataWarning(showWarning = true) {
this.slickEmptyWarning?.showEmptyDataMessage(showWarning);
if (this.gridOptions.enableEmptyDataWarningMessage) {
this.slickEmptyWarning?.showEmptyDataMessage(showWarning);
}
}

/** When data changes in the DataView, we'll refresh the metrics and/or display a warning if the dataset is empty */
Expand Down
1 change: 1 addition & 0 deletions packages/vanilla-force-bundle/src/__tests__/vanilla-force-bundle.spec.ts
View file
Expand Up @@ -555,6 +555,7 @@ describe('Vanilla-Force-Grid-Bundle Component instantiated via Constructor', ()
const refreshSpy = jest.spyOn(component, 'refreshGridData');

const mockData = [{ firstName: 'John', lastName: 'Doe' }, { firstName: 'Jane', lastName: 'Smith' }];
jest.spyOn(mockDataView, 'getItems').mockReturnValueOnce(mockData);
component.gridOptions = {
enablePagination: true,
presets: { pagination: { pageSize: 2, pageNumber: expectedPageNumber } }
Expand Down
4 changes: 2 additions & 2 deletions test/cypress/e2e/example17.cy.ts
View file
Expand Up @@ -200,8 +200,8 @@ describe('Example 17 - Auto-Scroll with Range Selector', () => {
cy.get(`.grid17-2 [style="top: ${CELL_HEIGHT * 0}px;"]`).should('have.length', 2 * 2);
cy.get(`.grid17-1 .grid-canvas-left > [style="top: ${CELL_HEIGHT * 0}px;"]`).children().should('have.length', 2 * 2);
cy.get(`.grid17-2 .grid-canvas-left > [style="top: ${CELL_HEIGHT * 0}px;"]`).children().should('have.length', 2 * 2);
cy.get('.grid17-1 .grid-canvas-top').children().should('have.length', 3 * 2 + 1); // +1 for "Empty Data" div
cy.get('.grid17-2 .grid-canvas-top').children().should('have.length', 3 * 2 + 1);
cy.get('.grid17-1 .grid-canvas-top').children().should('have.length', 3 * 2);
cy.get('.grid17-2 .grid-canvas-top').children().should('have.length', 3 * 2);
});

function resetScrollInFrozen() {
Expand Down
2 changes: 1 addition & 1 deletion test/translateServiceStub.ts
View file
Expand Up @@ -103,6 +103,6 @@ export class TranslateServiceStub implements TranslaterService {
}

use(locale: string) {
return new Promise(resolve => resolve(this._locale = locale));
return Promise.resolve(this._locale = locale);
}
}

0 comments on commit cab6898

Please sign in to comment.