fix: replace PureComponent with shouldComponentUpdate #534

patrykkopycinski · 2020-02-03T22:29:45Z

Summary

During working on elastic/kibana#56587 I have found out that charts-related components are probably re-renders unnecessarily, so I have decided to verify that and here is the outcome from https://github.com/welldone-software/why-did-you-render

Each line in the console means unnecessary component's re-render and after the row is expanded we can see which props are equal by value but not by reference, so it causes PureComponent to re-render.

After my changes

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
This was checked for cross-browser compatibility, including a check against IE11
Proper documentation or storybook story was added for features that require explanation or tutorials
Unit tests were updated or added to match the most common scenarios
Each commit follows the convention

codecov-io · 2020-02-03T22:53:44Z

Codecov Report

Merging #534 into master will increase coverage by 0.05%.
The diff coverage is 67.85%.

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   75.57%   75.63%   +0.05%     
==========================================
  Files         181      181              
  Lines        5651     5684      +33     
  Branches     1102     1102              
==========================================
+ Hits         4271     4299      +28     
- Misses       1364     1369       +5     
  Partials       16       16

Impacted Files	Coverage Δ
src/components/legend/legend_item.tsx	`100% <100%> (ø)`	⬆️
src/components/icons/icon.tsx	`100% <100%> (ø)`	⬆️
src/components/chart_container.tsx	`56.45% <100%> (+0.71%)`	⬆️
...types/xy_chart/renderer/canvas/rect_annotation.tsx	`42.85% <50%> (+6.49%)`	⬆️
src/chart_types/xy_chart/renderer/canvas/grid.tsx	`48.64% <50%> (+1.58%)`	⬆️
...hart_types/xy_chart/renderer/canvas/bar_values.tsx	`42.5% <50%> (+1.95%)`	⬆️
...types/xy_chart/renderer/canvas/line_annotation.tsx	`35.29% <50%> (+6.72%)`	⬆️
src/chart_types/xy_chart/renderer/canvas/axis.tsx	`21.6% <57.14%> (+1.6%)`	⬆️
..._types/xy_chart/renderer/canvas/bar_geometries.tsx	`33.33% <60%> (+7.61%)`	⬆️
src/chart_types/xy_chart/specs/line_annotation.tsx	`47.36% <60%> (+1.65%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 178afc9...e4fdb05. Read the comment docs.

markov00 · 2020-02-04T08:08:42Z

hi @patrykkopycinski thanks for this. I'm in the meantime of a refactoring that will move away from react for some of these components (the one used to render the chart).
I'd like to apply some of these fixes and some others I prefer to apply them on the redux-re-selectors chain, to avoid unnecessary recomputation that will avoid rerendering upstream.

patrykkopycinski · 2020-02-04T08:20:06Z

Sounds great @markov00! Do you know maybe when you will be able to merge your refactor? Because our app is heavily utilizing charts, so it has a big footprint on the performance

markov00 · 2020-02-04T08:39:52Z

Sounds great @markov00! Do you know maybe when you will be able to merge your refactor? Because our app is heavily utilizing charts, so it has a big footprint on the performance

I will push the PR today, it needs a review and then it will be merged

markov00

@patrykkopycinski let's merge this. I will merge mine changes later.
Thanks for the contribution

markov00 · 2020-02-04T08:47:05Z

src/chart_types/xy_chart/renderer/canvas/area_geometries.tsx

@@ -39,6 +40,11 @@ export class AreaGeometries extends React.PureComponent<AreaGeometriesDataProps,
      overPoint: undefined,
    };
  }
+
+  shouldComponentUpdate(nextProps: AreaGeometriesDataProps, nextState: AreaGeometriesDataState) {
+    return !deepEqual(this.props, nextProps) || !deepEqual(this.state, nextState);


you can avoid checking the equality of the state. This is not used anymore in this component and should be removed (there is no relevant use of the overPoint state object in the component)

markov00 · 2020-02-04T08:47:52Z

src/chart_types/xy_chart/renderer/canvas/bar_geometries.tsx

@@ -31,6 +32,11 @@ export class BarGeometries extends React.PureComponent<BarGeometriesDataProps, B
      overBar: undefined,
    };
  }
+
+  shouldComponentUpdate(nextProps: BarGeometriesDataProps, nextState: BarGeometriesDataState) {
+    return !deepEqual(this.props, nextProps) || !deepEqual(this.state, nextState);


same here, overBar is no more used (no one is setting it), you can remove the deepEqual for the state

markov00 · 2020-02-04T08:48:04Z

src/chart_types/xy_chart/renderer/canvas/line_geometries.tsx

@@ -37,6 +38,10 @@ export class LineGeometries extends React.PureComponent<LineGeometriesDataProps,
    };
  }

+  shouldComponentUpdate(nextProps: LineGeometriesDataProps, nextState: LineGeometriesDataState) {
+    return !deepEqual(this.props, nextProps) || !deepEqual(this.state, nextState);


robgil · 2020-02-05T10:10:22Z

Codecov Report

Merging #534 into master will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   75.57%   75.80%   +0.22%     
==========================================
  Files         181      193      +12     
  Lines        5651     5844     +193     
  Branches     1102     1116      +14     
==========================================
+ Hits         4271     4430     +159     
- Misses       1364     1397      +33     
- Partials       16       17       +1

Impacted Files	Coverage Δ
src/mocks/series/index.ts	`100.00% <0.00%> (ø)`
src/mocks/scale/scale.ts	`87.50% <0.00%> (ø)`
src/mocks/series/utils.ts	`88.88% <0.00%> (ø)`
src/mocks/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/specs.ts	`77.27% <0.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/series/seriesIdentifiers.ts	`93.33% <0.00%> (ø)`
src/mocks/hierarchical/palettes.ts	`100.00% <0.00%> (ø)`
src/mocks/scale/index.ts	`100.00% <0.00%> (ø)`
src/mocks/specs/index.ts	`100.00% <0.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 178afc9...e4fdb05. Read the comment docs.

codecov-io · 2020-02-05T10:20:11Z

Codecov Report

Merging #534 into master will increase coverage by 0.22%.
The diff coverage is 69.09%.

@@            Coverage Diff            @@
##           master    #534      +/-   ##
=========================================
+ Coverage   75.57%   75.8%   +0.22%     
=========================================
  Files         181     193      +12     
  Lines        5651    5844     +193     
  Branches     1102    1116      +14     
=========================================
+ Hits         4271    4430     +159     
- Misses       1364    1397      +33     
- Partials       16      17       +1

Impacted Files	Coverage Δ
src/components/legend/legend_item.tsx	`100% <100%> (ø)`	⬆️
src/components/icons/icon.tsx	`100% <100%> (ø)`	⬆️
src/components/chart_container.tsx	`56.45% <100%> (+0.71%)`	⬆️
...types/xy_chart/renderer/canvas/rect_annotation.tsx	`42.85% <50%> (+6.49%)`	⬆️
src/chart_types/xy_chart/renderer/canvas/grid.tsx	`48.64% <50%> (+1.58%)`	⬆️
...hart_types/xy_chart/renderer/canvas/bar_values.tsx	`42.5% <50%> (+1.95%)`	⬆️
...types/xy_chart/renderer/canvas/line_annotation.tsx	`35.29% <50%> (+6.72%)`	⬆️
src/chart_types/xy_chart/renderer/canvas/axis.tsx	`21.6% <57.14%> (+1.6%)`	⬆️
src/chart_types/xy_chart/specs/line_annotation.tsx	`47.36% <60%> (+1.65%)`	⬆️
..._types/xy_chart/renderer/canvas/bar_geometries.tsx	`28.94% <75%> (+3.23%)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 178afc9...e4fdb05. Read the comment docs.

patrykkopycinski · 2020-02-05T10:35:26Z

Thank you very much for help @markov00 💪

## [17.0.1](v17.0.0...v17.0.1) (2020-02-05) ### Bug Fixes * replace PureComponent with shouldComponentUpdate ([#534](#534)) ([5043725](5043725))

markov00 · 2020-02-05T10:42:26Z

🎉 This PR is included in version 17.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

This commit replaces React.PureComponent with React.Component with custom shouldComponentUpdate that deeply compares props. Also, it fixes shouldComponentUpdate logic in ChartContainerComponent that caused the component to re-render every time the props have changed.

This commit replaces React.PureComponent with React.Component with custom shouldComponentUpdate that deeply compares props. Also, it fixes shouldComponentUpdate logic in ChartContainerComponent that caused the component to re-render every time the props have changed. Co-authored-by: patrykkopycinski <contact@patrykkopycinski.com>

## [16.1.1](v16.1.0...v16.1.1) (2020-02-05) ### Bug Fixes * replace PureComponent with shouldComponentUpdate ([#534](#534)) ([#535](#535)) ([f05f8cb](f05f8cb))

## [17.0.1](elastic/elastic-charts@v17.0.0...v17.0.1) (2020-02-05) ### Bug Fixes * replace PureComponent with shouldComponentUpdate ([opensearch-project#534](elastic/elastic-charts#534)) ([e06aeb2](elastic/elastic-charts@e06aeb2))

patrykkopycinski added 2 commits February 3, 2020 23:15

Replace PureComponent with shouldComponentUpdate

de97ef7

package.json

bb4a38f

patrykkopycinski requested a review from markov00 February 3, 2020 22:29

patrykkopycinski self-assigned this Feb 3, 2020

patrykkopycinski changed the title ~~Chore/replace purecomponent with shouldcomponentupdate~~ perf : replace purecomponent with shouldcomponentupdate Feb 3, 2020

patrykkopycinski changed the title ~~perf : replace purecomponent with shouldcomponentupdate~~ perf: replace purecomponent with shouldcomponentupdate Feb 3, 2020

patrykkopycinski changed the title ~~perf: replace purecomponent with shouldcomponentupdate~~ perf: replace PureComponent with shouldComponentUpdate Feb 3, 2020

patrykkopycinski requested a review from nickofthyme February 3, 2020 22:34

markov00 approved these changes Feb 5, 2020

View reviewed changes

patrykkopycinski added 2 commits February 5, 2020 11:01

PR comments

bd58646

revert

e4fdb05

patrykkopycinski changed the title ~~perf: replace PureComponent with shouldComponentUpdate~~ fix: replace PureComponent with shouldComponentUpdate Feb 5, 2020

patrykkopycinski merged commit 5043725 into elastic:master Feb 5, 2020

patrykkopycinski deleted the chore/replace-purecomponent-with-shouldcomponentupdate branch February 5, 2020 10:34

markov00 pushed a commit that referenced this pull request Feb 5, 2020

chore(release): 17.0.1 [skip ci]

cc561a4

## [17.0.1](v17.0.0...v17.0.1) (2020-02-05) ### Bug Fixes * replace PureComponent with shouldComponentUpdate ([#534](#534)) ([5043725](5043725))

markov00 added the released Issue released publicly label Feb 5, 2020

nickofthyme mentioned this pull request Feb 5, 2020

fix: replace PureComponent with shouldComponentUpdate (#534) #535

Merged

markov00 pushed a commit that referenced this pull request Feb 5, 2020

chore(release): 16.1.1 [skip ci]

d5242c7

## [16.1.1](v16.1.0...v16.1.1) (2020-02-05) ### Bug Fixes * replace PureComponent with shouldComponentUpdate ([#534](#534)) ([#535](#535)) ([f05f8cb](f05f8cb))

markov00 mentioned this pull request Feb 10, 2020

fix(ie11): replace fast-deep-equal with an internal copy IE11 safe #542

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: replace PureComponent with shouldComponentUpdate #534

fix: replace PureComponent with shouldComponentUpdate #534

patrykkopycinski commented Feb 3, 2020 •

edited

Loading

codecov-io commented Feb 3, 2020 •

edited

Loading

markov00 commented Feb 4, 2020

patrykkopycinski commented Feb 4, 2020

markov00 commented Feb 4, 2020

markov00 left a comment

markov00 Feb 4, 2020

markov00 Feb 4, 2020

markov00 Feb 4, 2020

robgil commented Feb 5, 2020 •

edited

Loading

codecov-io commented Feb 5, 2020 •

edited

Loading

patrykkopycinski commented Feb 5, 2020

markov00 commented Feb 5, 2020

fix: replace PureComponent with shouldComponentUpdate #534

fix: replace PureComponent with shouldComponentUpdate #534

Conversation

patrykkopycinski commented Feb 3, 2020 • edited Loading

Summary

Checklist

codecov-io commented Feb 3, 2020 • edited Loading

Codecov Report

markov00 commented Feb 4, 2020

patrykkopycinski commented Feb 4, 2020

markov00 commented Feb 4, 2020

markov00 left a comment

Choose a reason for hiding this comment

markov00 Feb 4, 2020

Choose a reason for hiding this comment

markov00 Feb 4, 2020

Choose a reason for hiding this comment

markov00 Feb 4, 2020

Choose a reason for hiding this comment

robgil commented Feb 5, 2020 • edited Loading

Codecov Report

codecov-io commented Feb 5, 2020 • edited Loading

Codecov Report

patrykkopycinski commented Feb 5, 2020

markov00 commented Feb 5, 2020

patrykkopycinski commented Feb 3, 2020 •

edited

Loading

codecov-io commented Feb 3, 2020 •

edited

Loading

robgil commented Feb 5, 2020 •

edited

Loading

codecov-io commented Feb 5, 2020 •

edited

Loading