From cab68989ebd53178dfcee5ed293379dc8932a72f Mon Sep 17 00:00:00 2001 From: Ghislain B Date: Wed, 24 Jan 2024 11:13:41 -0500 Subject: [PATCH] perf: decrease calls to setItems & grid invalidate (#1363) * 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 --- .../__tests__/autocompleterEditor.spec.ts | 2 +- .../editors/__tests__/selectEditor.spec.ts | 2 +- .../services/__tests__/grid.service.spec.ts | 4 +- packages/common/src/services/grid.service.ts | 3 +- .../__tests__/slick-vanilla-grid.spec.ts | 20 ++++----- .../components/slick-vanilla-grid-bundle.ts | 43 +++++++++++-------- .../__tests__/vanilla-force-bundle.spec.ts | 1 + test/cypress/e2e/example17.cy.ts | 4 +- test/translateServiceStub.ts | 2 +- 9 files changed, 44 insertions(+), 37 deletions(-) diff --git a/packages/common/src/editors/__tests__/autocompleterEditor.spec.ts b/packages/common/src/editors/__tests__/autocompleterEditor.spec.ts index d095f0c01..00b4ed43d 100644 --- a/packages/common/src/editors/__tests__/autocompleterEditor.spec.ts +++ b/packages/common/src/editors/__tests__/autocompleterEditor.spec.ts @@ -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; diff --git a/packages/common/src/editors/__tests__/selectEditor.spec.ts b/packages/common/src/editors/__tests__/selectEditor.spec.ts index ebf7a2b9d..57d1b9330 100644 --- a/packages/common/src/editors/__tests__/selectEditor.spec.ts +++ b/packages/common/src/editors/__tests__/selectEditor.spec.ts @@ -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; diff --git a/packages/common/src/services/__tests__/grid.service.spec.ts b/packages/common/src/services/__tests__/grid.service.spec.ts index 1750e5e43..726843c6d 100644 --- a/packages/common/src/services/__tests__/grid.service.spec.ts +++ b/packages/common/src/services/__tests__/grid.service.spec.ts @@ -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(); }); }); diff --git a/packages/common/src/services/grid.service.ts b/packages/common/src/services/grid.service.ts index e03e41092..b125466a5 100644 --- a/packages/common/src/services/grid.service.ts +++ b/packages/common/src/services/grid.service.ts @@ -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(); } } 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 b187c447b..e050c0e81 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 @@ -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; @@ -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(), @@ -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; @@ -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 } } @@ -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; @@ -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); @@ -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); @@ -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); @@ -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); @@ -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(), @@ -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); }); @@ -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); 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 8163e90f4..1b5598e6d 100644 --- a/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts +++ b/packages/vanilla-bundle/src/components/slick-vanilla-grid-bundle.ts @@ -201,6 +201,10 @@ export class SlickVanillaGridBundle { this._eventPubSubService = pubSub; } + set isDatasetHierarchicalInitialized(isInitialized: boolean) { + this._isDatasetHierarchicalInitialized = isInitialized; + } + get gridOptions(): GridOption { return this._gridOptions || {} as GridOption; } @@ -382,11 +386,7 @@ export class SlickVanillaGridBundle { 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() { @@ -471,7 +471,7 @@ export class SlickVanillaGridBundle { 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; @@ -576,9 +576,13 @@ export class SlickVanillaGridBundle { } // 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) @@ -604,14 +608,14 @@ export class SlickVanillaGridBundle { } } - 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` @@ -643,6 +647,14 @@ export class SlickVanillaGridBundle { // 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 @@ -959,11 +971,6 @@ 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 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); @@ -1094,7 +1101,9 @@ export class SlickVanillaGridBundle { } 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 */ diff --git a/packages/vanilla-force-bundle/src/__tests__/vanilla-force-bundle.spec.ts b/packages/vanilla-force-bundle/src/__tests__/vanilla-force-bundle.spec.ts index 0c737372f..10d6160c6 100644 --- a/packages/vanilla-force-bundle/src/__tests__/vanilla-force-bundle.spec.ts +++ b/packages/vanilla-force-bundle/src/__tests__/vanilla-force-bundle.spec.ts @@ -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 } } diff --git a/test/cypress/e2e/example17.cy.ts b/test/cypress/e2e/example17.cy.ts index dc15667a9..f9541b313 100644 --- a/test/cypress/e2e/example17.cy.ts +++ b/test/cypress/e2e/example17.cy.ts @@ -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() { diff --git a/test/translateServiceStub.ts b/test/translateServiceStub.ts index d5ae74aa3..591cdcb0c 100644 --- a/test/translateServiceStub.ts +++ b/test/translateServiceStub.ts @@ -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); } }