From 29111a94756c34a2e01f2431c14b7ed806349a94 Mon Sep 17 00:00:00 2001 From: Ghislain B Date: Fri, 26 Jan 2024 20:16:53 -0500 Subject: [PATCH] perf(core): convert `for..in` to `Object.keys().forEach` for better perf (#1370) * perf(core): convert `for..in` to `Object.keys().forEach` for better perf - as mentioned in this [Stack Overflow](https://stackoverflow.com/a/43446317/1212166), `Object.keys().forEach` are quicker than `for..in` because the latter also loops through prototype chain. --- .../src/core/__tests__/slickGrid.spec.ts | 12 ++- packages/common/src/core/slickGrid.ts | 78 +++++++++---------- .../__tests__/filterUtilities.spec.ts | 5 ++ .../src/filter-conditions/filterUtilities.ts | 3 + .../src/slickRowDetailView.ts | 4 +- 5 files changed, 58 insertions(+), 44 deletions(-) diff --git a/packages/common/src/core/__tests__/slickGrid.spec.ts b/packages/common/src/core/__tests__/slickGrid.spec.ts index 2343931bf..74a6bfb97 100644 --- a/packages/common/src/core/__tests__/slickGrid.spec.ts +++ b/packages/common/src/core/__tests__/slickGrid.spec.ts @@ -362,12 +362,20 @@ describe('SlickGrid core file', () => { grid.setSelectionModel(rowSelectionModel); jest.spyOn(grid.getEditorLock(), 'isActive').mockReturnValueOnce(false); + grid.setSelectedRows([1]); + grid.invalidateRow(0); + grid.invalidateRow(1); + grid.render(); grid.setSelectedRows([0, 1]); + const firstRowItemCell = container.querySelector('.slick-row:nth-child(1) .slick-cell.l0.r0') as HTMLDivElement; + const secondRowItemCell = container.querySelector('.slick-row:nth-child(2) .slick-cell.l0.r0') as HTMLDivElement; expect(setRangeSpy).toHaveBeenCalledWith([ { fromCell: 0, fromRow: 0, toCell: 0, toRow: 0 }, { fromCell: 0, fromRow: 1, toCell: 0, toRow: 1 } ], 'SlickGrid.setSelectedRows'); + expect(firstRowItemCell.classList.contains('selected')).toBeTruthy(); + expect(secondRowItemCell.classList.contains('selected')).toBeTruthy(); }); it('should not call setSelectedRanges() when editor lock isActive() is define and is returning true', () => { @@ -379,6 +387,7 @@ describe('SlickGrid core file', () => { jest.spyOn(grid.getEditorLock(), 'isActive').mockReturnValueOnce(true); grid.setSelectedRows([0, 1]); + grid.render(); expect(setRangeSpy).not.toHaveBeenCalled(); }); @@ -391,6 +400,7 @@ describe('SlickGrid core file', () => { jest.spyOn(grid, 'getEditorLock').mockReturnValue(undefined as any); grid.setSelectedRows([0, 1]); + grid.render(); expect(grid.getEditorLock()).toBeUndefined(); expect(setRangeSpy).not.toHaveBeenCalledWith([ @@ -1829,7 +1839,6 @@ describe('SlickGrid core file', () => { const updateRowSpy = jest.spyOn(grid, 'updateRow'); const onCellChangeSpy = jest.spyOn(grid.onCellChange, 'notify'); jest.spyOn(editor!, 'serializeValue').mockReturnValueOnce(newValue); - const preClickSpy = jest.spyOn(editor!, 'preClick'); grid.editActiveCell(CheckboxEditor as any, true); const result = grid.getEditController()?.commitCurrentEdit(); @@ -4125,6 +4134,7 @@ describe('SlickGrid core file', () => { // 1. add CSS Cell Style grid.addCellCssStyles('age_greater30_highlight', hash); + grid.render(); let firstItemAgeCell = container.querySelector('.slick-row:nth-child(1) .slick-cell.l1.r1') as HTMLDivElement; let secondItemAgeCell = container.querySelector('.slick-row:nth-child(2) .slick-cell.l1.r1') as HTMLDivElement; diff --git a/packages/common/src/core/slickGrid.ts b/packages/common/src/core/slickGrid.ts index eeaf55ec7..f22240217 100644 --- a/packages/common/src/core/slickGrid.ts +++ b/packages/common/src/core/slickGrid.ts @@ -916,12 +916,12 @@ export class SlickGrid = Column, O e this._hiddenParents = Utils.parents(this._container, ':hidden') as HTMLElement[]; this._hiddenParents.forEach(el => { const old: Partial = {}; - for (const name in this.cssShow) { + Object.keys(this.cssShow).forEach(name => { if (this.cssShow) { old[name as any] = el.style[name as 'position' | 'visibility' | 'display']; el.style[name as any] = this.cssShow[name as 'position' | 'visibility' | 'display']; } - } + }); this.oldProps.push(old); }); } @@ -933,11 +933,11 @@ export class SlickGrid = Column, O e if (this._hiddenParents) { this._hiddenParents.forEach(el => { const old = this.oldProps[i++]; - for (const name in this.cssShow) { + Object.keys(this.cssShow).forEach(name => { if (this.cssShow) { el.style[name as CSSStyleDeclarationWritable] = (old as any)[name]; } - } + }); }); } } @@ -1619,11 +1619,11 @@ export class SlickGrid = Column, O e } if (m.hasOwnProperty('headerCellAttrs') && m.headerCellAttrs instanceof Object) { - for (const key in m.headerCellAttrs) { + Object.keys(m.headerCellAttrs).forEach(key => { if (m.headerCellAttrs.hasOwnProperty(key)) { header.setAttribute(key, m.headerCellAttrs[key]); } - } + }); } if (m.sortable) { @@ -3349,11 +3349,11 @@ export class SlickGrid = Column, O e } // TODO: merge them together in the setter - for (const key in this.cellCssClasses) { + Object.keys(this.cellCssClasses).forEach(key => { if (this.cellCssClasses[key][row]?.[m.id]) { cellCss += ` ${this.cellCssClasses[key][row][m.id]}`; } - } + }); let value: any = null; let formatterResult: FormatterResultWithHtml | FormatterResultWithText | HTMLElement | DocumentFragment | string = ''; @@ -3411,7 +3411,7 @@ export class SlickGrid = Column, O e } protected cleanupRows(rangeToKeep: { bottom: number; top: number; }) { - for (const rowId in this.rowsCache) { + Object.keys(this.rowsCache).forEach(rowId => { if (this.rowsCache) { let i = +rowId; let removeFrozenRow = true; @@ -3431,7 +3431,7 @@ export class SlickGrid = Column, O e this.removeRowFromCache(i); } } - } + }); if (this._options.enableAsyncPostRenderCleanup) { this.startPostProcessingCleanup(); } @@ -3611,10 +3611,10 @@ export class SlickGrid = Column, O e let formatterResult; const d = this.getDataItem(row); - for (const colIdx in cacheEntry.cellNodesByColumnIdx) { + Object.keys(cacheEntry.cellNodesByColumnIdx).forEach(colIdx => { /* istanbul ignore next */ if (!cacheEntry.cellNodesByColumnIdx.hasOwnProperty(colIdx)) { - continue; + return; } const columnIdx = +colIdx; @@ -3629,7 +3629,7 @@ export class SlickGrid = Column, O e } else { emptyElement(node); } - } + }); this.invalidatePostProcessingResults(row); } @@ -3980,11 +3980,11 @@ export class SlickGrid = Column, O e // Remove cells outside the range. const cellsToRemove: number[] = []; - for (const cellNodeIdx in cacheEntry.cellNodesByColumnIdx) { + Object.keys(cacheEntry.cellNodesByColumnIdx).forEach(cellNodeIdx => { // I really hate it when people mess with Array.prototype. /* istanbul ignore if */ if (!cacheEntry.cellNodesByColumnIdx.hasOwnProperty(cellNodeIdx)) { - continue; + return; } // This is a string, so it needs to be cast back to a number. @@ -3992,12 +3992,12 @@ export class SlickGrid = Column, O e // Ignore frozen columns if (i <= this._options.frozenColumn!) { - continue; + return; } // Ignore alwaysRenderedColumns if (Array.isArray(this.columns) && this.columns[i] && this.columns[i].alwaysRenderColumn) { - continue; + return; } const colspan = cacheEntry.cellColSpans[i]; @@ -4007,7 +4007,7 @@ export class SlickGrid = Column, O e cellsToRemove.push((i as unknown as number)); } } - } + }); let cellToRemove; let cellNode; @@ -4493,23 +4493,20 @@ export class SlickGrid = Column, O e } this.ensureCellNodesInRowsCache(row); - for (const colIdx in cacheEntry.cellNodesByColumnIdx) { - if (!cacheEntry.cellNodesByColumnIdx.hasOwnProperty(colIdx)) { - continue; - } - - const columnIdx = +colIdx; - - const m = this.columns[columnIdx]; - const processedStatus = this.postProcessedRows[row][columnIdx]; // C=cleanup and re-render, R=rendered - if (m.asyncPostRender && processedStatus !== 'R') { - const node = cacheEntry.cellNodesByColumnIdx[columnIdx]; - if (node) { - m.asyncPostRender(node, row, this.getDataItem(row), m, (processedStatus === 'C')); + Object.keys(cacheEntry.cellNodesByColumnIdx).forEach(colIdx => { + if (cacheEntry.cellNodesByColumnIdx.hasOwnProperty(colIdx)) { + const columnIdx = +colIdx; + const m = this.columns[columnIdx]; + const processedStatus = this.postProcessedRows[row][columnIdx]; // C=cleanup and re-render, R=rendered + if (m.asyncPostRender && processedStatus !== 'R') { + const node = cacheEntry.cellNodesByColumnIdx[columnIdx]; + if (node) { + m.asyncPostRender(node, row, this.getDataItem(row), m, (processedStatus === 'C')); + } + this.postProcessedRows[row][columnIdx] = 'R'; } - this.postProcessedRows[row][columnIdx] = 'R'; } - } + }); this.h_postrender = setTimeout(this.asyncPostProcessRows.bind(this), this._options.asyncPostRenderDelay); return; @@ -4544,9 +4541,8 @@ export class SlickGrid = Column, O e protected updateCellCssStylesOnRenderedRows(addedHash?: CssStyleHash | null, removedHash?: CssStyleHash | null) { let node: HTMLElement | null; - let columnId: number | string; - let addedRowHash; - let removedRowHash; + let addedRowHash: any; + let removedRowHash: any; if (typeof this.rowsCache === 'object') { Object.keys(this.rowsCache).forEach(row => { if (this.rowsCache) { @@ -4554,25 +4550,25 @@ export class SlickGrid = Column, O e addedRowHash = addedHash?.[row]; if (removedRowHash) { - for (columnId in removedRowHash) { - if (!addedRowHash || removedRowHash[columnId] !== addedRowHash[columnId]) { + Object.keys(removedRowHash).forEach(columnId => { + if (!addedRowHash || removedRowHash![columnId] !== addedRowHash[columnId]) { node = this.getCellNode(+row, this.getColumnIndex(columnId)); if (node) { node.classList.remove(removedRowHash[columnId]); } } - } + }); } if (addedRowHash) { - for (columnId in addedRowHash) { + Object.keys(addedRowHash).forEach(columnId => { if (!removedRowHash || removedRowHash[columnId] !== addedRowHash[columnId]) { node = this.getCellNode(+row, this.getColumnIndex(columnId)); if (node) { node.classList.add(addedRowHash[columnId]); } } - } + }); } } }); diff --git a/packages/common/src/filter-conditions/__tests__/filterUtilities.spec.ts b/packages/common/src/filter-conditions/__tests__/filterUtilities.spec.ts index 7ce2f2ed9..3754ffb24 100644 --- a/packages/common/src/filter-conditions/__tests__/filterUtilities.spec.ts +++ b/packages/common/src/filter-conditions/__tests__/filterUtilities.spec.ts @@ -31,6 +31,11 @@ describe('filterUtilities', () => { const output = compareObjects('John', obj3, 'id'); expect(output).toBeFalsy(); }); + + it('should return False when both objects have different properties count', () => { + const output = compareObjects(obj1, obj4); + expect(output).toBeFalsy(); + }); }); describe('testFilterCondition method', () => { diff --git a/packages/common/src/filter-conditions/filterUtilities.ts b/packages/common/src/filter-conditions/filterUtilities.ts index f073b28e1..182234232 100644 --- a/packages/common/src/filter-conditions/filterUtilities.ts +++ b/packages/common/src/filter-conditions/filterUtilities.ts @@ -15,6 +15,9 @@ export function compareObjects(o1: any, o2: any, compareKey?: string): boolean { return o1[compareKey] === o2 || o1 === o2[compareKey] || o1[compareKey] === o2[compareKey]; } + if (typeof o1 === 'object' && typeof o2 === 'object' && Object.keys(o1).length !== Object.keys(o2).length) { + return false; + } // loop through all object properties to compare the full content of the object // we'll return false as soon as a difference is detected for (const p in o1) { diff --git a/packages/row-detail-view-plugin/src/slickRowDetailView.ts b/packages/row-detail-view-plugin/src/slickRowDetailView.ts index 1c0af447c..6b6fe3bfc 100644 --- a/packages/row-detail-view-plugin/src/slickRowDetailView.ts +++ b/packages/row-detail-view-plugin/src/slickRowDetailView.ts @@ -594,11 +594,11 @@ export class SlickRowDetailView implements ExternalResource, UniversalRowDetailV protected getPaddingItem(parent: any, offset: any) { const item: any = {}; - for (const prop in this.dataView) { + Object.keys(this.dataView).forEach(prop => { if (prop) { item[prop] = null; } - } + }); item[this._dataViewIdProperty] = `${parent[this._dataViewIdProperty]}.${offset}`; // additional hidden padding metadata fields