trading-peter / chart-elements

Chart.js as Polymer Elements
https://robdodson.github.io/chart-elements
267 stars 70 forks source link

Async data loading can lead to race condition #47

Closed trading-peter closed 8 years ago

trading-peter commented 8 years ago

Demo of the issue Load and look at console. It should produce errors when you hover over the chart. Maybe need some tries to reproduce. https://jsbin.com/gequnosihe/1/edit?html,output

Explantation This is a tricky one. If data is set async it can actually happen that one sets an invalid object for the chart data. This leads to errors flooding the console when hovering over the chart or at least throws when chart.js tries to set up the labels. AFAIK what happens is that hasData is bypassed:

  1. Set valid chart data. Everything is fine. The element set's hasData to true and queues the async update callback.
  2. Immediately set data to an empty object. The internal async update callback hasn't run at this point.
  3. The update callback is eventually executed and sets the modified empty data object => error

The solution in my tests seems to be that we need to check hasData in the last async callback again to make sure.

this.async(function () {
  if (this.hasData) {
    this.chart = new Chart(this.ctx, {
      type: this.type,
      data: this.data,
      options: this.options
    });
  }
}, null, 0);

Can you guys confirm? /@robdodson /@Stormsys