From 771bdd1fa0a23cc8a18d27721ae88cb8e8470b10 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Fri, 14 Apr 2017 11:24:09 -0700 Subject: [PATCH 01/15] Add Utils.shallowCompare plus unit tests for shallowCompare and shallowCompareKeys --- packages/table/src/common/utils.ts | 46 ++++++++++++++--- packages/table/test/utilsTests.ts | 80 +++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index 65688870e0..adda282078 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -199,16 +199,24 @@ export const Utils = { return value; }, + shallowCompareKeys, + /** - * Partial shallow comparison between objects using the given list of keys. + * Returns true if the keys are the same between the two objects and the values for each key are + * shallowly equal. */ - shallowCompareKeys(objA: any, objB: any, keys: string[]) { - for (const key of keys) { - if (objA[key] !== objB[key]) { - return false; - } + shallowCompare(objA: any, objB: any) { + if (bothUndefined(objA, objB) || bothNull(objA, objB)) { + return true; + } else if (objA == null || objB == null) { + return false; + } else if (Array.isArray(objA) !== Array.isArray(objB)) { + return false; } - return true; + const keysA = Object.keys(objA); + const keysB = Object.keys(objB); + return shallowCompareKeys(objA, objB, keysA) + && shallowCompareKeys(objA, objB, keysB); }, /** @@ -334,3 +342,27 @@ export const Utils = { return event.button === 0; }, }; + +/** + * Partial shallow comparison between objects using the given list of keys. + */ +function shallowCompareKeys(objA: any, objB: any, keys: string[]) { + if (bothUndefined(objA, objB) || bothNull(objA, objB)) { + return true; + } else if (objA == null || objB == null) { + return false; + } else if (Array.isArray(objA) !== Array.isArray(objB)) { + return false; + } + return keys + .map((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key]) + .every((isEqual) => isEqual); +} + +function bothUndefined(objA: any, objB: any) { + return objA === undefined && objB === undefined; +} + +function bothNull(objA: any, objB: any) { + return objA === null && objB === null; +} diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 7f7d1e186c..ebce1159a9 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -225,7 +225,6 @@ describe("Utils", () => { }); describe("reorderArray", () => { - const ARRAY_STRING = "ABCDEFG"; const ARRAY = ARRAY_STRING.split(""); const ARRAY_LENGTH = ARRAY.length; @@ -303,4 +302,83 @@ describe("Utils", () => { expect(result).to.eql(expected.split("")); } }); + + describe("shallowCompareKeys", () => { + describe("returns true if only the specified values are shallowly equal", () => { + runTest(true, { a: 1 }, { a: 1 }, ["a"]); + runTest(true, { a: 1, b: true }, { a: 1, b: false }, ["a"]); + runTest(true, { a: 1, b: true }, { a: 1, b: false }, []); + runTest(true, { a: 1 }, { a: 1 }, ["a", "b", "c", "d"]); + runTest(true, { a: 1, b: [1, 2, 3], c: "3" }, { b: [1, 2, 3], a: 1, c: "3" }, ["a", "c"]); + runTest(true, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b"]); + }); + + describe("returns false if any specified values are not shallowly equal", () => { + runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }, ["a"]); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); + runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b", "c"]); + }); + + describe("edge cases that return true", () => { + runTest(true, undefined, undefined, []); + runTest(true, undefined, undefined, ["a"]); + runTest(true, null, null, []); + runTest(true, null, null, ["a"]); + runTest(true, {}, {}, []); + runTest(true, {}, {}, ["a"]); + }); + + describe("edge cases that return false", () => { + runTest(false, undefined, null, []); + runTest(false, null, {}, []); + runTest(false, {}, [], []); + }); + + function runTest(expectedResult: boolean, a: any, b: any, keys: string[]) { + it(`${JSON.stringify(a)} and ${JSON.stringify(b)} (keys: ${JSON.stringify(keys)})`, () => { + expect(Utils.shallowCompareKeys(a, b, keys)).to.equal(expectedResult); + }); + } + }); + + describe("shallowCompare", () => { + describe("returns true if values are shallowly equal", () => { + runTest(true, { a: 1, b: "2", c: true}, { a: 1, b: "2", c: true}); + runTest(true, {}, {}); + runTest(true, null, null); + runTest(true, undefined, undefined); + }); + + describe("returns false if values are not shallowly equal", () => { + runTest(false, undefined, null); + runTest(false, null, {}); + runTest(false, {}, []); + runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); + runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}); + }); + + describe("returns false if keys are different", () => { + runTest(false, {}, { a: 1 }); + runTest(false, { a: 1 }, {}); + runTest(false, { a: 1, b: "2" }, { b: "2" }); + runTest(false, { b: "2" }, { a: 1, b: "2" }); + runTest(false, { a: 1, b: "2", c: true}, { b: "2", c: true, d: 3 }); + }); + + describe("returns true if same deeply-comparable instance is reused in both objects", () => { + const deeplyComparableThing1 = { a: 1 }; + const deeplyComparableThing2 = [1, "2", true]; + runTest(true, { a: 1, b: deeplyComparableThing1 }, { a: 1, b: deeplyComparableThing1 }); + runTest(true, { a: 1, b: deeplyComparableThing2 }, { a: 1, b: deeplyComparableThing2 }); + }); + + function runTest(expectedResult: boolean, a: any, b: any) { + it(`${JSON.stringify(a)} and ${JSON.stringify(b)}`, () => { + expect(Utils.shallowCompare(a, b)).to.equal(expectedResult); + }); + } + }); }); From 287c001cccefbaa778f9481a76d5268703f035e6 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Fri, 14 Apr 2017 23:54:08 -0700 Subject: [PATCH 02/15] PureRender everywhere --- packages/table/src/cell/formats/jsonFormat.tsx | 2 ++ packages/table/src/cell/formats/truncatedFormat.tsx | 2 ++ packages/table/src/column.tsx | 2 ++ packages/table/src/common/contextMenuTargetWrapper.tsx | 2 ++ packages/table/src/common/loadableContent.tsx | 2 ++ packages/table/src/headers/editableName.tsx | 2 ++ packages/table/src/headers/rowHeaderCell.tsx | 2 ++ packages/table/src/interactions/menus/copyCellsMenuItem.tsx | 2 ++ packages/table/src/interactions/resizeHandle.tsx | 2 ++ 9 files changed, 18 insertions(+) diff --git a/packages/table/src/cell/formats/jsonFormat.tsx b/packages/table/src/cell/formats/jsonFormat.tsx index c827f56003..62b1e50407 100644 --- a/packages/table/src/cell/formats/jsonFormat.tsx +++ b/packages/table/src/cell/formats/jsonFormat.tsx @@ -6,6 +6,7 @@ */ import * as classNames from "classnames"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import * as Classes from "../../common/classes"; import { ITruncatedFormatProps, TruncatedFormat, TruncatedPopoverMode } from "./truncatedFormat"; @@ -29,6 +30,7 @@ export interface IJSONFormatProps extends ITruncatedFormatProps { stringify?: (obj: any) => string; } +@PureRender export class JSONFormat extends React.Component { public static defaultProps: IJSONFormatProps = { detectTruncation: true, diff --git a/packages/table/src/cell/formats/truncatedFormat.tsx b/packages/table/src/cell/formats/truncatedFormat.tsx index 8ea6330a55..57122fa5a6 100644 --- a/packages/table/src/cell/formats/truncatedFormat.tsx +++ b/packages/table/src/cell/formats/truncatedFormat.tsx @@ -8,6 +8,7 @@ import { Classes as CoreClasses, IProps, Popover, Position } from "@blueprintjs/core"; import * as classNames from "classnames"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import * as Classes from "../../common/classes"; @@ -66,6 +67,7 @@ export interface ITruncatedFormatState { isTruncated: boolean; } +@PureRender export class TruncatedFormat extends React.Component { public static defaultProps: ITruncatedFormatProps = { detectTruncation: true, diff --git a/packages/table/src/column.tsx b/packages/table/src/column.tsx index 43e2436f9a..85be21bbf1 100644 --- a/packages/table/src/column.tsx +++ b/packages/table/src/column.tsx @@ -5,6 +5,7 @@ * and https://github.com/palantir/blueprint/blob/master/PATENTS */ +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import { IProps } from "@blueprintjs/core"; @@ -46,6 +47,7 @@ export interface IColumnProps extends IColumnNameProps, IProps { renderColumnHeader?: IColumnHeaderRenderer; } +@PureRender export class Column extends React.Component { public static defaultProps: IColumnProps = { renderCell: emptyCellRenderer, diff --git a/packages/table/src/common/contextMenuTargetWrapper.tsx b/packages/table/src/common/contextMenuTargetWrapper.tsx index 9c47515dc5..a519873cbf 100644 --- a/packages/table/src/common/contextMenuTargetWrapper.tsx +++ b/packages/table/src/common/contextMenuTargetWrapper.tsx @@ -6,6 +6,7 @@ */ import { ContextMenuTarget, IProps } from "@blueprintjs/core"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; export interface IContextMenuTargetWrapper extends IProps { @@ -20,6 +21,7 @@ export interface IContextMenuTargetWrapper extends IProps { * chains. */ @ContextMenuTarget +@PureRender export class ContextMenuTargetWrapper extends React.Component { public render() { const { className, children, style } = this.props; diff --git a/packages/table/src/common/loadableContent.tsx b/packages/table/src/common/loadableContent.tsx index 0e2ac70743..1919f3d009 100644 --- a/packages/table/src/common/loadableContent.tsx +++ b/packages/table/src/common/loadableContent.tsx @@ -5,6 +5,7 @@ * and https://github.com/palantir/blueprint/blob/master/PATENTS */ +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import { Classes } from "@blueprintjs/core"; @@ -24,6 +25,7 @@ export interface ILoadableContentProps { } // This class expects a single, non-string child. +@PureRender export class LoadableContent extends React.Component { private style: React.CSSProperties; diff --git a/packages/table/src/headers/editableName.tsx b/packages/table/src/headers/editableName.tsx index 21fef75fb4..ab60b03199 100644 --- a/packages/table/src/headers/editableName.tsx +++ b/packages/table/src/headers/editableName.tsx @@ -7,6 +7,7 @@ import { EditableText, IIntentProps, IProps } from "@blueprintjs/core"; import * as classNames from "classnames"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import * as Classes from "../common/classes"; @@ -37,6 +38,7 @@ export interface IEditableNameProps extends IIntentProps, IProps { onConfirm?: (value: string) => void; } +@PureRender export class EditableName extends React.Component { public render() { const { className, intent, name, onCancel, onChange, onConfirm } = this.props; diff --git a/packages/table/src/headers/rowHeaderCell.tsx b/packages/table/src/headers/rowHeaderCell.tsx index 604589eda2..95051df53d 100644 --- a/packages/table/src/headers/rowHeaderCell.tsx +++ b/packages/table/src/headers/rowHeaderCell.tsx @@ -6,6 +6,7 @@ */ import * as classNames from "classnames"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import { Classes as CoreClasses, ContextMenuTarget, IProps } from "@blueprintjs/core"; @@ -67,6 +68,7 @@ export interface IRowHeaderState { } @ContextMenuTarget +@PureRender export class RowHeaderCell extends React.Component { public state = { isActive: false, diff --git a/packages/table/src/interactions/menus/copyCellsMenuItem.tsx b/packages/table/src/interactions/menus/copyCellsMenuItem.tsx index 096501afd8..5bc4a873aa 100644 --- a/packages/table/src/interactions/menus/copyCellsMenuItem.tsx +++ b/packages/table/src/interactions/menus/copyCellsMenuItem.tsx @@ -6,6 +6,7 @@ */ import { IMenuItemProps, MenuItem } from "@blueprintjs/core"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import { Clipboard } from "../../common/clipboard"; @@ -37,6 +38,7 @@ export interface ICopyCellsMenuItemProps extends IMenuItemProps { onCopy?: (success: boolean) => void; } +@PureRender export class CopyCellsMenuItem extends React.Component { public render() { return ; diff --git a/packages/table/src/interactions/resizeHandle.tsx b/packages/table/src/interactions/resizeHandle.tsx index c3c8412f60..c135c3c232 100644 --- a/packages/table/src/interactions/resizeHandle.tsx +++ b/packages/table/src/interactions/resizeHandle.tsx @@ -7,6 +7,7 @@ import { IProps } from "@blueprintjs/core"; import * as classNames from "classnames"; +import * as PureRender from "pure-render-decorator"; import * as React from "react"; import * as Classes from "../common/classes"; @@ -57,6 +58,7 @@ export interface IResizeHandleState { isDragging: boolean; } +@PureRender export class ResizeHandle extends React.Component { public state: IResizeHandleState = { isDragging: false, From 07dd7a644c344daceee0693b63e3c50730ce6752 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Sat, 15 Apr 2017 01:30:39 -0700 Subject: [PATCH 03/15] Add shouldComponentUpdate to GuideLayer --- packages/table/src/layers/guides.tsx | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/table/src/layers/guides.tsx b/packages/table/src/layers/guides.tsx index 64725df45a..0bd1a387e0 100644 --- a/packages/table/src/layers/guides.tsx +++ b/packages/table/src/layers/guides.tsx @@ -24,6 +24,17 @@ export interface IGuideLayerProps extends IProps { } export class GuideLayer extends React.Component { + public shouldComponentUpdate(nextProps: IGuideLayerProps) { + if (this.props.className !== nextProps.className) { + return false; + } + // shallow-comparing guide arrays leads to tons of unnecessary re-renders, so we check the + // array contents explicitly. + const verticalGuidesEqual = this.guidesArraysEqual(this.props.verticalGuides, nextProps.verticalGuides); + const horizontalGuidesEqual = this.guidesArraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); + return !verticalGuidesEqual || !horizontalGuidesEqual; + } + public render() { const { verticalGuides, horizontalGuides, className } = this.props; const verticals = (verticalGuides == null) ? undefined : verticalGuides.map(this.renderVerticalGuide); @@ -59,4 +70,16 @@ export class GuideLayer extends React.Component {
); } + + private guidesArraysEqual(arrA: number[], arrB: number[]) { + if (arrA === undefined && arrB === undefined) { + return true; + } else if (arrA === null && arrB === null) { + return true; + } else if (arrA == null || arrB == null || arrA.length !== arrB.length) { + return false; + } else { + return arrA.every((valA, i) => valA === arrB[i]); + } + } } From 7a46219e122294f5eff470fc1125428ab0100b7f Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Sat, 15 Apr 2017 01:33:49 -0700 Subject: [PATCH 04/15] Modify shallowCompareKeys to optionally check all keys --- packages/table/src/common/utils.ts | 35 ++++---- packages/table/test/utilsTests.ts | 134 +++++++++++++++-------------- 2 files changed, 87 insertions(+), 82 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index adda282078..d4018aff5f 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -199,24 +199,27 @@ export const Utils = { return value; }, - shallowCompareKeys, - /** - * Returns true if the keys are the same between the two objects and the values for each key are - * shallowly equal. + * Returns `true` if the mapped values for each of the specified keys are shallowly equal + * between the two objects. If `keys` is not provided, returns `true` if all keys and mapped + * values are shallowly equal between the two objects. */ - shallowCompare(objA: any, objB: any) { - if (bothUndefined(objA, objB) || bothNull(objA, objB)) { - return true; - } else if (objA == null || objB == null) { - return false; - } else if (Array.isArray(objA) !== Array.isArray(objB)) { - return false; + shallowCompareKeys(objA: any, objB: any, keys?: string[]) { + if (keys != null) { + return shallowCompareKeys(objA, objB, keys); + } else { + if (bothUndefined(objA, objB) || bothNull(objA, objB)) { + return true; + } else if (objA == null || objB == null) { + return false; + } else if (Array.isArray(objA) !== Array.isArray(objB)) { + return false; + } + const keysA = Object.keys(objA); + const keysB = Object.keys(objB); + return shallowCompareKeys(objA, objB, keysA) + && shallowCompareKeys(objA, objB, keysB); } - const keysA = Object.keys(objA); - const keysB = Object.keys(objB); - return shallowCompareKeys(objA, objB, keysA) - && shallowCompareKeys(objA, objB, keysB); }, /** @@ -346,7 +349,7 @@ export const Utils = { /** * Partial shallow comparison between objects using the given list of keys. */ -function shallowCompareKeys(objA: any, objB: any, keys: string[]) { +function shallowCompareKeys(objA: any, objB: any, keys?: string[]) { if (bothUndefined(objA, objB) || bothNull(objA, objB)) { return true; } else if (objA == null || objB == null) { diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index ebce1159a9..510e3847aa 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -303,82 +303,84 @@ describe("Utils", () => { } }); - describe("shallowCompareKeys", () => { - describe("returns true if only the specified values are shallowly equal", () => { - runTest(true, { a: 1 }, { a: 1 }, ["a"]); - runTest(true, { a: 1, b: true }, { a: 1, b: false }, ["a"]); - runTest(true, { a: 1, b: true }, { a: 1, b: false }, []); - runTest(true, { a: 1 }, { a: 1 }, ["a", "b", "c", "d"]); - runTest(true, { a: 1, b: [1, 2, 3], c: "3" }, { b: [1, 2, 3], a: 1, c: "3" }, ["a", "c"]); - runTest(true, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b"]); - }); - - describe("returns false if any specified values are not shallowly equal", () => { - runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }, ["a"]); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); - runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b", "c"]); - }); + describe.only("shallowCompareKeys", () => { + describe("with `keys` defined", () => { + describe("returns true if only the specified values are shallowly equal", () => { + runTest(true, { a: 1 }, { a: 1 }, ["a"]); + runTest(true, { a: 1, b: true }, { a: 1, b: false }, ["a"]); + runTest(true, { a: 1, b: true }, { a: 1, b: false }, []); + runTest(true, { a: 1 }, { a: 1 }, ["a", "b", "c", "d"]); + runTest(true, { a: 1, b: [1, 2, 3], c: "3" }, { b: [1, 2, 3], a: 1, c: "3" }, ["a", "c"]); + runTest(true, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b"]); + }); - describe("edge cases that return true", () => { - runTest(true, undefined, undefined, []); - runTest(true, undefined, undefined, ["a"]); - runTest(true, null, null, []); - runTest(true, null, null, ["a"]); - runTest(true, {}, {}, []); - runTest(true, {}, {}, ["a"]); - }); + describe("returns false if any specified values are not shallowly equal", () => { + runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }, ["a"]); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); + runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b", "c"]); + }); - describe("edge cases that return false", () => { - runTest(false, undefined, null, []); - runTest(false, null, {}, []); - runTest(false, {}, [], []); - }); + describe("edge cases that return true", () => { + runTest(true, undefined, undefined, []); + runTest(true, undefined, undefined, ["a"]); + runTest(true, null, null, []); + runTest(true, null, null, ["a"]); + runTest(true, {}, {}, []); + runTest(true, {}, {}, ["a"]); + }); - function runTest(expectedResult: boolean, a: any, b: any, keys: string[]) { - it(`${JSON.stringify(a)} and ${JSON.stringify(b)} (keys: ${JSON.stringify(keys)})`, () => { - expect(Utils.shallowCompareKeys(a, b, keys)).to.equal(expectedResult); + describe("edge cases that return false", () => { + runTest(false, undefined, null, []); + runTest(false, null, {}, []); + runTest(false, {}, [], []); }); - } - }); - describe("shallowCompare", () => { - describe("returns true if values are shallowly equal", () => { - runTest(true, { a: 1, b: "2", c: true}, { a: 1, b: "2", c: true}); - runTest(true, {}, {}); - runTest(true, null, null); - runTest(true, undefined, undefined); + function runTest(expectedResult: boolean, a: any, b: any, keys: string[]) { + it(`${JSON.stringify(a)} and ${JSON.stringify(b)} (keys: ${JSON.stringify(keys)})`, () => { + expect(Utils.shallowCompareKeys(a, b, keys)).to.equal(expectedResult); + }); + } }); - describe("returns false if values are not shallowly equal", () => { - runTest(false, undefined, null); - runTest(false, null, {}); - runTest(false, {}, []); - runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); - runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}); - }); + describe("with `keys` not defined", () => { + describe("returns true if values are shallowly equal", () => { + runTest(true, { a: 1, b: "2", c: true}, { a: 1, b: "2", c: true}); + runTest(true, {}, {}); + runTest(true, null, null); + runTest(true, undefined, undefined); + }); - describe("returns false if keys are different", () => { - runTest(false, {}, { a: 1 }); - runTest(false, { a: 1 }, {}); - runTest(false, { a: 1, b: "2" }, { b: "2" }); - runTest(false, { b: "2" }, { a: 1, b: "2" }); - runTest(false, { a: 1, b: "2", c: true}, { b: "2", c: true, d: 3 }); - }); + describe("returns false if values are not shallowly equal", () => { + runTest(false, undefined, null); + runTest(false, null, {}); + runTest(false, {}, []); + runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); + runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); + runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}); + }); - describe("returns true if same deeply-comparable instance is reused in both objects", () => { - const deeplyComparableThing1 = { a: 1 }; - const deeplyComparableThing2 = [1, "2", true]; - runTest(true, { a: 1, b: deeplyComparableThing1 }, { a: 1, b: deeplyComparableThing1 }); - runTest(true, { a: 1, b: deeplyComparableThing2 }, { a: 1, b: deeplyComparableThing2 }); - }); + describe("returns false if keys are different", () => { + runTest(false, {}, { a: 1 }); + runTest(false, { a: 1 }, {}); + runTest(false, { a: 1, b: "2" }, { b: "2" }); + runTest(false, { b: "2" }, { a: 1, b: "2" }); + runTest(false, { a: 1, b: "2", c: true}, { b: "2", c: true, d: 3 }); + }); - function runTest(expectedResult: boolean, a: any, b: any) { - it(`${JSON.stringify(a)} and ${JSON.stringify(b)}`, () => { - expect(Utils.shallowCompare(a, b)).to.equal(expectedResult); + describe("returns true if same deeply-comparable instance is reused in both objects", () => { + const deeplyComparableThing1 = { a: 1 }; + const deeplyComparableThing2 = [1, "2", true]; + runTest(true, { a: 1, b: deeplyComparableThing1 }, { a: 1, b: deeplyComparableThing1 }); + runTest(true, { a: 1, b: deeplyComparableThing2 }, { a: 1, b: deeplyComparableThing2 }); }); - } + + function runTest(expectedResult: boolean, a: any, b: any) { + it(`${JSON.stringify(a)} and ${JSON.stringify(b)}`, () => { + expect(Utils.shallowCompareKeys(a, b)).to.equal(expectedResult); + }); + } + }); }); }); From 0d13d003cd9e3f9421f4dac36375faeb6ad144a8 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 10:33:09 -0700 Subject: [PATCH 05/15] Add Utils.arraysEqual + tests --- packages/table/src/common/utils.ts | 16 +++++++++++ packages/table/test/utilsTests.ts | 45 ++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index d4018aff5f..3408b60243 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -344,6 +344,22 @@ export const Utils = { isLeftClick(event: MouseEvent) { return event.button === 0; }, + + /** + * Returns true if the arrays are equal based on the provided compare function. If no compare + * function is provided, each element will be shallowly compared to determine equality. + */ + arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) { + if (arrA === undefined || arrB === undefined) { + return true; + } else if (arrA === null || arrB === null) { + return true; + } else if (arrA == null || arrB == null || arrA.length !== arrB.length) { + return false; + } else { + return arrA.every((a, i) => compare == null ? a === arrB[i] : compare(a, arrB[i])); + } + }, }; /** diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 510e3847aa..2b24278a92 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -383,4 +383,49 @@ describe("Utils", () => { } }); }); + + describe("arraysEqual", () => { + describe("no compare function provided", () => { + it("should return true if the arrays are shallowly equal", () => { + runTest(true, undefined, undefined); + runTest(true, null, null); + runTest(true, [], []); + runTest(true, [3], [3]); + runTest(true, [3, "1", true], [3, "1", true]); + }); + + it("should return false if the arrays are not shallowly equal", () => { + runTest(false, undefined, null); + runTest(false, null, []); + runTest(false, [3], []); + runTest(false, [3, 1, 2], [3, 1]); + runTest(false, [{}], [{}]); + runTest(false, [{ x: 1 }], [{ x: 1 }]); + }); + }); + + describe("compare function provided", () => { + const COMPARE_FN = (a: any, b: any) => a.x === b.x; + + it("should return true if the arrays are equal using a custom compare function", () => { + runTest(true, undefined, undefined, COMPARE_FN); + runTest(true, null, null, COMPARE_FN); + runTest(true, [], [], COMPARE_FN); + runTest(true, [{}, {}], [{}, {}], COMPARE_FN); + runTest(true, [{ x: 1 }, { x: 2 }], [{ x: 1 }, { x: 2 }], COMPARE_FN); + }); + + it("should return false if the arrays are not equal using custom compare function", () => { + runTest(false, undefined, null); + runTest(false, null, []); + runTest(false, [{ x: 1 }, {}], [{ x: 1 }, { x: 2 }], COMPARE_FN); + }); + }); + + function runTest(expectedResult: boolean, a: any, b: any, compareFn?: (a: any, b: any) => boolean) { + it(`${JSON.stringify(a)} and ${JSON.stringify(b)}`, () => { + expect(Utils.arraysEqual(a, b, compareFn)).to.equal(expectedResult); + }); + } + }); }); From 244366b0538137c8bb6b82c7391b83e80274bf14 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 10:33:39 -0700 Subject: [PATCH 06/15] Refactor RegionLayer to receive pre-computed regionStyles. avoids unnecessary re-renders! --- packages/table/src/layers/regions.tsx | 29 +++++++++++++++++++-------- packages/table/src/regions.ts | 10 ++++----- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/packages/table/src/layers/regions.tsx b/packages/table/src/layers/regions.tsx index 6ea8718737..a2ac816113 100644 --- a/packages/table/src/layers/regions.tsx +++ b/packages/table/src/layers/regions.tsx @@ -7,10 +7,10 @@ import { IProps } from "@blueprintjs/core"; import * as classNames from "classnames"; -import * as PureRender from "pure-render-decorator"; import * as React from "react"; import * as Classes from "../common/classes"; -import { IRegion } from "../regions"; +import { Utils } from "../common/utils"; +import { IRegion, Regions } from "../regions"; export type IRegionStyler = (region: IRegion) => React.CSSProperties; @@ -21,13 +21,26 @@ export interface IRegionLayerProps extends IProps { regions?: IRegion[]; /** - * A callback interface for applying CSS styles to the regions. + * The array of CSS styles to apply to each corresponding region. */ - getRegionStyle: IRegionStyler; + regionStyles?: React.CSSProperties[]; } -@PureRender +// don't include "regions" or "regionStyles" in here, because they can't be shallowly compared +const UPDATE_PROPS_KEYS = [ + "className", +]; + export class RegionLayer extends React.Component { + public shouldComponentUpdate(nextProps: IRegionLayerProps) { + // shallowly comparable props like "className" tend not to change in the default table + // implementation, so do that check last with hope that we return earlier and avoid it + // altogether. + return !Utils.arraysEqual(this.props.regions, nextProps.regions, Regions.regionsEqual) + || !Utils.arraysEqual(this.props.regionStyles, nextProps.regionStyles, Utils.shallowCompareKeys) + || !Utils.shallowCompareKeys(this.props, nextProps, UPDATE_PROPS_KEYS); + } + public render() { return
{this.renderRegionChildren()}
; } @@ -40,13 +53,13 @@ export class RegionLayer extends React.Component { return regions.map(this.renderRegion); } - private renderRegion = (region: IRegion, index: number) => { - const { className, getRegionStyle } = this.props; + private renderRegion = (_region: IRegion, index: number) => { + const { className, regionStyles } = this.props; return (
); } diff --git a/packages/table/src/regions.ts b/packages/table/src/regions.ts index 75aead1cc9..f39e3833b8 100644 --- a/packages/table/src/regions.ts +++ b/packages/table/src/regions.ts @@ -528,6 +528,11 @@ export class Regions { return regionGroups; } + public static regionsEqual(regionA: IRegion, regionB: IRegion) { + return Regions.intervalsEqual(regionA.rows, regionB.rows) + && Regions.intervalsEqual(regionA.cols, regionB.cols); + } + /** * Iterates over the cells within an `IRegion`, invoking the callback with * each cell's coordinates. @@ -573,11 +578,6 @@ export class Regions { } } - private static regionsEqual(regionA: IRegion, regionB: IRegion) { - return Regions.intervalsEqual(regionA.rows, regionB.rows) - && Regions.intervalsEqual(regionA.cols, regionB.cols); - } - private static regionContains(regionA: IRegion, regionB: IRegion) { // containsRegion expects an array of regions as the first param return Regions.overlapsRegion([regionA], regionB, false); From af8d0b220c87414aefbf0141351a03cdd737d57a Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 11:34:58 -0700 Subject: [PATCH 07/15] Minor cleanups throughout --- packages/table/src/common/utils.ts | 11 +- packages/table/src/layers/guides.tsx | 17 +- packages/table/src/layers/regions.tsx | 2 +- packages/table/src/table.tsx | 215 ++++++++++++-------------- packages/table/test/utilsTests.ts | 2 +- 5 files changed, 112 insertions(+), 135 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index 3408b60243..dc0edad1b2 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -200,9 +200,8 @@ export const Utils = { }, /** - * Returns `true` if the mapped values for each of the specified keys are shallowly equal - * between the two objects. If `keys` is not provided, returns `true` if all keys and mapped - * values are shallowly equal between the two objects. + * Shallow comparison between objects. If `keys` is provided, just that subset of keys will be + * compared; otherwise, all keys will be compared. */ shallowCompareKeys(objA: any, objB: any, keys?: string[]) { if (keys != null) { @@ -346,8 +345,8 @@ export const Utils = { }, /** - * Returns true if the arrays are equal based on the provided compare function. If no compare - * function is provided, each element will be shallowly compared to determine equality. + * Returns true if the arrays are equal. Elements will be shallowly compared by default, or they + * will be compared using the custom `compare` function if one is provided. */ arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) { if (arrA === undefined || arrB === undefined) { @@ -365,7 +364,7 @@ export const Utils = { /** * Partial shallow comparison between objects using the given list of keys. */ -function shallowCompareKeys(objA: any, objB: any, keys?: string[]) { +function shallowCompareKeys(objA: any, objB: any, keys: string[]) { if (bothUndefined(objA, objB) || bothNull(objA, objB)) { return true; } else if (objA == null || objB == null) { diff --git a/packages/table/src/layers/guides.tsx b/packages/table/src/layers/guides.tsx index 0bd1a387e0..54de2919dd 100644 --- a/packages/table/src/layers/guides.tsx +++ b/packages/table/src/layers/guides.tsx @@ -10,6 +10,7 @@ import * as classNames from "classnames"; import * as React from "react"; import * as Classes from "../common/classes"; +import { Utils } from "../common/utils"; export interface IGuideLayerProps extends IProps { /** @@ -30,8 +31,8 @@ export class GuideLayer extends React.Component { } // shallow-comparing guide arrays leads to tons of unnecessary re-renders, so we check the // array contents explicitly. - const verticalGuidesEqual = this.guidesArraysEqual(this.props.verticalGuides, nextProps.verticalGuides); - const horizontalGuidesEqual = this.guidesArraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); + const verticalGuidesEqual = Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides); + const horizontalGuidesEqual = Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); return !verticalGuidesEqual || !horizontalGuidesEqual; } @@ -70,16 +71,4 @@ export class GuideLayer extends React.Component {
); } - - private guidesArraysEqual(arrA: number[], arrB: number[]) { - if (arrA === undefined && arrB === undefined) { - return true; - } else if (arrA === null && arrB === null) { - return true; - } else if (arrA == null || arrB == null || arrA.length !== arrB.length) { - return false; - } else { - return arrA.every((valA, i) => valA === arrB[i]); - } - } } diff --git a/packages/table/src/layers/regions.tsx b/packages/table/src/layers/regions.tsx index a2ac816113..59f0ef6df2 100644 --- a/packages/table/src/layers/regions.tsx +++ b/packages/table/src/layers/regions.tsx @@ -21,7 +21,7 @@ export interface IRegionLayerProps extends IProps { regions?: IRegion[]; /** - * The array of CSS styles to apply to each corresponding region. + * The array of CSS styles to apply to each region. */ regionStyles?: React.CSSProperties[]; } diff --git a/packages/table/src/table.tsx b/packages/table/src/table.tsx index 3dddd5b660..ab46732e56 100644 --- a/packages/table/src/table.tsx +++ b/packages/table/src/table.tsx @@ -579,7 +579,7 @@ export class Table extends AbstractComponent { ref={this.setMenuRef} onClick={this.selectAll} > - {this.maybeRenderMenuRegions()} + {this.maybeRenderRegions(this.styleMenuRegion)}
); } @@ -663,7 +663,7 @@ export class Table extends AbstractComponent { onFocus={this.handleFocus} onLayoutLock={this.handleLayoutLock} onReordered={this.handleColumnsReordered} - onReordering={this.handleColumnReorderPreview} + onReordering={this.handleColumnsReordering} onResizeGuide={this.handleColumnResizeGuide} onSelection={this.getEnabledSelectionHandler(RegionCardinality.FULL_COLUMNS)} selectedRegions={selectedRegions} @@ -674,7 +674,7 @@ export class Table extends AbstractComponent { {this.props.children} - {this.maybeRenderColumnHeaderRegions()} + {this.maybeRenderRegions(this.styleColumnHeaderRegion)}
); } @@ -715,7 +715,7 @@ export class Table extends AbstractComponent { onLayoutLock={this.handleLayoutLock} onResizeGuide={this.handleRowResizeGuide} onReordered={this.handleRowsReordered} - onReordering={this.handleRowReordering} + onReordering={this.handleRowsReordering} onRowHeightChanged={this.handleRowHeightChanged} onSelection={this.getEnabledSelectionHandler(RegionCardinality.FULL_ROWS)} renderRowHeader={renderRowHeader} @@ -725,7 +725,7 @@ export class Table extends AbstractComponent { {...rowIndices} /> - {this.maybeRenderRowHeaderRegions()} + {this.maybeRenderRegions(this.styleRowHeaderRegion)}
); } @@ -794,7 +794,7 @@ export class Table extends AbstractComponent { {...columnIndices} /> - {this.maybeRenderBodyRegions()} + {this.maybeRenderRegions(this.styleBodyRegion)} { ); return regionGroups.map((regionGroup, index) => { + const regionStyles = regionGroup.regions.map(getRegionStyle); return ( ); }); @@ -952,116 +953,104 @@ export class Table extends AbstractComponent { } } - private maybeRenderBodyRegions() { - const styler = (region: IRegion): React.CSSProperties => { - const cardinality = Regions.getRegionCardinality(region); - const style = this.grid.getRegionStyle(region); - switch (cardinality) { - case RegionCardinality.CELLS: - return style; - - case RegionCardinality.FULL_COLUMNS: - style.top = "-1px"; - return style; - - case RegionCardinality.FULL_ROWS: - style.left = "-1px"; - return style; - - case RegionCardinality.FULL_TABLE: - style.left = "-1px"; - style.top = "-1px"; - return style; - - default: - return { display: "none" }; - } - }; - return this.maybeRenderRegions(styler); + private styleBodyRegion = (region: IRegion): React.CSSProperties => { + const cardinality = Regions.getRegionCardinality(region); + const style = this.grid.getRegionStyle(region); + switch (cardinality) { + case RegionCardinality.CELLS: + return style; + + case RegionCardinality.FULL_COLUMNS: + style.top = "-1px"; + return style; + + case RegionCardinality.FULL_ROWS: + style.left = "-1px"; + return style; + + case RegionCardinality.FULL_TABLE: + style.left = "-1px"; + style.top = "-1px"; + return style; + + default: + return { display: "none" }; + } } - private maybeRenderMenuRegions() { - const styler = (region: IRegion): React.CSSProperties => { - const { grid } = this; - const { viewportRect } = this.state; - if (viewportRect == null) { - return {}; - } - const cardinality = Regions.getRegionCardinality(region); - const style = grid.getRegionStyle(region); - - switch (cardinality) { - case RegionCardinality.FULL_TABLE: - style.right = "0px"; - style.bottom = "0px"; - style.top = "0px"; - style.left = "0px"; - style.borderBottom = "none"; - style.borderRight = "none"; - return style; - - default: - return { display: "none" }; - } - }; - return this.maybeRenderRegions(styler); + private styleMenuRegion = (region: IRegion): React.CSSProperties => { + const { grid } = this; + const { viewportRect } = this.state; + if (viewportRect == null) { + return {}; + } + const cardinality = Regions.getRegionCardinality(region); + const style = grid.getRegionStyle(region); + + switch (cardinality) { + case RegionCardinality.FULL_TABLE: + style.right = "0px"; + style.bottom = "0px"; + style.top = "0px"; + style.left = "0px"; + style.borderBottom = "none"; + style.borderRight = "none"; + return style; + + default: + return { display: "none" }; + } } - private maybeRenderColumnHeaderRegions() { - const styler = (region: IRegion): React.CSSProperties => { - const { grid } = this; - const { viewportRect } = this.state; - if (viewportRect == null) { - return {}; - } - const cardinality = Regions.getRegionCardinality(region); - const style = grid.getRegionStyle(region); - - switch (cardinality) { - case RegionCardinality.FULL_TABLE: - style.left = "-1px"; - style.borderLeft = "none"; - style.bottom = "-1px"; - style.transform = `translate3d(${-viewportRect.left}px, 0, 0)`; - return style; - case RegionCardinality.FULL_COLUMNS: - style.bottom = "-1px"; - style.transform = `translate3d(${-viewportRect.left}px, 0, 0)`; - return style; - - default: - return { display: "none" }; - } - }; - return this.maybeRenderRegions(styler); + private styleColumnHeaderRegion = (region: IRegion): React.CSSProperties => { + const { grid } = this; + const { viewportRect } = this.state; + if (viewportRect == null) { + return {}; + } + const cardinality = Regions.getRegionCardinality(region); + const style = grid.getRegionStyle(region); + + switch (cardinality) { + case RegionCardinality.FULL_TABLE: + style.left = "-1px"; + style.borderLeft = "none"; + style.bottom = "-1px"; + style.transform = `translate3d(${-viewportRect.left}px, 0, 0)`; + return style; + case RegionCardinality.FULL_COLUMNS: + style.bottom = "-1px"; + style.transform = `translate3d(${-viewportRect.left}px, 0, 0)`; + return style; + + default: + return { display: "none" }; + } } - private maybeRenderRowHeaderRegions() { - const styler = (region: IRegion): React.CSSProperties => { - const { grid } = this; - const { viewportRect } = this.state; - if (viewportRect == null) { - return {}; - } - const cardinality = Regions.getRegionCardinality(region); - const style = grid.getRegionStyle(region); - switch (cardinality) { - case RegionCardinality.FULL_TABLE: - style.top = "-1px"; - style.borderTop = "none"; - style.right = "-1px"; - style.transform = `translate3d(0, ${-viewportRect.top}px, 0)`; - return style; - case RegionCardinality.FULL_ROWS: - style.right = "-1px"; - style.transform = `translate3d(0, ${-viewportRect.top}px, 0)`; - return style; - - default: - return { display: "none" }; - } - }; - return this.maybeRenderRegions(styler); + private styleRowHeaderRegion = (region: IRegion): React.CSSProperties => { + const { grid } = this; + const { viewportRect } = this.state; + if (viewportRect == null) { + return {}; + } + const cardinality = Regions.getRegionCardinality(region); + const style = grid.getRegionStyle(region); + switch (cardinality) { + case RegionCardinality.FULL_TABLE: + style.top = "-1px"; + style.borderTop = "none"; + style.right = "-1px"; + style.transform = `translate3d(0, ${-viewportRect.top}px, 0)`; + return style; + case RegionCardinality.FULL_ROWS: + style.right = "-1px"; + style.transform = `translate3d(0, ${-viewportRect.top}px, 0)`; + return style; + + default: + return { display: "none" }; + } } private handleColumnWidthChanged = (columnIndex: number, width: number) => { @@ -1215,7 +1204,7 @@ export class Table extends AbstractComponent { } } - private handleColumnReorderPreview = (oldIndex: number, newIndex: number, length: number) => { + private handleColumnsReordering = (oldIndex: number, newIndex: number, length: number) => { const guideIndex = Utils.reorderedIndexToGuideIndex(oldIndex, newIndex, length); const leftOffset = this.grid.getCumulativeWidthBefore(guideIndex); this.setState({ isReordering: true, verticalGuides: [leftOffset] } as ITableState); @@ -1226,7 +1215,7 @@ export class Table extends AbstractComponent { BlueprintUtils.safeInvoke(this.props.onColumnsReordered, oldIndex, newIndex, length); } - private handleRowReordering = (oldIndex: number, newIndex: number, length: number) => { + private handleRowsReordering = (oldIndex: number, newIndex: number, length: number) => { const guideIndex = Utils.reorderedIndexToGuideIndex(oldIndex, newIndex, length); const topOffset = this.grid.getCumulativeHeightBefore(guideIndex); this.setState({ isReordering: true, horizontalGuides: [topOffset] } as ITableState); diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 2b24278a92..030e83da08 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -303,7 +303,7 @@ describe("Utils", () => { } }); - describe.only("shallowCompareKeys", () => { + describe("shallowCompareKeys", () => { describe("with `keys` defined", () => { describe("returns true if only the specified values are shallowly equal", () => { runTest(true, { a: 1 }, { a: 1 }, ["a"]); From 6dffba39e35a9b314e1e8488b4313670b1d1869b Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 12:34:52 -0700 Subject: [PATCH 08/15] Eek, was testing arraysEqual improperly. Fixed a bug. --- packages/table/src/common/utils.ts | 4 ++-- packages/table/test/utilsTests.ts | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index dc0edad1b2..5a6b4c1bb6 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -349,9 +349,9 @@ export const Utils = { * will be compared using the custom `compare` function if one is provided. */ arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) { - if (arrA === undefined || arrB === undefined) { + if (arrA === undefined && arrB === undefined) { return true; - } else if (arrA === null || arrB === null) { + } else if (arrA === null && arrB === null) { return true; } else if (arrA == null || arrB == null || arrA.length !== arrB.length) { return false; diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 030e83da08..52eeef937f 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -386,7 +386,7 @@ describe("Utils", () => { describe("arraysEqual", () => { describe("no compare function provided", () => { - it("should return true if the arrays are shallowly equal", () => { + describe("should return true if the arrays are shallowly equal", () => { runTest(true, undefined, undefined); runTest(true, null, null); runTest(true, [], []); @@ -394,9 +394,10 @@ describe("Utils", () => { runTest(true, [3, "1", true], [3, "1", true]); }); - it("should return false if the arrays are not shallowly equal", () => { + describe("should return false if the arrays are not shallowly equal", () => { runTest(false, undefined, null); runTest(false, null, []); + runTest(false, null, [3]); runTest(false, [3], []); runTest(false, [3, 1, 2], [3, 1]); runTest(false, [{}], [{}]); @@ -407,7 +408,7 @@ describe("Utils", () => { describe("compare function provided", () => { const COMPARE_FN = (a: any, b: any) => a.x === b.x; - it("should return true if the arrays are equal using a custom compare function", () => { + describe("should return true if the arrays are equal using a custom compare function", () => { runTest(true, undefined, undefined, COMPARE_FN); runTest(true, null, null, COMPARE_FN); runTest(true, [], [], COMPARE_FN); @@ -415,7 +416,7 @@ describe("Utils", () => { runTest(true, [{ x: 1 }, { x: 2 }], [{ x: 1 }, { x: 2 }], COMPARE_FN); }); - it("should return false if the arrays are not equal using custom compare function", () => { + describe("should return false if the arrays are not equal using custom compare function", () => { runTest(false, undefined, null); runTest(false, null, []); runTest(false, [{ x: 1 }, {}], [{ x: 1 }, { x: 2 }], COMPARE_FN); From 10b410e5d2f2ae2ec3fc1632954f2724af791a70 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 12:35:11 -0700 Subject: [PATCH 09/15] Fix bug with className comparison in GuideLayer.shouldComponentUpdate --- packages/table/src/layers/guides.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/table/src/layers/guides.tsx b/packages/table/src/layers/guides.tsx index 54de2919dd..d4c162e60b 100644 --- a/packages/table/src/layers/guides.tsx +++ b/packages/table/src/layers/guides.tsx @@ -27,7 +27,7 @@ export interface IGuideLayerProps extends IProps { export class GuideLayer extends React.Component { public shouldComponentUpdate(nextProps: IGuideLayerProps) { if (this.props.className !== nextProps.className) { - return false; + return true; } // shallow-comparing guide arrays leads to tons of unnecessary re-renders, so we check the // array contents explicitly. From f26744fc34c3feafe6c8bf4b026c04da2e794e07 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 12:47:42 -0700 Subject: [PATCH 10/15] Use bothUndefined and bothNull helpers in arraysEqual --- packages/table/src/common/utils.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index 5a6b4c1bb6..639bb8b9d5 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -349,9 +349,7 @@ export const Utils = { * will be compared using the custom `compare` function if one is provided. */ arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) { - if (arrA === undefined && arrB === undefined) { - return true; - } else if (arrA === null && arrB === null) { + if (bothUndefined(arrA, arrB) || bothNull(arrA, arrB)) { return true; } else if (arrA == null || arrB == null || arrA.length !== arrB.length) { return false; From 8c1ed0e4af941c27de5f75c1a22ad32210ca2af1 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 12:52:47 -0700 Subject: [PATCH 11/15] Make shallowCompareKeys return false for two arrays --- packages/table/src/common/utils.ts | 2 +- packages/table/test/utilsTests.ts | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index 639bb8b9d5..785afa7c95 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -367,7 +367,7 @@ function shallowCompareKeys(objA: any, objB: any, keys: string[]) { return true; } else if (objA == null || objB == null) { return false; - } else if (Array.isArray(objA) !== Array.isArray(objB)) { + } else if (Array.isArray(objA) || Array.isArray(objB)) { return false; } return keys diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 52eeef937f..8bc300588d 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -334,6 +334,7 @@ describe("Utils", () => { runTest(false, undefined, null, []); runTest(false, null, {}, []); runTest(false, {}, [], []); + runTest(false, [], [], []); }); function runTest(expectedResult: boolean, a: any, b: any, keys: string[]) { From f3019c32bb389ca285389454759982419b55a01e Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 13:13:36 -0700 Subject: [PATCH 12/15] Respond to Gilad CR feedback --- packages/table/src/common/utils.ts | 33 +++++++++++---------------- packages/table/src/layers/guides.tsx | 5 ++-- packages/table/src/layers/regions.tsx | 3 ++- packages/table/test/utilsTests.ts | 16 +++++++------ 4 files changed, 26 insertions(+), 31 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index 785afa7c95..eb9a722c5d 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -205,9 +205,10 @@ export const Utils = { */ shallowCompareKeys(objA: any, objB: any, keys?: string[]) { if (keys != null) { - return shallowCompareKeys(objA, objB, keys); + return _shallowCompareKeys(objA, objB, keys); } else { - if (bothUndefined(objA, objB) || bothNull(objA, objB)) { + // treat `null` and `undefined` as the same + if (objA == null && objB == null) { return true; } else if (objA == null || objB == null) { return false; @@ -216,8 +217,8 @@ export const Utils = { } const keysA = Object.keys(objA); const keysB = Object.keys(objB); - return shallowCompareKeys(objA, objB, keysA) - && shallowCompareKeys(objA, objB, keysB); + return _shallowCompareKeys(objA, objB, keysA) + && _shallowCompareKeys(objA, objB, keysB); } }, @@ -348,13 +349,14 @@ export const Utils = { * Returns true if the arrays are equal. Elements will be shallowly compared by default, or they * will be compared using the custom `compare` function if one is provided. */ - arraysEqual(arrA: any[], arrB: any[], compare?: (a: any, b: any) => boolean) { - if (bothUndefined(arrA, arrB) || bothNull(arrA, arrB)) { + arraysEqual(arrA: any[], arrB: any[], compare = (a: any, b: any) => a === b) { + // treat `null` and `undefined` as the same + if (arrA == null && arrB == null) { return true; } else if (arrA == null || arrB == null || arrA.length !== arrB.length) { return false; } else { - return arrA.every((a, i) => compare == null ? a === arrB[i] : compare(a, arrB[i])); + return arrA.every((a, i) => compare(a, arrB[i])); } }, }; @@ -362,23 +364,14 @@ export const Utils = { /** * Partial shallow comparison between objects using the given list of keys. */ -function shallowCompareKeys(objA: any, objB: any, keys: string[]) { - if (bothUndefined(objA, objB) || bothNull(objA, objB)) { +function _shallowCompareKeys(objA: any, objB: any, keys: string[]) { + if (objA == null && objB == null) { return true; } else if (objA == null || objB == null) { return false; } else if (Array.isArray(objA) || Array.isArray(objB)) { return false; + } else { + return keys.every((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key]); } - return keys - .map((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key]) - .every((isEqual) => isEqual); -} - -function bothUndefined(objA: any, objB: any) { - return objA === undefined && objB === undefined; -} - -function bothNull(objA: any, objB: any) { - return objA === null && objB === null; } diff --git a/packages/table/src/layers/guides.tsx b/packages/table/src/layers/guides.tsx index d4c162e60b..7ef10bb710 100644 --- a/packages/table/src/layers/guides.tsx +++ b/packages/table/src/layers/guides.tsx @@ -31,9 +31,8 @@ export class GuideLayer extends React.Component { } // shallow-comparing guide arrays leads to tons of unnecessary re-renders, so we check the // array contents explicitly. - const verticalGuidesEqual = Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides); - const horizontalGuidesEqual = Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); - return !verticalGuidesEqual || !horizontalGuidesEqual; + return !Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides) + || !Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); } public render() { diff --git a/packages/table/src/layers/regions.tsx b/packages/table/src/layers/regions.tsx index 59f0ef6df2..e5b06d2fee 100644 --- a/packages/table/src/layers/regions.tsx +++ b/packages/table/src/layers/regions.tsx @@ -21,7 +21,8 @@ export interface IRegionLayerProps extends IProps { regions?: IRegion[]; /** - * The array of CSS styles to apply to each region. + * The array of CSS styles to apply to each region. The ith style object in this array will be + * applied to the ith region in `regions`. */ regionStyles?: React.CSSProperties[]; } diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 8bc300588d..2fdd01fff3 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -303,7 +303,7 @@ describe("Utils", () => { } }); - describe("shallowCompareKeys", () => { + describe.only("shallowCompareKeys", () => { describe("with `keys` defined", () => { describe("returns true if only the specified values are shallowly equal", () => { runTest(true, { a: 1 }, { a: 1 }, ["a"]); @@ -323,6 +323,7 @@ describe("Utils", () => { describe("edge cases that return true", () => { runTest(true, undefined, undefined, []); + runTest(true, undefined, null, []); runTest(true, undefined, undefined, ["a"]); runTest(true, null, null, []); runTest(true, null, null, ["a"]); @@ -331,7 +332,7 @@ describe("Utils", () => { }); describe("edge cases that return false", () => { - runTest(false, undefined, null, []); + runTest(false, {}, undefined, []); runTest(false, null, {}, []); runTest(false, {}, [], []); runTest(false, [], [], []); @@ -350,10 +351,11 @@ describe("Utils", () => { runTest(true, {}, {}); runTest(true, null, null); runTest(true, undefined, undefined); + runTest(true, null, undefined); }); describe("returns false if values are not shallowly equal", () => { - runTest(false, undefined, null); + runTest(false, undefined, {}); runTest(false, null, {}); runTest(false, {}, []); runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }); @@ -390,14 +392,14 @@ describe("Utils", () => { describe("should return true if the arrays are shallowly equal", () => { runTest(true, undefined, undefined); runTest(true, null, null); + runTest(true, undefined, null); runTest(true, [], []); runTest(true, [3], [3]); runTest(true, [3, "1", true], [3, "1", true]); }); describe("should return false if the arrays are not shallowly equal", () => { - runTest(false, undefined, null); - runTest(false, null, []); + runTest(false, undefined, []); runTest(false, null, [3]); runTest(false, [3], []); runTest(false, [3, 1, 2], [3, 1]); @@ -412,14 +414,14 @@ describe("Utils", () => { describe("should return true if the arrays are equal using a custom compare function", () => { runTest(true, undefined, undefined, COMPARE_FN); runTest(true, null, null, COMPARE_FN); + runTest(true, undefined, null, COMPARE_FN); runTest(true, [], [], COMPARE_FN); runTest(true, [{}, {}], [{}, {}], COMPARE_FN); runTest(true, [{ x: 1 }, { x: 2 }], [{ x: 1 }, { x: 2 }], COMPARE_FN); }); describe("should return false if the arrays are not equal using custom compare function", () => { - runTest(false, undefined, null); - runTest(false, null, []); + runTest(false, null, [], COMPARE_FN); runTest(false, [{ x: 1 }, {}], [{ x: 1 }, { x: 2 }], COMPARE_FN); }); }); From f749f570f5f07aa5875261676624324229308bd4 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 13:48:17 -0700 Subject: [PATCH 13/15] Delete unnecessary tests --- packages/table/test/utilsTests.ts | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 2fdd01fff3..5f306aa7e2 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -306,9 +306,6 @@ describe("Utils", () => { describe.only("shallowCompareKeys", () => { describe("with `keys` defined", () => { describe("returns true if only the specified values are shallowly equal", () => { - runTest(true, { a: 1 }, { a: 1 }, ["a"]); - runTest(true, { a: 1, b: true }, { a: 1, b: false }, ["a"]); - runTest(true, { a: 1, b: true }, { a: 1, b: false }, []); runTest(true, { a: 1 }, { a: 1 }, ["a", "b", "c", "d"]); runTest(true, { a: 1, b: [1, 2, 3], c: "3" }, { b: [1, 2, 3], a: 1, c: "3" }, ["a", "c"]); runTest(true, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b"]); @@ -316,24 +313,18 @@ describe("Utils", () => { describe("returns false if any specified values are not shallowly equal", () => { runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }, ["a"]); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}, ["a", "b", "c"]); runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}, ["a", "b", "c"]); }); describe("edge cases that return true", () => { - runTest(true, undefined, undefined, []); runTest(true, undefined, null, []); runTest(true, undefined, undefined, ["a"]); - runTest(true, null, null, []); runTest(true, null, null, ["a"]); - runTest(true, {}, {}, []); runTest(true, {}, {}, ["a"]); }); describe("edge cases that return false", () => { runTest(false, {}, undefined, []); - runTest(false, null, {}, []); runTest(false, {}, [], []); runTest(false, [], [], []); }); @@ -348,8 +339,6 @@ describe("Utils", () => { describe("with `keys` not defined", () => { describe("returns true if values are shallowly equal", () => { runTest(true, { a: 1, b: "2", c: true}, { a: 1, b: "2", c: true}); - runTest(true, {}, {}); - runTest(true, null, null); runTest(true, undefined, undefined); runTest(true, null, undefined); }); @@ -358,17 +347,12 @@ describe("Utils", () => { runTest(false, undefined, {}); runTest(false, null, {}); runTest(false, {}, []); - runTest(false, { a: [1, "2", true] }, { a: [1, "2", true] }); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); - runTest(false, { a: 1, b: "2", c: {}}, { a: 1, b: "2", c: {}}); runTest(false, { a: 1, b: "2", c: { a: 1 }}, { a: 1, b: "2", c: { a: 1 }}); }); describe("returns false if keys are different", () => { runTest(false, {}, { a: 1 }); - runTest(false, { a: 1 }, {}); runTest(false, { a: 1, b: "2" }, { b: "2" }); - runTest(false, { b: "2" }, { a: 1, b: "2" }); runTest(false, { a: 1, b: "2", c: true}, { b: "2", c: true, d: 3 }); }); @@ -391,19 +375,13 @@ describe("Utils", () => { describe("no compare function provided", () => { describe("should return true if the arrays are shallowly equal", () => { runTest(true, undefined, undefined); - runTest(true, null, null); runTest(true, undefined, null); - runTest(true, [], []); - runTest(true, [3], [3]); runTest(true, [3, "1", true], [3, "1", true]); }); describe("should return false if the arrays are not shallowly equal", () => { - runTest(false, undefined, []); runTest(false, null, [3]); - runTest(false, [3], []); runTest(false, [3, 1, 2], [3, 1]); - runTest(false, [{}], [{}]); runTest(false, [{ x: 1 }], [{ x: 1 }]); }); }); @@ -413,10 +391,7 @@ describe("Utils", () => { describe("should return true if the arrays are equal using a custom compare function", () => { runTest(true, undefined, undefined, COMPARE_FN); - runTest(true, null, null, COMPARE_FN); runTest(true, undefined, null, COMPARE_FN); - runTest(true, [], [], COMPARE_FN); - runTest(true, [{}, {}], [{}, {}], COMPARE_FN); runTest(true, [{ x: 1 }, { x: 2 }], [{ x: 1 }, { x: 2 }], COMPARE_FN); }); From 9db6978da023cdf9ace9b720324682aead23b3f6 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 17 Apr 2017 15:24:47 -0700 Subject: [PATCH 14/15] Oops, undo describe.only --- packages/table/test/utilsTests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/table/test/utilsTests.ts b/packages/table/test/utilsTests.ts index 5f306aa7e2..1f87b3aa04 100644 --- a/packages/table/test/utilsTests.ts +++ b/packages/table/test/utilsTests.ts @@ -303,7 +303,7 @@ describe("Utils", () => { } }); - describe.only("shallowCompareKeys", () => { + describe("shallowCompareKeys", () => { describe("with `keys` defined", () => { describe("returns true if only the specified values are shallowly equal", () => { runTest(true, { a: 1 }, { a: 1 }, ["a", "b", "c", "d"]); From 3b7c1057700071412d840ddaca584094662eba30 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Tue, 18 Apr 2017 11:47:28 -0700 Subject: [PATCH 15/15] Reduce duplicate logic in shallowCompareKeys --- packages/table/src/common/utils.ts | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/packages/table/src/common/utils.ts b/packages/table/src/common/utils.ts index eb9a722c5d..77550ecb98 100644 --- a/packages/table/src/common/utils.ts +++ b/packages/table/src/common/utils.ts @@ -204,17 +204,16 @@ export const Utils = { * compared; otherwise, all keys will be compared. */ shallowCompareKeys(objA: any, objB: any, keys?: string[]) { - if (keys != null) { + // treat `null` and `undefined` as the same + if (objA == null && objB == null) { + return true; + } else if (objA == null || objB == null) { + return false; + } else if (Array.isArray(objA) || Array.isArray(objB)) { + return false; + } else if (keys != null) { return _shallowCompareKeys(objA, objB, keys); } else { - // treat `null` and `undefined` as the same - if (objA == null && objB == null) { - return true; - } else if (objA == null || objB == null) { - return false; - } else if (Array.isArray(objA) !== Array.isArray(objB)) { - return false; - } const keysA = Object.keys(objA); const keysB = Object.keys(objB); return _shallowCompareKeys(objA, objB, keysA) @@ -365,13 +364,8 @@ export const Utils = { * Partial shallow comparison between objects using the given list of keys. */ function _shallowCompareKeys(objA: any, objB: any, keys: string[]) { - if (objA == null && objB == null) { - return true; - } else if (objA == null || objB == null) { - return false; - } else if (Array.isArray(objA) || Array.isArray(objB)) { - return false; - } else { - return keys.every((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key]); - } + return keys.every((key) => { + return objA.hasOwnProperty(key) === objB.hasOwnProperty(key) + && objA[key] === objB[key]; + }); }