From e82affb2c8d2ed36e31c9ddc6de3d4f4a9855c54 Mon Sep 17 00:00:00 2001 From: Diego Gorgazzi <44508750+DiegoGorgazzi@users.noreply.github.com> Date: Fri, 18 Oct 2019 14:21:03 +0200 Subject: [PATCH 1/4] Refactor componentWillReceiveProps --- packages/react-vis/src/make-vis-flexible.js | 10 ++++-- .../react-vis/src/plot/series/arc-series.js | 6 ++-- packages/react-vis/src/plot/xy-plot.js | 35 ++++++++++--------- packages/react-vis/src/treemap/index.js | 13 +++---- .../force-directed-graph.js | 10 +++--- .../responsive-vis/responsive-scatterplot.js | 19 +++++----- 6 files changed, 52 insertions(+), 41 deletions(-) diff --git a/packages/react-vis/src/make-vis-flexible.js b/packages/react-vis/src/make-vis-flexible.js index 9c875abe7..024b5dcc1 100644 --- a/packages/react-vis/src/make-vis-flexible.js +++ b/packages/react-vis/src/make-vis-flexible.js @@ -140,9 +140,13 @@ function makeFlexible(Component, isWidthFlexible, isHeightFlexible) { this.cancelSubscription = subscribeToDebouncedResize(this._onResize); } - // eslint-disable-next-line react/no-deprecated - componentWillReceiveProps() { - this._onResize(); + componentDidUpdate(prevProps, prevState, snapshot) { + if ( + this.state.width !== prevState.width || + this.state.height !== prevState.height + ) { + this._onResize(); + } } componentWillUnmount() { diff --git a/packages/react-vis/src/plot/series/arc-series.js b/packages/react-vis/src/plot/series/arc-series.js index 2b90c63ae..a9b10d17b 100644 --- a/packages/react-vis/src/plot/series/arc-series.js +++ b/packages/react-vis/src/plot/series/arc-series.js @@ -74,8 +74,10 @@ class ArcSeries extends AbstractSeries { this.state = {scaleProps}; } - componentWillReceiveProps(nextProps) { - this.setState({scaleProps: this._getAllScaleProps(nextProps)}); + componentDidUpdate(prevProps, prevState, snapshot) { + if (this.props !== prevProps) { + this.setState({scaleProps: this._getAllScaleProps(prevProps)}); + } } /** diff --git a/packages/react-vis/src/plot/xy-plot.js b/packages/react-vis/src/plot/xy-plot.js index ccb503c5a..a8eb60d20 100644 --- a/packages/react-vis/src/plot/xy-plot.js +++ b/packages/react-vis/src/plot/xy-plot.js @@ -144,23 +144,24 @@ class XYPlot extends React.Component { }; } - // eslint-disable-next-line react/no-deprecated - componentWillReceiveProps(nextProps) { - const children = getSeriesChildren(nextProps.children); - const nextData = getStackedData(children, nextProps.stackBy); - const {scaleMixins} = this.state; - const nextScaleMixins = this._getScaleMixins(nextData, nextProps); - if ( - !checkIfMixinsAreEqual( - nextScaleMixins, - scaleMixins, - nextProps.hasTreeStructure - ) - ) { - this.setState({ - scaleMixins: nextScaleMixins, - data: nextData - }); + componentDidUpdate(prevProps, prevState, snapshot) { + if (this.props !== prevProps) { + const children = getSeriesChildren(prevProps.children); + const nextData = getStackedData(children, prevProps.stackBy); + const {scaleMixins} = this.state; + const nextScaleMixins = this._getScaleMixins(nextData, prevProps); + if ( + !checkIfMixinsAreEqual( + nextScaleMixins, + scaleMixins, + prevProps.hasTreeStructure + ) + ) { + this.setState({ + scaleMixins: nextScaleMixins, + data: nextData + }); + } } } diff --git a/packages/react-vis/src/treemap/index.js b/packages/react-vis/src/treemap/index.js index 2badb12d4..8143d9f35 100644 --- a/packages/react-vis/src/treemap/index.js +++ b/packages/react-vis/src/treemap/index.js @@ -100,12 +100,13 @@ class Treemap extends React.Component { }; } - // eslint-disable-next-line react/no-deprecated - componentWillReceiveProps(props) { - this.setState({ - scales: _getScaleFns(props), - ...getInnerDimensions(props, props.margin) - }); + componentDidUpdate(prevProps, prevState, snapshot) { + if (this.props !== prevProps) { + this.setState({ + scales: _getScaleFns(props), + ...getInnerDimensions(props, props.margin) + }); + } } /** diff --git a/packages/showcase/examples/force-directed-graph/force-directed-graph.js b/packages/showcase/examples/force-directed-graph/force-directed-graph.js index 779357de0..06849396e 100644 --- a/packages/showcase/examples/force-directed-graph/force-directed-graph.js +++ b/packages/showcase/examples/force-directed-graph/force-directed-graph.js @@ -102,10 +102,12 @@ class ForceDirectedGraph extends React.Component { }; } - UNSAFE_componentWillReceiveProps(nextProps) { - this.setState({ - data: generateSimulation(nextProps) - }); + componentDidUpdate(prevProps, prevState, snapshot) { + if (this.props !== prevProps) { + this.setState({ + data: generateSimulation(nextProps) + }); + } } render() { diff --git a/packages/showcase/examples/responsive-vis/responsive-scatterplot.js b/packages/showcase/examples/responsive-vis/responsive-scatterplot.js index ac3b0f449..e75f68fed 100644 --- a/packages/showcase/examples/responsive-vis/responsive-scatterplot.js +++ b/packages/showcase/examples/responsive-vis/responsive-scatterplot.js @@ -74,15 +74,16 @@ export default class ResponsiveScatterplot extends React.Component { selectedPoints: [] }; - UNSAFE_componentWillReceiveProps(nextProps) { - // not the greatest - this.setState({ - binData: transformToBinData( - nextProps.data, - nextProps.width, - nextProps.height - ) - }); + componentDidUpdate(prevProps, prevState, snapshot) { + if (this.props !== prevProps) { + this.setState({ + binData: transformToBinData( + nextProps.data, + nextProps.width, + nextProps.height + ) + }); + } } getFeatures() { From 7b3271102cf9c4cbd59a79f07c97da03501e5757 Mon Sep 17 00:00:00 2001 From: Diego Gorgazzi <44508750+DiegoGorgazzi@users.noreply.github.com> Date: Fri, 18 Oct 2019 15:03:29 +0200 Subject: [PATCH 2/4] Fix props replacing componentWillReceiveProps --- packages/react-vis/src/plot/series/arc-series.js | 2 +- packages/react-vis/src/plot/xy-plot.js | 8 ++++---- packages/react-vis/src/treemap/index.js | 4 ++-- .../examples/force-directed-graph/force-directed-graph.js | 2 +- .../examples/responsive-vis/responsive-scatterplot.js | 6 +++--- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/react-vis/src/plot/series/arc-series.js b/packages/react-vis/src/plot/series/arc-series.js index a9b10d17b..90e8e4067 100644 --- a/packages/react-vis/src/plot/series/arc-series.js +++ b/packages/react-vis/src/plot/series/arc-series.js @@ -76,7 +76,7 @@ class ArcSeries extends AbstractSeries { componentDidUpdate(prevProps, prevState, snapshot) { if (this.props !== prevProps) { - this.setState({scaleProps: this._getAllScaleProps(prevProps)}); + this.setState({scaleProps: this._getAllScaleProps(this.props)}); } } diff --git a/packages/react-vis/src/plot/xy-plot.js b/packages/react-vis/src/plot/xy-plot.js index a8eb60d20..7515f3b3c 100644 --- a/packages/react-vis/src/plot/xy-plot.js +++ b/packages/react-vis/src/plot/xy-plot.js @@ -146,15 +146,15 @@ class XYPlot extends React.Component { componentDidUpdate(prevProps, prevState, snapshot) { if (this.props !== prevProps) { - const children = getSeriesChildren(prevProps.children); - const nextData = getStackedData(children, prevProps.stackBy); + const children = getSeriesChildren(this.props.children); + const nextData = getStackedData(children, this.props.stackBy); const {scaleMixins} = this.state; - const nextScaleMixins = this._getScaleMixins(nextData, prevProps); + const nextScaleMixins = this._getScaleMixins(nextData, this.props); if ( !checkIfMixinsAreEqual( nextScaleMixins, scaleMixins, - prevProps.hasTreeStructure + this.props.hasTreeStructure ) ) { this.setState({ diff --git a/packages/react-vis/src/treemap/index.js b/packages/react-vis/src/treemap/index.js index 8143d9f35..43408da29 100644 --- a/packages/react-vis/src/treemap/index.js +++ b/packages/react-vis/src/treemap/index.js @@ -103,8 +103,8 @@ class Treemap extends React.Component { componentDidUpdate(prevProps, prevState, snapshot) { if (this.props !== prevProps) { this.setState({ - scales: _getScaleFns(props), - ...getInnerDimensions(props, props.margin) + scales: _getScaleFns(this.props), + ...getInnerDimensions(this.props, this.props.margin) }); } } diff --git a/packages/showcase/examples/force-directed-graph/force-directed-graph.js b/packages/showcase/examples/force-directed-graph/force-directed-graph.js index 06849396e..b7ea18b7c 100644 --- a/packages/showcase/examples/force-directed-graph/force-directed-graph.js +++ b/packages/showcase/examples/force-directed-graph/force-directed-graph.js @@ -105,7 +105,7 @@ class ForceDirectedGraph extends React.Component { componentDidUpdate(prevProps, prevState, snapshot) { if (this.props !== prevProps) { this.setState({ - data: generateSimulation(nextProps) + data: generateSimulation(this.props) }); } } diff --git a/packages/showcase/examples/responsive-vis/responsive-scatterplot.js b/packages/showcase/examples/responsive-vis/responsive-scatterplot.js index e75f68fed..48abd799b 100644 --- a/packages/showcase/examples/responsive-vis/responsive-scatterplot.js +++ b/packages/showcase/examples/responsive-vis/responsive-scatterplot.js @@ -78,9 +78,9 @@ export default class ResponsiveScatterplot extends React.Component { if (this.props !== prevProps) { this.setState({ binData: transformToBinData( - nextProps.data, - nextProps.width, - nextProps.height + this.props.data, + this.props.width, + this.props.height ) }); } From d42a21eada6d9f720f911df45c313a212e7ea231 Mon Sep 17 00:00:00 2001 From: Chris Thomas Date: Sun, 31 May 2020 11:01:23 -0400 Subject: [PATCH 3/4] Fix Tests --- packages/react-vis/tests/components/area-series.test.js | 2 ++ packages/react-vis/tests/components/heatmap.test.js | 6 ++++-- packages/react-vis/tests/components/line-series.test.js | 2 ++ packages/react-vis/tests/components/radial.test.js | 2 ++ packages/react-vis/tests/components/sunburst.test.js | 1 + 5 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/react-vis/tests/components/area-series.test.js b/packages/react-vis/tests/components/area-series.test.js index 993d488e3..6829f507e 100644 --- a/packages/react-vis/tests/components/area-series.test.js +++ b/packages/react-vis/tests/components/area-series.test.js @@ -31,6 +31,8 @@ describe('AreaSeries', () => { expect($.find('path.area-chart-example').length).toBe(1); $.setProps({children: }); + $.update(); + expect($.find('.rv-xy-plot__series').length).toBe(0); expect($.find('.rv-xy-plot__series path').length).toBe(0); expect($.find('.area-chart-example').length).toBe(0); diff --git a/packages/react-vis/tests/components/heatmap.test.js b/packages/react-vis/tests/components/heatmap.test.js index 72109a0af..20542d129 100644 --- a/packages/react-vis/tests/components/heatmap.test.js +++ b/packages/react-vis/tests/components/heatmap.test.js @@ -30,7 +30,7 @@ describe('Heatmap', () => { test('basic rendering', () => { const $ = mount( - + ); expect($.find('.rv-xy-plot__series--heatmap').length).toBe(1); @@ -38,8 +38,10 @@ describe('Heatmap', () => { expect($.find('g.heatmap-series-example').length).toBe(1); $.setProps({ - children: + children: }); + $.update(); + expect($.find('.rv-xy-plot__series--heatmap').length).toBe(0); expect($.find('.rv-xy-plot__series--heatmap rect').length).toBe(0); expect($.find('.heatmap-series-example').length).toBe(0); diff --git a/packages/react-vis/tests/components/line-series.test.js b/packages/react-vis/tests/components/line-series.test.js index f4261dafe..8eb507fa2 100644 --- a/packages/react-vis/tests/components/line-series.test.js +++ b/packages/react-vis/tests/components/line-series.test.js @@ -57,6 +57,8 @@ describe('LineSeries', () => { expect($.find('path.line-chart-example').length).toBe(1); $.setProps({children: }); + $.update(); + expect($.find('.rv-xy-plot__series').length).toBe(0); expect($.find('.rv-xy-plot__series path').length).toBe(0); expect($.find('.line-chart-example').length).toBe(0); diff --git a/packages/react-vis/tests/components/radial.test.js b/packages/react-vis/tests/components/radial.test.js index a9caafc17..2d7d96155 100644 --- a/packages/react-vis/tests/components/radial.test.js +++ b/packages/react-vis/tests/components/radial.test.js @@ -43,6 +43,8 @@ describe('RadialChart', () => { expect($.text()).toBe('yellow againmagentacyanyellowgreen'); $.setProps({data: []}); + $.update(); + expect($.find('.rv-radial-chart__series--pie__slice').length).toBe(0); expect($.find('.rv-radial-chart__series--pie__slice-overlay').length).toBe( 0 diff --git a/packages/react-vis/tests/components/sunburst.test.js b/packages/react-vis/tests/components/sunburst.test.js index 0f6b27470..caaf53bc5 100644 --- a/packages/react-vis/tests/components/sunburst.test.js +++ b/packages/react-vis/tests/components/sunburst.test.js @@ -57,6 +57,7 @@ describe('Sunburst', () => { ).toBe(21); $.setProps({data: INTERPOLATE_DATA}); + $.update(); expect($.find('.rv-xy-plot__series--arc-path').length).toBe(9); }); From a94e834dc9ea57d98b7a93de20e7d998d480ad6b Mon Sep 17 00:00:00 2001 From: Chris Thomas Date: Sun, 31 May 2020 11:22:18 -0400 Subject: [PATCH 4/4] Fix Lint --- packages/react-vis/src/make-vis-flexible.js | 2 +- packages/react-vis/src/plot/series/arc-series.js | 2 +- packages/react-vis/src/plot/xy-plot.js | 2 +- packages/react-vis/src/treemap/index.js | 2 +- packages/react-vis/tests/components/heatmap.test.js | 6 ++++-- .../examples/force-directed-graph/force-directed-graph.js | 2 +- .../examples/responsive-vis/responsive-scatterplot.js | 2 +- 7 files changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/react-vis/src/make-vis-flexible.js b/packages/react-vis/src/make-vis-flexible.js index 024b5dcc1..071e7e7b2 100644 --- a/packages/react-vis/src/make-vis-flexible.js +++ b/packages/react-vis/src/make-vis-flexible.js @@ -140,7 +140,7 @@ function makeFlexible(Component, isWidthFlexible, isHeightFlexible) { this.cancelSubscription = subscribeToDebouncedResize(this._onResize); } - componentDidUpdate(prevProps, prevState, snapshot) { + componentDidUpdate(prevProps, prevState) { if ( this.state.width !== prevState.width || this.state.height !== prevState.height diff --git a/packages/react-vis/src/plot/series/arc-series.js b/packages/react-vis/src/plot/series/arc-series.js index 90e8e4067..be7f7a2b0 100644 --- a/packages/react-vis/src/plot/series/arc-series.js +++ b/packages/react-vis/src/plot/series/arc-series.js @@ -74,7 +74,7 @@ class ArcSeries extends AbstractSeries { this.state = {scaleProps}; } - componentDidUpdate(prevProps, prevState, snapshot) { + componentDidUpdate(prevProps) { if (this.props !== prevProps) { this.setState({scaleProps: this._getAllScaleProps(this.props)}); } diff --git a/packages/react-vis/src/plot/xy-plot.js b/packages/react-vis/src/plot/xy-plot.js index 7515f3b3c..1f7b563fb 100644 --- a/packages/react-vis/src/plot/xy-plot.js +++ b/packages/react-vis/src/plot/xy-plot.js @@ -144,7 +144,7 @@ class XYPlot extends React.Component { }; } - componentDidUpdate(prevProps, prevState, snapshot) { + componentDidUpdate(prevProps) { if (this.props !== prevProps) { const children = getSeriesChildren(this.props.children); const nextData = getStackedData(children, this.props.stackBy); diff --git a/packages/react-vis/src/treemap/index.js b/packages/react-vis/src/treemap/index.js index 43408da29..c9a466868 100644 --- a/packages/react-vis/src/treemap/index.js +++ b/packages/react-vis/src/treemap/index.js @@ -100,7 +100,7 @@ class Treemap extends React.Component { }; } - componentDidUpdate(prevProps, prevState, snapshot) { + componentDidUpdate(prevProps) { if (this.props !== prevProps) { this.setState({ scales: _getScaleFns(this.props), diff --git a/packages/react-vis/tests/components/heatmap.test.js b/packages/react-vis/tests/components/heatmap.test.js index 20542d129..6c990363f 100644 --- a/packages/react-vis/tests/components/heatmap.test.js +++ b/packages/react-vis/tests/components/heatmap.test.js @@ -30,7 +30,7 @@ describe('Heatmap', () => { test('basic rendering', () => { const $ = mount( - + ); expect($.find('.rv-xy-plot__series--heatmap').length).toBe(1); @@ -38,7 +38,9 @@ describe('Heatmap', () => { expect($.find('g.heatmap-series-example').length).toBe(1); $.setProps({ - children: + children: ( + + ) }); $.update(); diff --git a/packages/showcase/examples/force-directed-graph/force-directed-graph.js b/packages/showcase/examples/force-directed-graph/force-directed-graph.js index b7ea18b7c..a6c00f4b4 100644 --- a/packages/showcase/examples/force-directed-graph/force-directed-graph.js +++ b/packages/showcase/examples/force-directed-graph/force-directed-graph.js @@ -102,7 +102,7 @@ class ForceDirectedGraph extends React.Component { }; } - componentDidUpdate(prevProps, prevState, snapshot) { + componentDidUpdate(prevProps) { if (this.props !== prevProps) { this.setState({ data: generateSimulation(this.props) diff --git a/packages/showcase/examples/responsive-vis/responsive-scatterplot.js b/packages/showcase/examples/responsive-vis/responsive-scatterplot.js index 48abd799b..dce3b6982 100644 --- a/packages/showcase/examples/responsive-vis/responsive-scatterplot.js +++ b/packages/showcase/examples/responsive-vis/responsive-scatterplot.js @@ -74,7 +74,7 @@ export default class ResponsiveScatterplot extends React.Component { selectedPoints: [] }; - componentDidUpdate(prevProps, prevState, snapshot) { + componentDidUpdate(prevProps) { if (this.props !== prevProps) { this.setState({ binData: transformToBinData(