highcharts / highcharts-angular

Highcharts official integration for Angular
Other
430 stars 117 forks source link

[options] in <highcharts-angular> is not immutable #174

Open dolanmiu opened 4 years ago

dolanmiu commented 4 years ago

When passing options into <highcharts-angular> via the async pipe, it mutates the options object.

<highcharts-chart
  *ngIf="chartOptions$ | async as chartOptions"
  class="chart"
  [Highcharts]="Highcharts"
  [options]="chartOptions"
  [runOutsideAngular]="true"
></highcharts-chart>

chartOptions$ is an observable which takes chartOptions from ngrx store, but whenever I change data in the store, it mutates the PREVIOUS value stored in the store.

I fixed this by deep cloning the chartOptions before it gets passed into highcharts-cangular:

JSON.parse(JSON.stringify());

But can we have an official fix?

Here is the StackBlitz to re-produce the problem:

https://stackblitz.com/edit/angular-soounx

KacperMadej commented 4 years ago

Hi @dolanmiu

In Highcharts series data is not copied but is being used directly for better performance. Could you provide more info about the changes that are being done in the chartOptions?

dolanmiu commented 4 years ago

Can you provide a solution for making it so that it copies it rather than using it directly?

So in the app im working on, we change "tabs" and each tab has the SAME GRAPH, but with different values/data.

So in essence, the series changes whenever the tab changes

dolanmiu commented 4 years ago

@KacperMadej Pending your reply

KacperMadej commented 4 years ago

If you have a live demo that pleases share and we could confirm if there is any other option to resolve this.

The data will be used directly and to change that we would have to either change how Highcharts are processing data or add some code that would provide this feature. Both options are enhancement suggestions and could be implemented if enough votes from clients and community will be gathered.

For now, you could make a copy of the data, so if you provide any data to a Highcharts chart, then you could expect that it will be at least altered to some degree.

Let's keep this issue as a tracked for the feature request. If you would like to see this feature implemented please vote with a thumb up emoji for the first and opening comment of this issue.

dolanmiu commented 4 years ago

@KacperMadej I have made a StackBlitz to demonstrate the problem:

https://stackblitz.com/edit/angular-soounx

h11a commented 4 years ago

Also experiencing this problem and it is fixed with JSON.parse(JSON.stringify());... An optional param for mutability would be nice.

dolanmiu commented 4 years ago

Also experiencing this problem and it is fixed with JSON.parse(JSON.stringify());... An optional param for mutability would be nice.

Quote:

Let's keep this issue as a tracked for the feature request. If you would like to see this feature implemented please vote with a thumb up emoji for the first and opening comment of this issue.

Can you upvote the first comment so that they can prioritise the fix?

Thanks

h11a commented 4 years ago

done

stevethemacguy commented 4 years ago

My use case is similar, but doesn't involve the async pipe. In our app, we create multiple charts using the same chart options. This allows us to modify the options slightly for different charts without having to duplicate a large set of options for every variation.

For example, if Chart A and B are both constructed from the same chartOptions property in Angular, both instances will share the same options. Modifying Chart B in response to a user action has the side effect of also mutating the options for Chart A. To get around this, we use cloneDeep (Lodash) when instantiating Chart B, so that changes to Chart B don't modify Chart A.

However, cloning the chartOptions on construction-only isn't usually sufficient. If the options for Charts A or B change at a later point, it is sometimes necessary to re-clone the options again to 'reset' the options after accidental mutation.

You might ask why we don't just create completely separate chartOptions for every chart, but this is un-maintainable for a large app (we would have hundreds of chart configs to keep track of).

For example, in our app, users can click a button to open a larger version of the chart in a 'full screen' NG-Boostrap modal. The larger ('full-screen') version of the chart requires slightly different chartOptions (e.g. increased font-size, adding a title, etc), but it doesn't warrant creating a completely separate set of chartOptions. In this case, it makes sense to re-use the same chart options and only modify them when the user clicks the button.

When the modal is closed, we destroy the 'full-screen' chart, but the original chart was mutated as a side effect, so we have to perform another cloneDeep operation to 'undo' the changes made to the original chart.

Here's an example from our app. Note: We keep the chartOptions for all charts in separate config files to avoid clutter in the component code.

// Import the 'base' chart options. The 'full-screen' chart will also share these options.
import * as BarChartConfig from './charts/bar-chart';

// Use the same BarChartConfig for two different charts.
// The fullscreen options will be modified when the user clicks a button.

this.barChartOptions = cloneDeep(BarChartConfig);
this.barChartOptionsFullScreen = cloneDeep(BarChartConfig);

// Once the user closes the modal, reset the chart options again to 'undo' the changes
// made to the original barChartOptions (which were mutated as a side effect).
this.barChartOptions = cloneDeep(BarChartConfig);

Cloning in this way fixes several bugs that occur when mutating the chart options while the app is running. Hopefully this provides some context into how an 'immutable' feature could be useful.

We were previously using the angular2-highcharts plugin, but are now switching to the 'official' library in order to (hopefully) move to Angular 9 once #161 is fixed.

h11a commented 4 years ago

Any progress on whether this will be added as a feature?

dolanmiu commented 4 years ago

I would still like this as a feature too

kdbruin commented 2 years ago

Man, 2 years later and this is still an issue. Any possible workarounds?

karolkolodziej commented 2 years ago

Since 10.1.0 we introduced on the chart config the allowMutatingData property, API. Setting that on the chart solves the mutation issue but might negatively influence the chart performance in the case of heavier data sets.