n3-charts / line-chart

Awesome charts for AngularJS.
http://n3-charts.github.io/line-chart/
MIT License
1.2k stars 179 forks source link

add option to control whether graph data is deep-watched by Angular #519

Closed epistemancer closed 1 year ago

epistemancer commented 8 years ago

Hi guys, I haven't filed an issue for this, but I'm having performance problems sometimes because of the deep watch on the data. When there's a lot of data in the charts -- tens of thousands of points -- it takes significant time for Angular to inspect it every digest cycle, even when it isn't changing. I've added an optional attribute to options to make the watch on data shallow. Please LMK what you think.

lorem--ipsum commented 8 years ago

Hi, thanks for the PR ! I'm cool with merging this, however could you move the default into Options.ts ?

epistemancer commented 8 years ago

Sure! Options.ts is already handling the default, but if you're talking about the options ? options.deepWatchData : true bit in LineChart.ts, that was added to handle a case I ran into during testing. The watch on scope.data can fire before the watch on scope.options has ever fired, which means options is still undefined.

I've modified it to set options to an empty Options.Options at the start of the link method, so that it will use defaults until the watch on scope.options fires. Does that look better to you?

chaosmail commented 7 years ago

This looks great, thanks a lot. I think this could be a really useful feature, especially for static data.

Considering that this is an angular specific optimization option should we put the option into an angular or optimization property, such as

$scope.options = {
   ..,
  angular: {
    deepWatchData: false,
  }
}

This might sound ridcioulos because this is a Angular directive, however we also support React (and Angular 2 hopefully soon), we better keep these options organized.

@epistemancer it would be great if we could have a unit test verifiying the options and an e2e test on this.

@lorem--ipsum did you assign the do not merge label because you would like to merge this to the dev branch?