Skip to content

Commit

Permalink
Fix updating plugin options
Browse files Browse the repository at this point in the history
Cached plugin descriptors hold a reference on the plugin options, which break if the plugin options object is replaced. That case happens when the user updates the plugin options with a new object, but also since the new config update logic (chartjs#4198) that now always clones the plugin options. The fix consists in explicitly invalidating that cache before updating the chart.
  • Loading branch information
simonbrunel committed Jan 13, 2018
1 parent 2f5a3e1 commit 49d7e8f
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 1 deletion.
4 changes: 4 additions & 0 deletions src/core/core.controller.js
Expand Up @@ -376,6 +376,10 @@ module.exports = function(Chart) {

updateConfig(me);

// plugins options references might have change, let's invalidate the cache
// https://github.com/chartjs/Chart.js/issues/5111#issuecomment-355934167
plugins._invalidate(me);

if (plugins.notify(me, 'beforeUpdate') === false) {
return;
}
Expand Down
12 changes: 11 additions & 1 deletion src/core/core.plugins.js
Expand Up @@ -121,7 +121,7 @@ module.exports = {
* @private
*/
descriptors: function(chart) {
var cache = chart._plugins || (chart._plugins = {});
var cache = chart.$plugins || (chart.$plugins = {});
if (cache.id === this._cacheId) {
return cache.descriptors;
}
Expand Down Expand Up @@ -157,6 +157,16 @@ module.exports = {
cache.descriptors = descriptors;
cache.id = this._cacheId;
return descriptors;
},

/**
* Invalidates cache for the given chart: descriptors hold a reference on plugin option,
* but in some cases, this reference can be changed by the user when updating options.
* https://github.com/chartjs/Chart.js/issues/5111#issuecomment-355934167
* @private
*/
_invalidate: function(chart) {
delete chart.$plugins;
}
};

Expand Down
33 changes: 33 additions & 0 deletions test/specs/core.plugin.tests.js
Expand Up @@ -339,6 +339,39 @@ describe('Chart.plugins', function() {

expect(plugin.hook).toHaveBeenCalled();
expect(plugin.hook.calls.first().args[1]).toEqual({a: 'foobar'});

delete Chart.defaults.global.plugins.a;
});

// https://github.com/chartjs/Chart.js/issues/5111#issuecomment-355934167
it('should invalidate cache when update plugin options', function() {
var plugin = {id: 'a', hook: function() {}};
var chart = window.acquireChart({
plugins: [plugin],
options: {
plugins: {
a: {
foo: 'foo'
}
}
},
});

spyOn(plugin, 'hook');

Chart.plugins.notify(chart, 'hook');

expect(plugin.hook).toHaveBeenCalled();
expect(plugin.hook.calls.first().args[1]).toEqual({foo: 'foo'});

chart.options.plugins.a = {bar: 'bar'};
chart.update();

plugin.hook.calls.reset();
Chart.plugins.notify(chart, 'hook');

expect(plugin.hook).toHaveBeenCalled();
expect(plugin.hook.calls.first().args[1]).toEqual({bar: 'bar'});
});
});
});

0 comments on commit 49d7e8f

Please sign in to comment.