Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dolanmiu opened this issue Jan 30, 2020 · 13 comments
Open

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

dolanmiu opened this issue Jan 30, 2020 · 13 comments

Comments

@dolanmiu
Copy link

dolanmiu commented Jan 30, 2020

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

dolanmiu commented Feb 3, 2020

@KacperMadej Pending your reply

@KacperMadej
Copy link
Contributor

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
Copy link
Author

dolanmiu commented Feb 3, 2020

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

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

@h11a
Copy link

h11a commented Feb 5, 2020

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

@dolanmiu
Copy link
Author

dolanmiu commented Feb 5, 2020

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
Copy link

h11a commented Feb 5, 2020

done

@stevethemacguy
Copy link

stevethemacguy commented Feb 19, 2020

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
Copy link

h11a commented May 21, 2020

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

@dolanmiu
Copy link
Author

I would still like this as a feature too

@kdbruin
Copy link

kdbruin commented Jul 29, 2022

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

@karolkolodziej
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants