From 98bd3341e5d089cedf79e09944b8ffc9c4c1a273 Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Sun, 15 Sep 2024 01:13:00 -0400 Subject: [PATCH 1/2] perf(treeData): huge time decrease to expandAll/collapseAll tree grid --- .../__tests__/treeData.service.spec.ts | 98 ++++++++++++++----- .../common/src/services/treeData.service.ts | 28 ++++-- 2 files changed, 91 insertions(+), 35 deletions(-) diff --git a/packages/common/src/services/__tests__/treeData.service.spec.ts b/packages/common/src/services/__tests__/treeData.service.spec.ts index 7acba3afa..011b73955 100644 --- a/packages/common/src/services/__tests__/treeData.service.spec.ts +++ b/packages/common/src/services/__tests__/treeData.service.spec.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, Mock, vi } from 'vitest'; import { type BasePubSubService } from '@slickgrid-universal/event-pub-sub'; import type { Column, GridOption, BackendService } from '../../interfaces/index'; @@ -15,16 +15,19 @@ vi.mock('../utilities', async (importOriginal) => ({ unflattenParentChildArrayToTree: vi.fn(), })); +const unflattenActual = (await vi.importActual('../utilities')).unflattenParentChildArrayToTree; + vi.useFakeTimers(); -const gridOptionsMock = { +const gridOptionsMock: GridOption = { multiColumnSort: false, enableFiltering: true, enableTreeData: true, treeDataOptions: { - columnId: 'file' + columnId: 'file', + childrenPropName: 'files' } -} as unknown as GridOption; +}; const backendServiceStub = { buildQuery: vi.fn(), @@ -97,6 +100,7 @@ describe('TreeData Service', () => { service = new TreeDataService(mockPubSub, sharedService, sortServiceStub); slickgridEventHandler = service.eventHandler; vi.spyOn(gridStub, 'getData').mockReturnValue(dataViewStub); + (unflattenParentChildArrayToTree as Mock).mockImplementationOnce(unflattenActual); }); afterEach(() => { @@ -181,7 +185,7 @@ describe('TreeData Service', () => { it('should return hierarchical dataset when defined', () => { const mockHierarchical = [{ file: 'documents', files: [{ file: 'vacation.txt' }] }]; - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue(mockHierarchical); + vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValueOnce(mockHierarchical); expect(service.datasetHierarchical).toEqual(mockHierarchical); }); @@ -265,7 +269,10 @@ describe('TreeData Service', () => { mockRowData.__collapsed = true; vi.spyOn(gridStub, 'getData').mockReturnValue(dataViewStub); const spyGetItem = vi.spyOn(dataViewStub, 'getItem').mockReturnValue(mockRowData); - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue([mockRowData]); + vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get') + .mockReturnValueOnce([mockRowData]) + .mockReturnValueOnce([mockRowData]) + .mockReturnValueOnce([mockRowData]); const spyUptItem = vi.spyOn(dataViewStub, 'updateItem'); const spyInvalidate = vi.spyOn(gridStub, 'invalidate'); @@ -316,22 +323,22 @@ describe('TreeData Service', () => { vi.clearAllMocks(); mockFlatDataset = [ { id: 0, file: 'TXT', size: 5.8, __hasChildren: true, __treeLevel: 0 }, - { id: 1, file: 'myFile.txt', size: 0.5, __treeLevel: 1 }, - { id: 2, file: 'myMusic.txt', size: 5.3, __treeLevel: 1 }, + { id: 1, file: 'myFile.txt', size: 0.5, __treeLevel: 1, __parentId: 0 }, + { id: 2, file: 'myMusic.txt', size: 5.3, __treeLevel: 1, __parentId: 0 }, { id: 4, file: 'MP3', size: 3.4, __hasChildren: true, __treeLevel: 0 }, - { id: 5, file: 'relaxation.mp3', size: 3.4, __treeLevel: 1 } + { id: 5, file: 'relaxation.mp3', size: 3.4, __treeLevel: 1, __parentId: 4 } ]; mockHierarchical = [ { id: 0, file: 'TXT', files: [{ id: 1, file: 'myFile.txt', size: 0.5, }, { id: 2, file: 'myMusic.txt', size: 5.3, }] }, { id: 4, file: 'MP3', files: [{ id: 5, file: 'relaxation.mp3', size: 3.4, }] } ]; - gridOptionsMock.treeDataOptions = { columnId: 'file' }; + gridOptionsMock.treeDataOptions = { columnId: 'file', childrenPropName: 'files' }; + (unflattenParentChildArrayToTree as Mock).mockImplementationOnce(unflattenActual); }); it('should collapse all items when calling the method with collapsing True', async () => { const dataGetItemsSpy = vi.spyOn(dataViewStub, 'getItems').mockReturnValue(mockFlatDataset); vi.spyOn(dataViewStub, 'getItemCount').mockReturnValue(mockFlatDataset.length); - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue(mockHierarchical); const beginUpdateSpy = vi.spyOn(dataViewStub, 'beginUpdate'); const endUpdateSpy = vi.spyOn(dataViewStub, 'endUpdate'); const updateItemSpy = vi.spyOn(dataViewStub, 'updateItem'); @@ -344,12 +351,30 @@ describe('TreeData Service', () => { expect(pubSubSpy).toHaveBeenCalledWith(`onTreeFullToggleEnd`, { type: 'full-collapse', previousFullToggleType: 'full-collapse', toggledItems: null, }); expect(dataGetItemsSpy).toHaveBeenCalled(); expect(beginUpdateSpy).toHaveBeenCalled(); - expect(updateItemSpy).toHaveBeenNthCalledWith(1, 0, { __collapsed: true, __hasChildren: true, id: 0, file: 'TXT', size: 5.8, __treeLevel: 0 }); - expect(updateItemSpy).toHaveBeenNthCalledWith(2, 4, { __collapsed: true, __hasChildren: true, id: 4, file: 'MP3', size: 3.4, __treeLevel: 0 }); - expect(SharedService.prototype.hierarchicalDataset![0].file).toBe('TXT'); - expect(SharedService.prototype.hierarchicalDataset![0].__collapsed).toBe(true); - expect(SharedService.prototype.hierarchicalDataset![1].file).toBe('MP3'); - expect(SharedService.prototype.hierarchicalDataset![1].__collapsed).toBe(true); + expect(updateItemSpy).toHaveBeenNthCalledWith(1, 0, { + id: 0, file: 'TXT', size: 5.8, __collapsed: true, __hasChildren: true, __treeLevel: 0, + files: [ + { file: 'myFile.txt', id: 1, size: 0.5, __parentId: 0, __treeLevel: 1, }, + { file: 'myMusic.txt', id: 2, size: 5.3, __parentId: 0, __treeLevel: 1, }, + ], + }); + expect(updateItemSpy).toHaveBeenNthCalledWith(2, 4, { + id: 4, file: 'MP3', size: 3.4, __collapsed: true, __hasChildren: true, __treeLevel: 0, + files: [{ id: 5, file: 'relaxation.mp3', size: 3.4, __parentId: 4, __treeLevel: 1, },], + }); + expect(sharedService.hierarchicalDataset).toEqual([ + { + id: 0, file: 'TXT', size: 5.8, __collapsed: true, __hasChildren: true, __treeLevel: 0, + files: [ + { id: 1, file: 'myFile.txt', size: 0.5, __parentId: 0, __treeLevel: 1, }, + { id: 2, file: 'myMusic.txt', size: 5.3, __parentId: 0, __treeLevel: 1, } + ], + }, + { + id: 4, file: 'MP3', size: 3.4, __collapsed: true, __hasChildren: true, __treeLevel: 0, + files: [{ id: 5, file: 'relaxation.mp3', size: 3.4, __parentId: 4, __treeLevel: 1 }], + } + ]); expect(service.getItemCount(0)).toBe(2); // get count by tree level 0 expect(service.getItemCount(1)).toBe(3); expect(service.getItemCount()).toBe(5); // get full count of all tree @@ -372,8 +397,17 @@ describe('TreeData Service', () => { expect(dataGetItemsSpy).toHaveBeenCalled(); expect(dataGetItemsSpy).toHaveBeenCalled(); expect(beginUpdateSpy).toHaveBeenCalled(); - expect(updateItemSpy).toHaveBeenNthCalledWith(1, 0, { customCollapsed: true, __hasChildren: true, id: 0, file: 'TXT', size: 5.8, __treeLevel: 0 }); - expect(updateItemSpy).toHaveBeenNthCalledWith(2, 4, { customCollapsed: true, __hasChildren: true, id: 4, file: 'MP3', size: 3.4, __treeLevel: 0 }); + expect(updateItemSpy).toHaveBeenNthCalledWith(1, 0, { + customCollapsed: true, __hasChildren: true, id: 0, file: 'TXT', size: 5.8, __treeLevel: 0, + files: [ + { file: 'myFile.txt', id: 1, size: 0.5, __parentId: 0, __treeLevel: 1, }, + { file: 'myMusic.txt', id: 2, size: 5.3, __parentId: 0, __treeLevel: 1, }, + ], + }); + expect(updateItemSpy).toHaveBeenNthCalledWith(2, 4, { + customCollapsed: true, __hasChildren: true, id: 4, file: 'MP3', size: 3.4, __treeLevel: 0, + files: [{ file: 'relaxation.mp3', id: 5, size: 3.4, __parentId: 4, __treeLevel: 1, },], + }); expect(endUpdateSpy).toHaveBeenCalled(); }); @@ -391,8 +425,17 @@ describe('TreeData Service', () => { expect(pubSubSpy).toHaveBeenCalledWith(`onTreeFullToggleEnd`, { type: 'full-expand', previousFullToggleType: 'full-expand', toggledItems: null, }); expect(dataGetItemsSpy).toHaveBeenCalled(); expect(beginUpdateSpy).toHaveBeenCalled(); - expect(updateItemSpy).toHaveBeenNthCalledWith(1, 0, { __collapsed: false, __hasChildren: true, id: 0, file: 'TXT', size: 5.8, __treeLevel: 0 }); - expect(updateItemSpy).toHaveBeenNthCalledWith(2, 4, { __collapsed: false, __hasChildren: true, id: 4, file: 'MP3', size: 3.4, __treeLevel: 0 }); + expect(updateItemSpy).toHaveBeenNthCalledWith(1, 0, { + __collapsed: false, __hasChildren: true, id: 0, file: 'TXT', size: 5.8, __treeLevel: 0, + files: [ + { file: 'myFile.txt', id: 1, size: 0.5, __parentId: 0, __treeLevel: 1, }, + { file: 'myMusic.txt', id: 2, size: 5.3, __parentId: 0, __treeLevel: 1, }, + ], + }); + expect(updateItemSpy).toHaveBeenNthCalledWith(2, 4, { + __collapsed: false, __hasChildren: true, id: 4, file: 'MP3', size: 3.4, __treeLevel: 0, + files: [{ file: 'relaxation.mp3', id: 5, size: 3.4, __parentId: 4, __treeLevel: 1, },], + }); expect(endUpdateSpy).toHaveBeenCalled(); }); @@ -400,7 +443,7 @@ describe('TreeData Service', () => { it('should execute the method and expect a full toggle or items', () => { const dataGetItemsSpy = vi.spyOn(dataViewStub, 'getItems').mockReturnValue(mockFlatDataset); vi.spyOn(dataViewStub, 'getItemById').mockReturnValue(mockFlatDataset[3]); - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue(mockHierarchical); + vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValueOnce(mockHierarchical); const beginUpdateSpy = vi.spyOn(dataViewStub, 'beginUpdate'); const endUpdateSpy = vi.spyOn(dataViewStub, 'endUpdate'); const updateItemSpy = vi.spyOn(dataViewStub, 'updateItem'); @@ -419,7 +462,7 @@ describe('TreeData Service', () => { it('should execute the method but without calling "getItems" to skip doing a full toggle of items', () => { const dataGetItemsSpy = vi.spyOn(dataViewStub, 'getItems').mockReturnValue(mockFlatDataset); vi.spyOn(dataViewStub, 'getItemById').mockReturnValue(mockFlatDataset[3]); - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue(mockHierarchical); + vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValueOnce(mockHierarchical); const beginUpdateSpy = vi.spyOn(dataViewStub, 'beginUpdate'); const endUpdateSpy = vi.spyOn(dataViewStub, 'endUpdate'); const updateItemSpy = vi.spyOn(dataViewStub, 'updateItem'); @@ -438,7 +481,7 @@ describe('TreeData Service', () => { it('should execute the method and also trigger an event when specified', () => { const dataGetItemsSpy = vi.spyOn(dataViewStub, 'getItems').mockReturnValue(mockFlatDataset); vi.spyOn(dataViewStub, 'getItemById').mockReturnValue(mockFlatDataset[3]); - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue(mockHierarchical); + vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValueOnce(mockHierarchical); const beginUpdateSpy = vi.spyOn(dataViewStub, 'beginUpdate'); const endUpdateSpy = vi.spyOn(dataViewStub, 'endUpdate'); const updateItemSpy = vi.spyOn(dataViewStub, 'updateItem'); @@ -459,7 +502,7 @@ describe('TreeData Service', () => { describe('dynamicallyToggleItemState method', () => { it('should execute the method and also trigger an event by default', () => { vi.spyOn(dataViewStub, 'getItemById').mockReturnValue(mockFlatDataset[3]); - vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValue(mockHierarchical); + vi.spyOn(SharedService.prototype, 'hierarchicalDataset', 'get').mockReturnValueOnce(mockHierarchical); const beginUpdateSpy = vi.spyOn(dataViewStub, 'beginUpdate'); const endUpdateSpy = vi.spyOn(dataViewStub, 'endUpdate'); const updateItemSpy = vi.spyOn(dataViewStub, 'updateItem'); @@ -580,8 +623,9 @@ describe('TreeData Service', () => { // 2nd test, if we toggled all items to be collapsed, we should expect the unflatten to be called with updated `initiallyCollapsed` flag await service.toggleTreeDataCollapse(true); - service.convertFlatParentChildToTreeDatasetAndSort(mockFlatDataset, mockColumns, gridOptionsMock); - expect(unflattenParentChildArrayToTree).toHaveBeenNthCalledWith(2, mockFlatDataset, { + const result2 = service.convertFlatParentChildToTreeDatasetAndSort(mockFlatDataset, mockColumns, gridOptionsMock); + expect(result2).toEqual({ flat: mockFlatDataset as any[], hierarchical: mockHierarchical as any[] }); + expect(unflattenParentChildArrayToTree).toHaveBeenNthCalledWith(3, mockFlatDataset, { // 3rd call because toggleTreeDataCollapse() made the 2nd call columnId: 'file', identifierPropName: 'id', initiallyCollapsed: true, // changed to True diff --git a/packages/common/src/services/treeData.service.ts b/packages/common/src/services/treeData.service.ts index 93053d301..f28c4cb8c 100644 --- a/packages/common/src/services/treeData.service.ts +++ b/packages/common/src/services/treeData.service.ts @@ -183,7 +183,7 @@ export class TreeDataService { // then we reapply only the ones that changed (provided as argument to the function) treeToggledItems.forEach(collapsedItem => { const item = this.dataView.getItemById(collapsedItem.itemId); - this.updateToggledItem(item, collapsedItem.isCollapsed); + this.updateToggledItem(item, collapsedItem.isCollapsed, true); if (shouldTriggerEvent) { const parentFoundIdx = this._currentToggledItems.findIndex(treeChange => treeChange.itemId === collapsedItem.itemId); @@ -360,12 +360,17 @@ export class TreeDataService { this.dataView.beginUpdate(true); // toggle the collapsed flag but only when it's a parent item with children - (this.dataView.getItems() || []).forEach((item: any) => { + const flatDataItems = (this.dataView.getItems() || []); + flatDataItems.forEach((item: any) => { if (item[hasChildrenPropName]) { - this.updateToggledItem(item, collapsing); + this.updateToggledItem(item, collapsing, false); } }); + // instead of finding each tree item and update their collapse flag one-by-one, we can simply recreate the tree dataset + // since the flat dataset already has collapse flag updated from previous step + this.sharedService.hierarchicalDataset = this.convertFlatParentChildToTreeDataset(flatDataItems, this.gridOptions); + this.dataView.endUpdate(); this.dataView.refresh(); this._isLastFullToggleCollapsed = collapsing; @@ -437,7 +442,12 @@ export class TreeDataService { } } - protected updateToggledItem(item: any, isCollapsed: boolean): void { + /** + * Toggle an item in the flat dataset and update it in the grid. + * NOTE: We should update the Tree Data collapse status (shouldUpdateTree=true) ONLY when toggling 1 item at a time, + * however for toggle a batch (i.e. collapse all), we'll want to convert skip updating the tree but rather convert from flat to tree which is much quicker to execute. + */ + protected updateToggledItem(item: any, isCollapsed: boolean, shouldUpdateTree: boolean): void { const dataViewIdIdentifier = this.gridOptions?.datasetIdPropertyName ?? 'id'; const childrenPropName = getTreeDataOptionPropName(this.treeDataOptions, 'childrenPropName'); const collapsedPropName = getTreeDataOptionPropName(this.treeDataOptions, 'collapsedPropName'); @@ -448,10 +458,12 @@ export class TreeDataService { this.dataView.updateItem(item[dataViewIdIdentifier], item); // also update the hierarchical tree item - const searchTreePredicate = (treeItemToSearch: any) => treeItemToSearch[dataViewIdIdentifier] === item[dataViewIdIdentifier]; - const treeItemFound = findItemInTreeStructure(this.sharedService.hierarchicalDataset || [], searchTreePredicate, childrenPropName); - if (treeItemFound) { - treeItemFound[collapsedPropName] = isCollapsed; + if (shouldUpdateTree) { + const searchTreePredicate = (treeItemToSearch: any) => treeItemToSearch[dataViewIdIdentifier] === item[dataViewIdIdentifier]; + const treeItemFound = findItemInTreeStructure(this.sharedService.hierarchicalDataset || [], searchTreePredicate, childrenPropName); + if (treeItemFound) { + treeItemFound[collapsedPropName] = isCollapsed; + } } } } From 62d342a3ce92748f9f63c6a4a7124f9482a0acfd Mon Sep 17 00:00:00 2001 From: ghiscoding Date: Sun, 15 Sep 2024 01:15:43 -0400 Subject: [PATCH 2/2] chore: add type prefix to import --- packages/common/src/services/__tests__/treeData.service.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/common/src/services/__tests__/treeData.service.spec.ts b/packages/common/src/services/__tests__/treeData.service.spec.ts index 011b73955..cfb5c91bb 100644 --- a/packages/common/src/services/__tests__/treeData.service.spec.ts +++ b/packages/common/src/services/__tests__/treeData.service.spec.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, it, Mock, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, type Mock, vi } from 'vitest'; import { type BasePubSubService } from '@slickgrid-universal/event-pub-sub'; import type { Column, GridOption, BackendService } from '../../interfaces/index';