core.controller.js bug breaking chartjs-annotation and chartjs-draggable plugins (configMerge issue)

[nullpointer77]

With the release of 2.7.x the chartjs-plugin-annotation(https://github.com/chartjs/chartjs-plugin-annotation) and chartjs-plugin-draggable (https://github.com/compwright/chartjs-plugin-draggable) are now broken.

If you pull down their sample files you can see that by using chart.js 2.7.x doesn't work while 2.6.x works just fine. I have filed issues in those respective plugins and posted temporary work arounds for these problems, but would like some clarification from the chart.js team if this is a bug or an architecture change in the framework.

What I have discovered is that the reason why these plugins are breaking is the in the samples they are doing something like this. . .

Please forgive the pseudo code

var anno = [{annotation_config}];

var chartOptions = {
. . . .
annotation: {annotations: anno},
. . . .
}

var myChart = new Chart(ctx, chartOptions);

function addLine(){
anno.push({another_annotation_config});
myChart.update(0);
}

What you will see if you inspect the chart object in chrome's console is that after the addLine function is called the annotation array is not updated with the added value.

I did some digging in the various commits leading up to the release of 2.7.x and saw that /src/core/core.controller.js was changed in commit 333f2eb#diff-de7249441169d0e646c53d8220e8b0c5

Looking here the chart options were changed to use something called helpers.configMerge(). . . Looking at that method it appears to copy all configuration to a new array and then set it back as the chart.options property. I believe that this is effectively losing reference to outside arrays/objects so that they can no longer be changed by their original object but rather must be accessed via the chart itself. Example of what I'm talking about below.

var anno = [{annotation_config}];

var chartOptions = {
. . . .
annotation: {annotations: anno},
. . . .
}

var myChart = new Chart(ctx, chartOptions);

function addLine(){
//DOES NOT WORK AS REFERENCE IS LOST DUE TO MERGE
anno.push({another_annotation_config});
myChart.update(0);
}


//WORKS FINE AS YOU ARE EDITING THE NOW "INTERNAL" CONFIGURATION THAT WAS MERGED
Chart.helpers.each(Chart.instances,(instance) => {
            if (chartId == instance.canvas.id) {
                instance.options.annotation.annotations.push({another_annotation_config});
                return;
            }
        });

So ultimately what I'm asking here is was this done as part of an architecture change, meaning you want people to not be able to update external objects/arrays but rather update them on the instance itself? Was this a bug that was introduced?

[simonbrunel]

Good catch @nullpointer77, I didn't notice that issue when reviewing #4198. That's not the expected behavior since we don't want to break existing projects that modify the original config object instead of chart.options. Fortunately, these changes hasn't been released yet!

@xg-wang do you think you can easily fix this issue?

[xg-wang]

@simonbrunel Sorry I wasn't aware of the plugin when writing that pr. Right now I'm on my trip so I can only put more time in this next week.

[simonbrunel]

I think it actually breaks updating all plugin options (e.g. http://www.chartjs.org/samples/master/charts/area/line-datasets.html -> clicking propagate doesn't work anymore), whatever method we use. Changing the chart.options or chart.config.options reference is definitely not a solution because we keep references on nested values (e.g. chart.options.plugins.*).

We should add some unit tests on the config.options reference.

@etimberg @xg-wang it's quite blocking, should we revert #4198?

[xg-wang]

@simonbrunel Agree to revert #4198 until it's compatible with plugin options.

[nullpointer3798]

Thanks for taking a look at this guys, and I appreciate the work you guys do keeping chart.js a great product!

@simonbrunel Just a heads up, I'm pretty sure that this is out in your production builds, as npm chart.js will pull in 2.7.1 which contains this issue. I'm not sure if it's appropriate to cut a release to 2.7.2 or not. Regardless, I don't really care one way or the other as I have worked around this problem in my code base and posted this work around in various plugin forums.

[simonbrunel]

If the issue is in 2.7.1, then it doesn't come from the commit you reported in your first message. Can you build a jsfiddle that reproduces this issue with 2.7.1 and one of this plugins?

[nullpointer3798]

Certainly. https://jsfiddle.net/LLxgy9rh/

You can just change the chart.js version from 2.7.x to 2.6 to see it working correctly.

[nullpointer3798]

@simonbrunel did a quick search in the chart.js bundle for 2.7.1 and found this. . .

	function initConfig(config) {
		config = config || {};

		// Do NOT use configMerge() for the data object because this method merges arrays
		// and so would change references to labels and datasets, preventing data updates.
		var data = config.data = config.data || {};
		data.datasets = data.datasets || [];
		data.labels = data.labels || [];

		config.options = helpers.configMerge(
			defaults.global,
			defaults[config.type],
			config.options || {});

		return config;
	}

Perhaps this is the cause of the issue in the 2.7.1 release. Comment seems to state more or less what I was saying at the beginning of this thread. I haven't traced this back to a changeset unfortunately. Need to take off for the night.

[simonbrunel]

Thanks @nullpointer77, the issue you reported might be caused by #4422 (2.7.0) which now always clones values when merging (including arrays). Your fiddle helps me to understand better the use case and finally, I don't think it should be supported:

Since 2.0.0, a new options object has always been generated (679fa4e) when creating a chart:

var options = {foo: {bar: [1, 2, 3]}};
var config = {data: data, options: options};
var chart = new Chart('id', config);

// options !== chart.options
// options !== chart.config.options
// options !== config.options

// config === chart.config
// config.options === chart.options
// config.options === chart.config.options

The difference since 2.7.0 is:

// 2.6.0-
// options !== chart.options
// options.foo !== chart.options.foo (new reference)
// options.foo.bar === chart.options.foo.bar (copied - same reference)

// 2.7.0+
// options !== chart.options
// options.foo !== chart.options.foo (new reference)
// options.foo.bar !== chart.options.foo.bar (cloned - new reference)

The 2.7.0+ behavior seems more consistent since it doesn't keep references on the original values whatever the value type. It's also safer and more predictable when creating charts from the same initial options, then modifying only one chart instance.

I guess that being able to update options via a reference on a child array wasn't a feature, but more a bug/open door, since it wasn't possible to do that on a child object. I think we always promoted updating options via config.options or from chart.options since you still need chart to call update() (I would personally always recommend the last one).

#4198 still introduces a bug since now, the options references are also changed when calling update(), which break cached plugin options. We might need to fix both, the options update (#4198) and the plugin caching logic (#3363).

Though, fixing #4198 will not fix this issue and I don't think it should be fixed in the core library but rather in broken plugins repositories, that (from my point of view) misuse the options object.

@etimberg you might have some inputs as well?

[etimberg]

I agree that the 2.7 behaviour seems more consistent and I think it's what we should support going forward.

Could we perhaps have a warning that displays during update() if the options and data didn't change? Then users who see this case would get something that could even link to this issue to help understand why no updates happened.

Regarding #4198 I think the fix actually isn't too bad, we just have to write some extra work to copy all the keys into the chart.options object and never re-assign it.

[simonbrunel]

Instead of a (probably annoying) sticky console warning, we could update the documentation to explicitly state on the correct way to update the options (maybe only chart.options), with a link to that issue.

[nullpointer77]

So it sounds like the conclusion that was reached is to use chart.options instead of array references. I will go update the same files and throw together some documentation on the various plugins that I use that alerts people that they may have to change their code to use the chart.options approach.

Thanks for looking into this!

[ThinkerBell1]

@nullpointer77 - Hi! I think I have a similar issue to this..
I want to display a toolTip when moseOver using the annotation-plugin, and it throws the same error.
You wrote:

So it sounds like the conclusion that was reached is to use chart.options instead of array references. I will go update the same files and throw together some documentation on the various plugins that I use that alerts people that they may have to change their code to use the chart.options approach.

Now I couldn't find a reference to this, can you add a link or explanation how to solve the problem ?
Thanks in advance !

[nullpointer77]

@ThinkerBell1 It's been a little while as I have written a "helper" layer around chartjs-draggable and chartjs-annotation to deal with this in my angular6 app, but here goes nothing.

The problem is that internally chartjs is cloning the options parameters you are passing in upon chart creation. This means if you have an options variable sitting in your code and you go to update a value in it, chartJS has cloned this object and doesn't recognize the change. An example of this is below.

//WRONG way of dealing with this.
var opt ={ stuff : 'more stuff'}
new Chart(opts);

opt.stuff = 'even more stuff';

The correct approach is if you wanted to change the values of a tooltip or any options that you passed you need to do it via referencing the opts variable from the chart context. . . like ah so.

//RIGHT way of doing this.

var opt ={ stuff : 'more stuff'}
var awesomeChart = new Chart(opts);

awesomeChart.opts.stuff = 'even more stuff';

I hope this helps you out. If you are able to hit me with jsfiddle example of what you are trying to accomplish I can likely help you out more.

[ThinkerBell1]

@nullpointer77 - Thanx for your answer, and sorry for my delayed response.
I was using your suggestion of the right way of doing it, but still had the problem.
At the end I was able to solve it by adding an id to the annotation..
more about it here: Annotation event not updating with Chart.js 2.7

[Svetomechc]

chartjs-plugin-annotation is broken again with 2.8.0

[benmccann]

@Svetomechc I see you also reported the bug in chartjs/chartjs-plugin-annotation#156, so let's track it there since there aren't enough details that it's clear that breakage is related to the issue being talked about here

[Svetomechc]

@benmccann fair point

[etimberg]

Starting with v3.0.0-beta.11 the config is no longer merged. This issue is still tracked on the annotation side