Skip to content

Commit

Permalink
perf(core): convert for..in to Object.keys().forEach for better p…
Browse files Browse the repository at this point in the history 
…erf (#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.
  • Loading branch information
@ghiscoding
ghiscoding authored Jan 27, 2024
1 parent ef4e8f9 commit 29111a9
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 44 deletions.
12 changes: 11 additions & 1 deletion packages/common/src/core/__tests__/slickGrid.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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();
});
Expand All @@ -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([
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down
78 changes: 37 additions & 41 deletions packages/common/src/core/slickGrid.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -916,12 +916,12 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
this._hiddenParents = Utils.parents(this._container, ':hidden') as HTMLElement[];
this._hiddenParents.forEach(el => {
const old: Partial<CSSStyleDeclaration> = {};
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);
});
}
Expand All @@ -933,11 +933,11 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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];
}
}
});
});
}
}
Expand Down Expand Up @@ -1619,11 +1619,11 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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) {
Expand Down Expand Up @@ -3349,11 +3349,11 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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 = '';
Expand Down Expand Up @@ -3411,7 +3411,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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;
Expand All @@ -3431,7 +3431,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
this.removeRowFromCache(i);
}
}
}
});
if (this._options.enableAsyncPostRenderCleanup) {
this.startPostProcessingCleanup();
}
Expand Down Expand Up @@ -3611,10 +3611,10 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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;
Expand All @@ -3629,7 +3629,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
} else {
emptyElement(node);
}
}
});

this.invalidatePostProcessingResults(row);
}
Expand Down Expand Up @@ -3980,24 +3980,24 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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.
const i = +cellNodeIdx;

// 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];
Expand All @@ -4007,7 +4007,7 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, O e
cellsToRemove.push((i as unknown as number));
}
}
}
});

let cellToRemove;
let cellNode;
Expand Down Expand Up @@ -4493,23 +4493,20 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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;
Expand Down Expand Up @@ -4544,35 +4541,34 @@ export class SlickGrid<TData = any, C extends Column<TData> = Column<TData>, 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) {
removedRowHash = removedHash?.[row];
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]);
}
}
}
});
}
}
});
Expand Down
5 changes: 5 additions & 0 deletions packages/common/src/filter-conditions/__tests__/filterUtilities.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
3 changes: 3 additions & 0 deletions packages/common/src/filter-conditions/filterUtilities.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
4 changes: 2 additions & 2 deletions packages/row-detail-view-plugin/src/slickRowDetailView.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 29111a9

Please sign in to comment.