-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Changes from 11 commits
771bdd1
287c001
07dd7a6
7a46219
0d13d00
244366b
af8d0b2
6dffba3
10b410e
f26744f
8c1ed0e
f3019c3
f749f57
9db6978
3b7c105
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]) { | ||
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); | ||
} | ||
return true; | ||
}, | ||
|
||
/** | ||
|
@@ -333,4 +343,42 @@ 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) => boolean) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sweet |
||
if (bothUndefined(arrA, arrB) || bothNull(arrA, arrB)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
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])); | ||
} | ||
}, | ||
}; | ||
|
||
/** | ||
* 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keys.every((key) => objA.hasOwnProperty(key) === objB.hasOwnProperty(key) && objA[key] === objB[key]) please There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops, fixed this elsewhere but missed this one |
||
} | ||
|
||
function bothUndefined(objA: any, objB: any) { | ||
return objA === undefined && objB === undefined; | ||
} | ||
|
||
function bothNull(objA: any, objB: any) { | ||
return objA === null && objB === null; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
/** | ||
|
@@ -24,6 +25,17 @@ 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. | ||
const verticalGuidesEqual = Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides); | ||
const horizontalGuidesEqual = Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); | ||
return !verticalGuidesEqual || !horizontalGuidesEqual; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may be a minor optimization here to inline both vars in the return !Utils.arraysEqual(this.props.verticalGuides, nextProps.verticalGuides)
|| !Utils.arraysEqual(this.props.horizontalGuides, nextProps.horizontalGuides); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noice. That's my understanding too - did this same thing in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah i saw! sounds good |
||
} | ||
|
||
public render() { | ||
const { verticalGuides, horizontalGuides, className } = this.props; | ||
const verticals = (verticalGuides == null) ? undefined : verticalGuides.map(this.renderVerticalGuide); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 region. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how do we know which styles belong to which region? just by index? worth noting in docs here i think. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. LMK if the wording needs to be massaged though. |
||
*/ | ||
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>; | ||
} | ||
|
@@ -40,13 +53,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]} | ||
/> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -528,6 +528,11 @@ export class Regions { | |
return regionGroups; | ||
} | ||
|
||
public static regionsEqual(regionA: IRegion, regionB: IRegion) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's public now, so it has to move up. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this not an immediate infinite loop every time?
can we please rename the internal function? might be legit to use
_
prefix here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done