Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Table] Eliminate unnecessary re-renders everywhere #998

Merged
merged 15 commits into from
Apr 18, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/table/src/cell/formats/jsonFormat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -29,6 +30,7 @@ export interface IJSONFormatProps extends ITruncatedFormatProps {
stringify?: (obj: any) => string;
}

@PureRender
export class JSONFormat extends React.Component<IJSONFormatProps, {}> {
public static defaultProps: IJSONFormatProps = {
detectTruncation: true,
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/cell/formats/truncatedFormat.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -66,6 +67,7 @@ export interface ITruncatedFormatState {
isTruncated: boolean;
}

@PureRender
export class TruncatedFormat extends React.Component<ITruncatedFormatProps, ITruncatedFormatState> {
public static defaultProps: ITruncatedFormatProps = {
detectTruncation: true,
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/column.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -46,6 +47,7 @@ export interface IColumnProps extends IColumnNameProps, IProps {
renderColumnHeader?: IColumnHeaderRenderer;
}

@PureRender
export class Column extends React.Component<IColumnProps, {}> {
public static defaultProps: IColumnProps = {
renderCell: emptyCellRenderer,
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/common/contextMenuTargetWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -20,6 +21,7 @@ export interface IContextMenuTargetWrapper extends IProps {
* chains.
*/
@ContextMenuTarget
@PureRender
export class ContextMenuTargetWrapper extends React.Component<IContextMenuTargetWrapper, {}> {
public render() {
const { className, children, style } = this.props;
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/common/loadableContent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -24,6 +25,7 @@ export interface ILoadableContentProps {
}

// This class expects a single, non-string child.
@PureRender
export class LoadableContent extends React.Component<ILoadableContentProps, {}> {
private style: React.CSSProperties;

Expand Down
49 changes: 42 additions & 7 deletions packages/table/src/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -200,15 +200,25 @@ export const Utils = {
},

/**
* Partial shallow comparison between objects using the given list of keys.
* 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[]) {
for (const key of keys) {
if (objA[key] !== objB[key]) {
return false;
}
shallowCompareKeys(objA: any, objB: any, keys?: string[]) {
// 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 {
const keysA = Object.keys(objA);
const keysB = Object.keys(objB);
return _shallowCompareKeys(objA, objB, keysA)
&& _shallowCompareKeys(objA, objB, keysB);
}
return true;
},

/**
Expand Down Expand Up @@ -333,4 +343,29 @@ export const Utils = {
isLeftClick(event: MouseEvent) {
return event.button === 0;
},

/**
* 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) => 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(a, arrB[i]));
}
},
};

/**
* Partial shallow comparison between objects using the given list of keys.
*/
function _shallowCompareKeys(objA: any, objB: any, keys: string[]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any way you can simplify this to all just one function?

shallowCompareKeys: function shallowCompareKeys(objA: any, objB: any, keys?: string[]) {
    ...

    // recurse
    shallowCompareKeys(...);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would literally be the second time I've actually had to use recursion in the wild, ha. Let me dust off my knowledge and give it a try.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recursion is a tree's best friend

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for this kid. https://www.youtube.com/watch?v=1TZCP6OqRlE

@adidahiya ended up pulling the duplicate validation checks into one place and greatly simplifying the helper function. Didn't do the recursive approach though. This look okay now?

return keys.every((key) => {
return objA.hasOwnProperty(key) === objB.hasOwnProperty(key)
&& objA[key] === objB[key];
});
}
2 changes: 2 additions & 0 deletions packages/table/src/headers/editableName.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -37,6 +38,7 @@ export interface IEditableNameProps extends IIntentProps, IProps {
onConfirm?: (value: string) => void;
}

@PureRender
export class EditableName extends React.Component<IEditableNameProps, {}> {
public render() {
const { className, intent, name, onCancel, onChange, onConfirm } = this.props;
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/headers/rowHeaderCell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -67,6 +68,7 @@ export interface IRowHeaderState {
}

@ContextMenuTarget
@PureRender
export class RowHeaderCell extends React.Component<IRowHeaderCellProps, IRowHeaderState> {
public state = {
isActive: false,
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/interactions/menus/copyCellsMenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -37,6 +38,7 @@ export interface ICopyCellsMenuItemProps extends IMenuItemProps {
onCopy?: (success: boolean) => void;
}

@PureRender
export class CopyCellsMenuItem extends React.Component<ICopyCellsMenuItemProps, {}> {
public render() {
return <MenuItem {...this.props} onClick={this.handleClick} />;
Expand Down
2 changes: 2 additions & 0 deletions packages/table/src/interactions/resizeHandle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -57,6 +58,7 @@ export interface IResizeHandleState {
isDragging: boolean;
}

@PureRender
export class ResizeHandle extends React.Component<IResizeHandleProps, IResizeHandleState> {
public state: IResizeHandleState = {
isDragging: false,
Expand Down
11 changes: 11 additions & 0 deletions packages/table/src/layers/guides.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand All @@ -24,6 +25,16 @@ export interface IGuideLayerProps extends IProps {
}

export class GuideLayer extends React.Component<IGuideLayerProps, {}> {
public shouldComponentUpdate(nextProps: IGuideLayerProps) {
if (this.props.className !== nextProps.className) {
return true;
}
// shallow-comparing guide arrays leads to tons of unnecessary re-renders, so we check the
// array contents explicitly.
return !Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides)
|| !Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides);
}

public render() {
const { verticalGuides, horizontalGuides, className } = this.props;
const verticals = (verticalGuides == null) ? undefined : verticalGuides.map(this.renderVerticalGuide);
Expand Down
30 changes: 22 additions & 8 deletions packages/table/src/layers/regions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -21,13 +21,27 @@ 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 region. The ith style object in this array will be
* applied to the ith region in `regions`.
*/
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<IRegionLayerProps, {}> {
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

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 <div className={Classes.TABLE_OVERLAY_LAYER}>{this.renderRegionChildren()}</div>;
}
Expand All @@ -40,13 +54,13 @@ export class RegionLayer extends React.Component<IRegionLayerProps, {}> {
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 (
<div
className={classNames(Classes.TABLE_OVERLAY, Classes.TABLE_REGION, className)}
key={index}
style={getRegionStyle(region)}
style={regionStyles[index]}
/>
);
}
Expand Down
10 changes: 5 additions & 5 deletions packages/table/src/regions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,11 @@ export class Regions {
return regionGroups;
}

public static regionsEqual(regionA: IRegion, regionB: IRegion) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why move this? the previous location seems more logical to me: next to all the other comparison checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's public now, so it has to move up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhh public gotcha ok

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.
Expand Down Expand Up @@ -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);
Expand Down
Loading