react-highcharts icon indicating copy to clipboard operation
react-highcharts copied to clipboard

Dont use this.refs.chart.getChart

Open madnight opened this issue 8 years ago • 12 comments

In the README.md you describe the update mechanism as follows:

 componentDidMount() {
    let chart = this.refs.chart.getChart();
    chart.series[0].addPoint({x: 10, y: 12});
  }

  render() {
    return <ReactHighcharts config={config} ref="chart"></ReactHighcharts>;
  }

this is not the react way it should be updated via the react state so for example:

componentDidMount() {
        this.setState(
          { series: this.series(),
            xAxis: { categories: this.categories() }
          }
        );
    }

    render() {
        return (<ReactHighcharts config={this.state}/>);
    }

while the highchart config is represented as component stat

madnight avatar Jan 03 '17 22:01 madnight

Hey, I was wondering, dont you think, if we update the config and the data using state, the whole graph would rerender?

Instead .update method has to be used to update the graph or .addPoint method to add a point.

What do you think?

harshmaur avatar Mar 05 '17 04:03 harshmaur

The whole Graph would rerender, but only in Reacts virtual DOM which is extremly fast. After that React creates a minimal diff from the virtual DOM and real DOM and only updates the minimal necessary changeset, because real DOM manipulations are indeed slow. So as you can see such performance issues are solved by a virtual DOM and same goes for Vue js or other virtual DOM frameworks.

madnight avatar Mar 05 '17 10:03 madnight

@madnight I have another situation that I can't just set the state again:

I want to call setVisible for some series I can't just set satet

ikhattab avatar Mar 14 '17 10:03 ikhattab

@ikhattab sure you can, since all dynamic functions on the highcharts graph change the internal highcharts state. In your case it would be this boolean http://api.highcharts.com/highstock/series%3Cline%3E.visible

No one can stop you from using the dynamic functions via this.refs.chart.getChart, but keep in mind that you are not using react in this case and do not gain any virtual dom advantages. It's like using react-highcharts without react.

madnight avatar Mar 14 '17 11:03 madnight

Hey @madnight thanks for your input, I would love to improve reactwayness of this lib :)

Unfortunately I couldn't find a way to make Highcharts return anything diffable we could feed to react.

if you look at the code, we're currently just render Highcharts on top of a div and not getting any dom diffing benefits at all, therefore docs are correct. https://github.com/kirjs/react-highcharts/blob/master/src/chartsFactory.jsx#L27

I would love to hear your thoughts about getting Highcharts and React work together in a proper way though.

kirjs avatar Mar 18 '17 16:03 kirjs

I have another issue related to this one.

I'm getting chart in callback:

<ReactHighcharts
   config={this.gaugeOptions()}
   callback={chart => {
     this.chart = chart
   }}
/>

and trying to destroy chart on componentWillUnmount:

componentWillUnmount = () => {
  this.chart.destroy()
}

In this case I receive an error in chart.destroy():

highcharts.js:265 Uncaught TypeError: Cannot read property 'removeAttribute' of undefined
    at a.Chart.destroy (highcharts.js:265)
    at Object.componentWillUnmount (ReactHighcharts.js:1)
    at ReactCompositeComponent.js:409
    at measureLifeCyclePerf (ReactCompositeComponent.js:75)
    at ReactCompositeComponentWrapper.unmountComponent (ReactCompositeComponent.js:408)
    at Object.unmountComponent (ReactReconciler.js:79)
    at Object.unmountChildren (ReactChildReconciler.js:146)
    at ReactDOMComponent.unmountChildren (ReactMultiChild.js:373)
    at ReactDOMComponent.unmountComponent (ReactDOMComponent.js:980)
    at Object.unmountComponent (ReactReconciler.js:79)

Do I really need to destroy chart in React way to save memory?

JustFly1984 avatar Mar 25 '17 07:03 JustFly1984

@JustFly1984 no, because this highchart lib does not use react. They are only loosley coupled (glued together) but not integrated. Im not familiar enough with highcharts, so i cannot tell how to feed something diffable to reacts render method, but manipulating the state of the chart config as shown in my first post does work and make it more "reactive" if thats a word.

madnight avatar Mar 25 '17 09:03 madnight

Does ReactHighcharts component destroys chart internally?

JustFly1984 avatar Mar 29 '17 06:03 JustFly1984

In my case, there is a lot of data coming frequently from socket.io to redux, and amount of datasets could vary. Currently, to reproduce the bug, I do this steps:

  1. Reload the page.

  2. render 2 datasets for 5 seconds with random values. result: 1.1 LineChart shows 2 lines. 1.1 Chart Labels: 2 Labels.

  3. stopped (cleared values in redux store).

  4. render 1 dataset for 10 seconds with random values result: 3.1 LineChart for 5 seconds shows 2 lines, from 5 to 10 seconds printing only 1st line, 2nd line is abrupt at 5 seconds. 3.2 Still showing a Chart Label for 2nd dataset.

The only way to get around I found, is creating a separate component for each dataset, which is real opposite of DRY!

JustFly1984 avatar Mar 29 '17 06:03 JustFly1984

Changing the config of the chart does not seem like a great solution... If you are hovering the chart during a rerender you will lose the line which shows where you are at on the chart. It's a pretty hacky solution. I've used addPoint and it works fine. I don't see why you wouldn't use it...

haydencarlson avatar Sep 28 '17 16:09 haydencarlson

@haydencarlson sorry but i couldn't disgree more, addPoint is the hack, modifing reacts state is the correct way, example: https://github.com/madnight/githut/blob/master/src/js/components/LangChart.js#L108

madnight avatar Sep 28 '17 18:09 madnight

I think someone is programming a proper (reactive) version of react highcharts: https://github.com/whawker/react-jsx-highcharts

So this issue might be closed and people could be referred to this project, if they want a reactive approach.

madnight avatar Mar 09 '18 12:03 madnight