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

setData mutating source array #4259

Closed
mjyeaney opened this issue Jun 3, 2015 · 24 comments
Closed

setData mutating source array #4259

mjyeaney opened this issue Jun 3, 2015 · 24 comments
Assignees
Labels
Type: Feature Request Used when a new feature is requested either directly or indirectly Type: Undecided

Comments

@mjyeaney
Copy link

mjyeaney commented Jun 3, 2015

Seeing a strange behavior against Highcharts 4.1.5. When providing an Array of numbers (i.e, [1,2,3,4]), after some threshold the source array seems to be mutated into the following form:

[{y = 1}, {y=2}, {y=3}, {y=4}]

This seems to happen on larger sets of data (1300 - 1400 points + ), but we can't seem to get turboThreshold or cropThreshold to make a difference. Is there a setting we're missing, or is this a known bug?

Normally not an issue, but we're visualizing running time-series data windows and as a result are hanging on to the arrays we're calling setData() with. Once they are mutated, they are effectively "broke"...upstream sorting/binning code doesn't expect the inner objects, and stops working.

Thanks for your help!

@mjmckp
Copy link

mjmckp commented Jun 16, 2015

I'm seeing this as well

@TorsteinHonsi
Copy link
Collaborator

Can you provide a demo? I set up a test case, but even when running 10 000 points the numbers are preserved in the original array.

@mjyeaney
Copy link
Author

I created a trimmed-down version of the basic approach we're using [http://jsfiddle.net/LLExL/4807/]. However, as I mentioned, I can't trigger the exact use case (nor could I before). The only remaining significant difference between this example and the real code is there are 8 - 10 graphs (4 - 5 pairs just like the demo).

Can you see any issues with the approach shown that may cause issues?

@TorsteinHonsi
Copy link
Collaborator

There is one workaround that should be valid under any circumstance - if you copy (splice) the array before running setData, you are sure it isn't modified:

series.setData(myData.splice(0));

@andylash
Copy link

andylash commented Oct 2, 2015

Don't have a reliable repro either, but we also see this in 4.1.6. It seems to happen on certain charts with certain data, which doesn't make any sense.

@vickychijwani
Copy link

This seems related to #4701 / #3604, which are presumably fixed in the next version (likely v4.1.10).

@LinGG
Copy link

LinGG commented Dec 21, 2015

here is the demo. http://jsfiddle.net/xq29x6ja/3/

p.s. seems it is nothing about turboTreshold

@TorsteinHonsi
Copy link
Collaborator

@TorsteinHonsi
Copy link
Collaborator

@pawel: Consider the fiddle above.

Like we talked about earlier, creating a copy of of the source data is expensive, especially when dealing with a large amount of data. A leaner solution would be to identify the places where the source data is mutated, and do the copy there. Like in the case above, it is fixed by creating a copy in point.update:

Find this code:

            // Record the options to options.data. If there is an object from before,
            // use point options, otherwise use raw options. (#4701)
            seriesOptions.data[i] =  (isObject(seriesOptions.data[i]) && !isArray(seriesOptions.data[i])) ? point.options : options;

Before it, add this code:

            // If this is the first time we're writing back to options.data, create
            // a copy so that we don't mutate the source array. (#4259)
            if (!seriesOptions.data.isCopy) {
                seriesOptions.data = seriesOptions.data.slice();
                seriesOptions.data.isCopy = true;
            }

What do you think? And do you have other cases of data source mutation where this fix doesn't work?

@pawelfus
Copy link
Contributor

The most popular are ones with setData() and when recreating the chart. I setup two use cases:

@TorsteinHonsi
Copy link
Collaborator

Yes, the first case is a different issue, but it raises another question. If we don't want to mutate the input options at all, it will require a wide range of changes in our source code. For example, the exported chart is based on the idea that the configuration options are mutated and kept in sync with the state of the chart.

@stale
Copy link

stale bot commented Aug 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions!

@stale stale bot added the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Aug 29, 2019
@pbowyer
Copy link

pbowyer commented Aug 29, 2019

Has this issue been addressed? If not, I feel the issue should not be closed.

@stale stale bot removed the Status: Stale This issue hasn't had any activity for a while, and will be auto-closed if no further updates occur. label Aug 29, 2019
@KacperMadej
Copy link
Contributor

The current status of the issue is that creating a copy of data is too expensive (performance- and memory-wise). The issue is open, but undecided how it will be resolved - fixed through a code change or dismissed.

As a workaround, you could provide a copy of the original data when using Highcharts.

@soetji
Copy link

soetji commented May 4, 2020

If I Object.freeze() the original data, Chart.update() seems to work still. Would you recommend this approach in addition to copying the data?

https://jsfiddle.net/soetji/atz64f8h/

@pawelfus
Copy link
Contributor

pawelfus commented May 4, 2020

If you make a deep copy @soetji then you don't need to use Object.freeze(). Note that freeze does not work with nested props: https://jsfiddle.net/BlackLabel/uprL2k6v/

@soetji
Copy link

soetji commented May 4, 2020

Thanks @pawelfus. Sorry that my question wasn't clear. In my example, my data was a plain array so I used Object.freeze(). For nested props, I would use some deep freeze function.

I have a huge amount of data to display and need to preserve. So first, I want to avoid copying the data in order to preserve memory. Also I don't want Highcharts to change it. So I freeze the data. It seems that Highcharts still update properly with frozen data. Is this a recommended approach?

@pawelfus
Copy link
Contributor

pawelfus commented May 5, 2020

So first, I want to avoid copying the data in order to preserve memory. Also I don't want Highcharts to change it.

Well, you just posted one of the reason why Highcharts does not copy the data. Memory and performance-wise most users don't care if the variable is unchanged (e.g. it's just a local variable from AJAX). The rest should pass on the copy of the dataset.

Is this a recommended approach?

No, personally I don't recommend this approach because of the nested properties issue I explained above. From the maintenance of any application perspective it's the best to avoid caveats like the one with Object.freeze(). If your dataset format will change in future, other dev will spend hours to figure out the reason behind it..

The only perfectly safe solution is to use deep-copy of the dataset in Highcharts API.

@Arut42
Copy link

Arut42 commented May 13, 2020

i did an update from V 5.0.7 to 8.0.4 and using chart.Data as [ [Date,Data], [Date,Data] ....]
since the update, highchart is overwriteing my original data if i use setData([ [Date,Data], [Date,Data] ....]).

is there any way of workaround?
atm i use
chart.series[0].setData($.extend(true, [] , myobj.data1));
but this is really slow on more data.

http://jsfiddle.net/mey6u2pb/

btw. if i use only Data array without Date this dont happen.

@pawelfus
Copy link
Contributor

Hi @Arut42

is there any way of workaround?

Yes, the one you posted: chart.series[0].setData($.extend(true, [] , myobj.data1));. You can also try Array.slice() or Object.freeze() mentioned in the comments above. But deep copy is the safest solution at this moment.

@rocmewtwo
Copy link

Hello, I faced the same issue. when i run these step

data.push(newPoint) // add new point to dataSet
setData(data) // set dataSet to chart

after setData, my dataSet will duplicate newPoint twice.

And I found a workaround to prevent enter this part updatedData = this.updateData(data, animation); in setData is setting series cropThreshold smaller than dataSet, and this issue will not happen again.
I am not sure the exactly issue reason. Hope this can help you.

also refenence: https://api.highcharts.com/highcharts/plotOptions.series.cropThreshold

@ashlincascade
Copy link

We ran into the same issue with the highcharts-react wrapper, as noted by others in highcharts/highcharts-react#326. It was a doozy to track down why charts were changing data elsewhere in our application.

We ended up spreading the series data into a new array when passed to our graph components, which works for our use case.

@Maximaximum
Copy link

How about adding a config option which would decide whether the input data is deep-copied by highcharts internally before mutating it?

@raf18seb
Copy link
Contributor

raf18seb commented Dec 15, 2021

Hi!

How about adding a config option which would decide whether the input data is deep-copied by highcharts internally before mutating it?

That's a good idea. I'm tagging the ticket as a feature request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Request Used when a new feature is requested either directly or indirectly Type: Undecided
Projects
None yet
Development

No branches or pull requests