grofit / aurelia-chart

A chart element for aurelia which is powered by chart js using html5 canvas.
MIT License
46 stars 25 forks source link

Chart binding issue #7

Closed hopkins700 closed 8 years ago

hopkins700 commented 8 years ago

I have a chart which is bound to 'data' property as it is shown in the sample:

<canvas chart="type: Bar; data.bind:chartData; throttle: 100; should-update:true" style="width:100%; height: 100%;"></canvas>

"data" property returns data which has the following format

return { labels:[...], datasets:[{ fillColor:'#ee5315', data:[...] }]};

For some reason the data binding works fine only first two times and then the chart stops working and shows the old data. Any ideas why could it happen? Some other elements on the page are bound to the same "data" property so I'm sure that there are no issues on my side.

grofit commented 8 years ago

Not a clue off top of my head, if you can isolate a reproduction scenario on cloud 9 or somewhere then I can attempt to take a look.

grofit commented 8 years ago

I am not sure but given your code and a conversation I just had maybe it is related:

https://github.com/grofit/aurelia-chart/issues/6

If you are using promises and assigning those to variables hoping to eventually get the end value out, its probably not the way its meant to be used.

hopkins700 commented 8 years ago

I hope I'll publish the demo version in a few days or so and will be able to send a preview link with this issue. Anyway the chart widget logic is quite simple. Here is the source code:

export class JqChartContent extends WidgetContent { constructor(widget) { super(widget); }

get chartData(){ return this._chartData; } set chartData(value){ this._chartData = value; }

refresh(){ let query = new Query(); query.serverSideFilter = this.widget.dataFilter; this.widget.dataSource.getData(query).then(dH=>{ this.chartData = this.mapData(dH.data,this.widget.categoriesField); }); }

mapData(data, categoryField){ let lbl = [], d = []; .forOwn(.groupBy(data,categoryField), (v, k)=> { lbl.push(k); d.push(v.length); }); return { labels:lbl, datasets:[{ fillColor:'#ee5315', data:d }]}; }

}

grofit commented 8 years ago

I think the problem is that you are replacing the whole dataset each time, I know there is some magic available where aurelia notifies you of properties changing which are bindable so this may be fixable by just listening for them and resetting the chart, but trying to track down the docs on this stuff is a nightmare.

grofit commented 8 years ago

right after having a chat with Rob it may be possible to fix this scenario by adding a property change check to the code, so I will give that a try in isolation and see if that works later.

propertyChanged(propertyName, newValue, oldValue) { ... }

grofit commented 8 years ago

This should hopefully be fixed in 0.1.1, give that a try and let me know if it works for you.

hopkins700 commented 8 years ago

thanks for the quick update! but it still doesn't work for me.. after the first two updates the chart disappears at all.. :(

tom1980west commented 8 years ago

Definite improvement, the chart now binds on the completion of the promise. If I update the model, the chart does not refresh however - though it maybe something I'm doing wrong.

grofit commented 8 years ago

If you can create a repro I can take a look, the example has been updated to show updating of the model and resetting it and it should all work as expected, so I would assume it is something you are doing in your code, but again happy to take a look if you isolate your stuff in cloud 9 or whatever.

hopkins700 commented 8 years ago

While I'm trying to setup a demo on cloud9 could you please have a look on the source code? https://github.com/privosoft/periscope/blob/datatables.net_grid/src/layout/widgets/jq-chart-content.js https://github.com/privosoft/periscope/blob/datatables.net_grid/src/layout/widgets/jq-chart-content.html

grofit commented 8 years ago

Your source seems ok from what I can tell, I have not used lodash before (used similar libs) but I assume that it is synchronous in execution.

As it seems your getData call should refresh the underlying data bound to the view, so there is nothing that jumps out to me as being wrong. if you do a console.log and output the data after each mapData call does it always output correct data? (I assume it does but it seems odd that it works for one refresh but not the next).

grofit commented 8 years ago

I tried doing some quick attempts to do similar stuff to you locally (i.e use getter/setters for data, completely replace model) and I couldn't get it to freeze up. Although I have noticed that when resetting it seems to trigger the throttle so you get 2 updates when you only really need one, but other than that worked fine.

Only thing I can think of is that the data you are adding is somehow getting corrupted or incorrect in some way so each time you keep adding the data back in it is somehow nesting or something which causes it to blow up, but if you do a quick console.log that should work fine.

One other thing you can try, just try making your mapData method just output a fixed object, and see if that works, so it will just keep resetting to a default dataset which you know works, i.e:

mapData(data, categoryField)
{
    // commented out other stuff
    return {
        labels: ["One", "Two"],
        datasets: [{ data: [65, 59] }]
    };
}

If that works and refreshes correctly then it must be something to do with how you are mapping the data.

hopkins700 commented 8 years ago

With fixed data it works fine. The chart disappears if I change the dataset data (or create a new one.. whatever)

grofit commented 8 years ago

it would appear there is something wrong with the data you get out from your mapping then, if you do a console.log with the data you are getting out of it, or at least the data you are returning and see if it is how you expect, then if it isn't it will probably just refactoring your code, if it is how you expect but is still not working then I would probably need an isolated repro to assist any further so I could try things within your specific scenario.

grofit commented 8 years ago

hey so I think I may have some idea what your problem is after looking over your stuff again, depending upon what sort of charts you are using, this plugin "currently" uses the 1.x syntax for chart js not the 2.x syntax.

There is some work happening to update to the version 2 syntax which can be seen in the chart-js-2 branch (mentioned in #10), so you can either confirm you are using the 1.x style syntax for your charts, or you can try the version 2 branch, or alternatively just wait until the branch is promoted to master and see if that helps.

grofit commented 8 years ago

Master now contains the latest version of chartjs, so if you can try your code again with the latest version (0.2.0) then if it works close the issue if not we can discuss further.

sevenshadow commented 8 years ago

Hi,

I tried this last evening. My chart doesn't update when I try to send it new data. I can continue to fool around with it, but if you have any ideas please let me know.

Thanks.

grofit commented 8 years ago

not sure off top of my head, I know sharing the data between 2 charts does not work currently and I posed question to aurelia chaps with no response yet. If it is not that scenario then I would recommend trying to isolate the use case in any of the various online webby things (I use cloud 9) and send me a link to it misbehaving so I can try and diagnose it fully.

I do expect there to be some quirks with the 2.0 update of Chartjs but I use the examples page as the test bed, if that works then I just assume most other stuff works.

gregdouglas commented 8 years ago

I had a similar issue - when using more than one pie chart on a page - even when having two different sources of data - strangeness with pie charts not being renders or displaying old data. It was only having issues after updating the data after the initial display.

I will see if I can demo something up over the weekend.

grofit commented 8 years ago

ok, I know its a pain but at this point its difficult to replicate on my side without knowing exactly what you are doing. So if you get time to isolate brilliant, if not then at least if you can narrow down the pertinent points that trigger the issue we can try to take another look.

sevenshadow commented 8 years ago

Thanks. I will have time to take a look at this tomorrow (Sunday my time) to see if I can get some more clarity. I am only trying to render one chart. The issue seems to be with the update of the data. It is strange because it was working fine with the second to last build.

My issue is that the data is coming from a promise, so it needs to bind later (unless you know how I could just wait to bind the whole chart later. I don't need to update the chart. I just need to bind it on the resolution of a promise.

Thanks again for any support on this.

grofit commented 8 years ago

it shouldnt matter if the data comes from a promise or a sync function or getter etc, as long as you are only assigning values to the model you should be fine, i.e:

Bad, dont do this

myChartData = somethingThatReturnsAPromise()
.then(someOtherPromise)
.then(function(newData) { return newData; });

Do this

somethingThatReturnsAPromise()
.then(someOtherPromise)
.then(function(newData) { myChartData = newData; });

That way you are not going to have a promise as the variable for a period which will probably wreak havoc with the watchers and everything else in aurelia (not sure if they internally support promises as values). So if you just make sure you assign your data when your promise is ended so at no point is your data being assigned the promise object, it should just work.

Another approach which I tend to do to make it a bit nicer to read and re-useable is to have a method like:

updateChartData = (newData) => {
   this.chartData = newData;
}

This way you can do:

somethingThatReturnsAPromise()
.then(someOtherPromise)
.then(this.updateChartData);

// or manually
var newChartData = [...];
this.updateChartData(newChartData);

So you keep succinct promise chains but also only assign chart values to the chart data.

sevenshadow commented 8 years ago

Thanks again for the help.

I will try to set up a place to replicate. In the meantime here is my code:

I am using typescript:

On the view I have:

<canvas chart="type: line; data.bind: chartData; should-update: true; throttle: 1000; native-options.bind: chartOptions; "></canvas>

In my code .ts file I have:

import {inject, bindable} from 'aurelia-framework';
import {DataService} from '../../services/data-service';
import {CompanyIndex} from './company-index';

@inject(DataService, CompanyIndex)
export class Profile {

  heading = 'Company Profile';
  companyDetails;
  _dataService;
  companyIndex;
  showChart: boolean = false;
  daysToReturn: number = 2;

  quoteDates = ["Here"];
  closingPrices = [0.12];

  chartData = {
    labels: this.quoteDates,
    datasets: [
      {
        label: 'This is a test 1',
        pointBackgroundColor: "#008FD5",
        pointBorderColor: "#008FD5",
        borderColor: "#008FD5",
        data: this.closingPrices
      }
    ]
  };

  chartOptions = {
    legend: { display: true }
  };

  constructor(dataService, companyIndex) {
    this._dataService = dataService;
    this.companyIndex = companyIndex;

  }

  activate() {
    this.companyIndex.activeNav = 'profile';
    this._dataService.getCompanyDetails(this.companyIndex.company.id).then(companyDetails => {

      this.companyDetails = JSON.parse(companyDetails).companies[0];
      this._dataService.getCompanyQuotes(this.companyIndex.company.ticker, this.daysToReturn).then(quotes => {

        this.closingPrices = quotes.markets[0].prices.map(function (a) { return a.close; });
        this.quoteDates = quotes.markets[0].prices.map(function (a) { return a.date.toString(); });
        console.log(this.quoteDates);
        console.log(this.closingPrices);

        this.chartData = {
          labels: this.quoteDates,
          datasets: [
            {
              label: 'This is a test 2',
              pointBackgroundColor: "#008FD5",
              pointBorderColor: "#008FD5",
              borderColor: "#008FD5",
              data: this.closingPrices
            }
          ]
        }

        this.showChart = true;
      });

    })
  }

}

In the console, I get the following output for those console.logs

["2016-05-06", "2016-05-05"]
[50.39, 49.94] 
sevenshadow commented 8 years ago

FYI, I did get it to bind like this, but this seems to bypass your control.

     <canvas id="my-chart"></canvas>
import {inject, bindable} from 'aurelia-framework';
import {DataService} from '../../services/data-service';
import {CompanyIndex} from './company-index';
import $ from 'jquery';
import * from 'chartjs';

@inject(DataService, CompanyIndex)
export class Profile {

  heading = 'Company Profile';
  companyDetails;
  _dataService;
  companyIndex;
  showChart: boolean = false;
  daysToReturn: number = 20;

  quoteDates; 
  closingPrices;
  chartData;

  chartOptions = {
    legend: { display: false }
  };

  constructor(dataService, companyIndex) {
    this._dataService = dataService;
    this.companyIndex = companyIndex;

  }

  activate() {
    this.companyIndex.activeNav = 'profile';
    this._dataService.getCompanyDetails(this.companyIndex.company.id).then(companyDetails => {

      this.companyDetails = JSON.parse(companyDetails).companies[0];
      this._dataService.getCompanyQuotes(this.companyIndex.company.ticker, this.daysToReturn).then(quotes => {

        this.closingPrices = quotes.markets[0].prices.map(function (a) { return a.close; }).reverse();
        this.quoteDates = quotes.markets[0].prices.map(function (a) { return a.date.toString(); }).reverse();
        this.chartData = {
          labels: this.quoteDates,
          datasets: [
            {
              label: '',
              pointBackgroundColor: "#008FD5",
              pointBorderColor: "#008FD5",
              borderColor: "#008FD5",
              data: this.closingPrices
            }
          ]
        }

        var ctx = $("#my-chart");
        var myChart = new Chart(ctx, {
          type: 'line',
          data: this.chartData,
          options: this.chartOptions
        });

        setTimeout(function(){ 
          this.showChart = true;
        }, 0);

      });

    })
  }

}
grofit commented 8 years ago

Your code seems ok, the only difference is that in the plugin it updates the data and calls update on the chart rather than replacing it (which is what used to happen before version 2.0), one other thing which is worth noting in the 2.0 version as chart.js does some crazy stuff to the models it only monitors the data element for changes not the labels (but you are changing both there).

I will see if given your data I can replicate the scenario later.

gregdouglas commented 8 years ago

Hi. I have managed to come up with a similar issue, but this time using two different data-sets.

When adding a entry, I expect both pie charts to reflect the change. Here are the files I have changed in your examples folder:

examples/src/app.html

<template>
    <fieldset>
        <legend>Line Graph via Attribute</legend>
        <canvas id="line-chart" chart="type: line; 'should-update': false; data.bind: SimpleLineData"></canvas>
        <p>Changingddd the data wont change this graph as it is not update-able</p>
    </fieldset>

    <fieldset>
        <legend>Doughnut Chart via Element</legend>
        <chart id="dynamic-doughnut-chart" type="doughnut" style="width: 50%; height: 50%; display: block;" should-update="true" throttle="2000" data.bind="DynamicDoughnutData"></chart>
        <chart id="dynamic-doughnut-chart2" type="doughnut" style="width: 50%; height: 50%; display: block;" should-update="true" throttle="2000" data.bind="DynamicDoughnutData2"></chart>
        <div>
            <label>Values</label>
            <div repeat.for="i of DynamicDoughnutData.datasets[0].data.length">
                <input value.bind="DynamicDoughnutData.datasets[0].data[i]" placeholder="Value ${$index + 1}"/>
            </div>
        </div>

        <p>Changing the above value should trigger a chart refresh after 2 seconds</p>
        <p>Although dynamically added entries will not be tracked currently</p>
        <button click.delegate="addEntry()">Add Entry</button>
        <button click.delegate="resetPieData()">Reset</button>
    </fieldset>
</template>

examples/src/app.js

export class App
{
    constructor(){
        this.DynamicDoughnutData = {};
       this.DynamicDoughnutData2 = {};
        this.SimpleLineData = {};

        this.resetPieData();
        this.resetLineData();
        this.resetPieData2();
    }

    resetPieData2(){
         this.DynamicDoughnutData2 = {
            labels: ["Red1", "Green1", "Yellow2" ],
            datasets: [
                {
                    data: [400, 10, 300],
                    backgroundColor: [
                        "#FF6384",
                        "#36A2EB",
                        "#FFCE56"
                    ],
                    hoverBackgroundColor: [
                        "#FF6384",
                        "#36A2EB",
                        "#FFCE56"
                    ]
                }]
        };
    }

    resetPieData() {
        this.DynamicDoughnutData = {
            labels: ["Red", "Green", "Yellow" ],
            datasets: [
                {
                    data: [300, 50, 100],
                    backgroundColor: [
                        "#FF6384",
                        "#36A2EB",
                        "#FFCE56"
                    ],
                    hoverBackgroundColor: [
                        "#FF6384",
                        "#36A2EB",
                        "#FFCE56"
                    ]
                }]
        };
    }

    resetLineData() {
        this.SimpleLineData = {
            labels: ["January", "February", "March", "April", "May", "June", "July"],
            datasets: [
                {
                    label: "Healthy People",
                    backgroundColor: "rgba(220,220,220,0.2)",
                    borderColor: "rgba(220,220,220,1)",
                    pointColor: "rgba(220,220,220,1)",
                    pointStrokeColor: "#fff",
                    pointHighlightFill: "#fff",
                    pointHighlightStroke: "rgba(220,220,220,1)",
                    data: [65, 59, 80, 81, 56, 55, 40]
                },
                {
                    label: "Ill People",
                    backgroundColor: "rgba(151,187,205,0.2)",
                    borderColor: "rgba(151,187,205,1)",
                    pointColor: "rgba(151,187,205,1)",
                    pointStrokeColor: "#fff",
                    pointHighlightFill: "#fff",
                    pointHighlightStroke: "rgba(151,187,205,1)",
                    data: [28, 48, 40, 19, 86, 27, 90]
                }
            ]
        };
    }

    addEntry() {
        this.DynamicDoughnutData.labels.push("New Colour");
        this.DynamicDoughnutData.datasets[0].data.push(50);
        this.DynamicDoughnutData.datasets[0].backgroundColor.push("#B4FD5C");

        this.DynamicDoughnutData2.labels.push("New Colour");
        this.DynamicDoughnutData2.datasets[0].data.push(50);
        this.DynamicDoughnutData2.datasets[0].backgroundColor.push("#B4FD5C");
    };
}
grofit commented 8 years ago

So I have just tried the scenario you mention above and it has highlighted a minor issue with the attribute binding as it does not use the observable check that the element does, also in the example I was using chart="'should-update':true;" which is incorrect as there should be no quotes around it, as that stops it resolving.

Will fix that bit of code and update the repo (should be version 0.2.1 when I am done), if you can give that one a go (once its pushed) and see if it fixes the attribute related issues you had @sevenshadow

grofit commented 8 years ago

Upon digging further into the whole multiple subscribed charts have issues thing, I think its down to aurelia DI'ing the same instance rather than a new one, which I had been told multiple times would be the case, so will raise a new issue for that and go see if I can explicitly tell it to new an observer up per element/attribute.

grofit commented 8 years ago

@gregdouglas check out #11 (version 0.2.2) this should be fixed now. I will let @sevenshadow be the one to indicate if this issue is deemed fixed with the latest updates.

gregdouglas commented 8 years ago

@grofit - that's awesome - I will give this a go sometime today - thanks as usual for your speedy response. :)

EDIT - For me, the issues I was having have been solved with the latest release.

sevenshadow commented 8 years ago

Thanks for the work on this. I really appreciate it. I should have time tonight to take a look at this again and see if it is resolved on my end.

sevenshadow commented 8 years ago

Hi,

This looks good for me now. I appreciate the help. If I see something else, I will let you know. Also, let me know if you would like to see code samples of how I implemented this in typescripts (although they are not the different from the above examples).

grofit commented 8 years ago

Hey,

Good to know, I am more than happy to hear about bugs etc so please raise anything that breaks for you. As far as TS stuff I usually write everything in TS but back when I first created these plugins aurelia didnt really have TS bindings and the support for TS was awful, so I just never bothered expending the effort in re-writing it when TS support improved.

Anyway closing this issue, happy charting.