[Table] Add @PureRender everywhere and eliminate excessive RegionLayer re-renders #988
Conversation
…nd shallowCompareKeys" This reverts commit 771bdd1.
Revert "Add Utils.shallowCompare plus unit tests for shallowCompare and shallowCompareKeys"Preview: documentation | table |
Oops, fix test descriptionPreview: documentation | table |
| export class RegionLayer extends React.Component<IRegionLayerProps, {}> { | ||
| public shouldComponentUpdate(nextProps: IRegionLayerProps) { | ||
| return !Utils.shallowCompareKeys(this.props, nextProps, UPDATE_PROPS_KEYS) | ||
| || !Regions.regionArraysEqual(this.props.regions, nextProps.regions); |
There was a problem hiding this comment.
is array equality not sufficient here such that we have to do this deep compare?
There was a problem hiding this comment.
Nope, see Regions.joinStyledRegionGroups, which creates a new regions array every time props are passed into RegionLayer.
| @@ -528,6 +528,20 @@ export class Regions { | |||
| return regionGroups; | |||
| } | |||
|
|
|||
| public static regionArraysEqual(regionsA: IRegion[], regionsB: IRegion[]) { | |||
There was a problem hiding this comment.
i might call this DeepEqual to clarify that it's much more than a === check.
| public static regionArraysEqual(regionsA: IRegion[], regionsB: IRegion[]) { | ||
| if (regionsA === undefined && regionsB === undefined) { | ||
| return true; | ||
| } else if (regionsA === null && regionsB === null) { |
There was a problem hiding this comment.
do you have a good reason for differentiating btw null and undefined here? given deep equality, those two seem the same to me.
null == undefined
There was a problem hiding this comment.
No great reason aside from the desire to use === everywhere for consistency. Strong aversion to keeping as is? I'm ambivalent.
| } else { | ||
| return regionsA | ||
| .map((regionA, i) => Regions.regionsEqual(regionA, regionsB[i])) | ||
| .every((isEqual) => isEqual); |
There was a problem hiding this comment.
please just replace map with every. all Array iterators receive the same three args. no need to create a new array of booleans.
There was a problem hiding this comment.
Yep, realized this after the fact haha.
| style.right = "0px"; | ||
| style.bottom = "0px"; | ||
| style.top = "0px"; | ||
| style.left = "0px"; |
There was a problem hiding this comment.
should be safe (and preferred, by stylelint at least) to use number 0 directly. style.right = 0;
There was a problem hiding this comment.
Agree, though I'd prefer to wait until a future PR just to minimize sneaky changes in this refactor.
| } | ||
| } | ||
|
|
||
| private styleRowHeaderRegion = (region: IRegion): React.CSSProperties => { |
There was a problem hiding this comment.
i wonder how much work it would be to make all these functions static? they could even live in a separate file.
you'd have to pull the this.grid and this.state.viewportRect logic out. maybe even accept an optional object of base styles to modify?
There was a problem hiding this comment.
Ooh, interesting. Would love to try, but okay if I wait until a future PR?
| } | ||
| }; | ||
| return this.maybeRenderRegions(styler); | ||
| return this.maybeRenderRegions(this.styleColumnHeaderRegion); | ||
| } | ||
|
|
||
| private maybeRenderRowHeaderRegions() { |
There was a problem hiding this comment.
each of these maybeRender* functions above are used exactly once, so consider just inlining them.
There was a problem hiding this comment.
Pulled these out for the perf benefit: before, the constant recreation of new inline functions (const styleFn = ... was causing === checks to fail when comparing function instances in RegionLayer.shouldComponentUpdate, triggering tons of additional re-renders.
My latest work work passes the computed region styles directly into RegionLayer, so we no longer care about ===-ing these function instances within RegionLayer (the function isn't even passed in anymore). But we still need to pass styler functions into maybeRenderRegions, and I'd rather refer to a single, unchanging function instance than constantly recreate styler functions like we used to do. Helps reduce activity on each render.
|
Some of the bugs that emerged in this PR were pretty nefarious (big one: region layers not updating when you resize columns), to the point that I made a second attempt at this in a separate branch. Closing this PR in favor of a new one that I'll open momentarily. |
Checklist
Changes proposed in this pull request:
PureRenderwherever it was missing. Nothing huge to be gained from here, probably.RegionLayerwas re-rendering A TON before. In particular, it would re-render on everymousemovewhen re-ordering, because the unchangingRegionLayer.props.getRegionStylefunctions were being recreated inline intable.tsxevery cycle. I moved those styler functions out into the<Table>component body. Now the only re-render happens when reordering is complete and the selection needs to move. (That's hundreds of renders down to ONE!)Reviewers should focus on:
@PureRendereverywhere I added it?shouldComponentUpdatelogic inRegionLayer. I had to omitprops.regionsfrom theshallowComparecheck and add a new function inRegionsto explicitly checkregionArraysEqual.@PureRenderwasn't sufficient here.