Chart.js: Updating datasets with points

[stockiNail]

Expected Behavior

In version 2.9.3, when you want to update a datasets with data as points (i.e.scatter), you could change the "point" object in its x and y properties.
The following code is "randomize" implementation (updated by me to show how it works in 2.9.3) of Scatter chart in CHART.js samples.

document.getElementById('randomizeData').addEventListener('click', function() {
    scatterChartData.datasets.forEach(function(dataset) {
        dataset.data.forEach(function(item) {
            item.x = randomScalingFactor();
            item.y = randomScalingFactor();
        });
    });
    window.myScatter.update();
});

Current Behavior

With version 3, the above code does not work and the data points are not updated and the chart does not change the data representation.

To work properly it seems you have to create every time new point objects (not good for performance), as the code of Scatter chart sample is showing:

document.getElementById('randomizeData').addEventListener('click', function() {
    scatterChartData.datasets.forEach(function(dataset) {
        dataset.data = dataset.data.map(function() {
            return {
                x: randomScalingFactor(),
                y: randomScalingFactor()
            };
        });
    });
    window.myScatter.update();
});

Possible Solution

In my opinion it should work as was in version 2.9.3 because it's more efficient.

Environment

• Chart.js version: DevBuild: https://www.chartjs.org/dist/master/chart.min.js
• Browser name and version: FF 76.0.1

[benmccann]

I'm not sure we should be encouraging people to modify properties on the elements directly. It seems like a better API to modify the input data. What do you think @etimberg @kurkle ?

[kurkle]

The demo is modifying Input data, not elements. ArrayEquals fails to notice the change (compares the objects, and those are still the same). I'd consider this to be a bug.

(assuming dataset.data is the Input data, I did not verify)

[stockiNail]

@kurkle that's exactly what I meant.
The object itself can be the same but the equals should be applied to the values which are contained by object. That's my object oriented mindset which is speaking! ;)
Otherwise we should declare the data points as immutable but honestly it does not make much sense in this use case, in my opinion.

[stockiNail]

@kurkle @benmccann let me also apologize if I have started going to Chart.js 3 last week and I didn't find those issues before. It has been just a matter of time.

[stockiNail]

@kurkle I have tested right also to change the array for floating bars. Same error.
Therefore not only for points but also arrays (use as data) for floating bars.

[etimberg]

I agree with @kurkle. While lots of frameworks these days have immutable data, we haven't required it in the past. I think this is a bug we'll need to fix for the v3 release

[benmccann]

Thanks for the correction @kurkle! I agree this seems like a bug to fix. Maybe we should remove the data checks and just support the splice/push/etc. API for fast data modification?

@stockiNail super glad for all your feedback. We've only had one alpha so far, so it's still very early and appreciated!! :-)

[stockiNail]

Maybe we should remove the data checks and just support the splice/push/etc. API for fast data modification?

@benmccann in my opinion this is a constraint that sounds against to object oriented. The array is just the container of the data and not the data and then the object of array can be changed and in cascade mode also the array is changed. Going to the proposed direction it's the same to declare to use immutable objects.
I don't know the reason behind this data checking before updating, because maybe it could be removed and update the chart every time.

I forgot to mention that I have developed a plugin where I'm checking if the data are changed in order to update the chart only if needed.

To check the content of 2 arrays, I take a string representation of both and compare them.
This is done by JSON.stringify.

In this way, whatever is the content of arrays data (numbers, strings, arrays or objects) can be compared.
This also helps instead to scan all array items recursively and check if any property values are changed (deleted or inserted) and also can check custom properties apply to a point objects by a controller, a plugin or by user for specific use case (for instance metadata).

In terms of performance, it couldn't be a problem I guess.

There is only a point of attention if the user adds cycled references to objects inside points and in this case you will get an exception. But it should be a bad practice to have cycled references.