From 4b0dc2fb1993201ea6573f8be8b5759eb6e30777 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Mon, 28 Jan 2019 22:29:59 +0100 Subject: [PATCH 1/5] Deprecate configMerge and scaleMerge helpers These methods shouldn't have been public since they are specific to the chart controller internal logic. Note that this scale custom merging will be removed in v3. --- src/core/core.controller.js | 78 ++++++++++++- src/core/core.helpers.js | 54 --------- src/helpers/helpers.core.js | 2 +- test/specs/core.controller.tests.js | 114 +++++++++++++++++++ test/specs/core.helpers.tests.js | 166 ---------------------------- 5 files changed, 190 insertions(+), 224 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index a552bb58abf..5a17d1afb64 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -35,16 +35,70 @@ defaults._set('global', { responsiveAnimationDuration: 0 }); +function mergeScaleConfig(/* objects ... */) { + return helpers.merge(helpers.clone(arguments[0]), [].slice.call(arguments, 1), { + merger: function(key, target, source, options) { + if (key === 'xAxes' || key === 'yAxes') { + var slen = source[key].length; + var i, type, scale; + + if (!target[key]) { + target[key] = []; + } + + for (i = 0; i < slen; ++i) { + scale = source[key][i]; + type = helpers.valueOrDefault(scale.type, key === 'xAxes' ? 'category' : 'linear'); + + if (i >= target[key].length) { + target[key].push({}); + } + + if (!target[key][i].type || (scale.type && scale.type !== target[key][i].type)) { + // new/untyped scale or type changed: let's apply the new defaults + // then merge source scale to correctly overwrite the defaults. + helpers.merge(target[key][i], [scaleService.getScaleDefaults(type), scale]); + } else { + // scales type are the same + helpers.merge(target[key][i], scale); + } + } + } else { + helpers._merger(key, target, source, options); + } + } + }); +} + +function mergeConfig(/* objects ... */) { + return helpers.merge(helpers.clone(arguments[0]), [].slice.call(arguments, 1), { + merger: function(key, target, source, options) { + var tval = target[key] || {}; + var sval = source[key]; + + if (key === 'scales') { + // scale config merging is complex. Add our own function here for that + target[key] = mergeScaleConfig(tval, sval); + } else if (key === 'scale') { + // used in polar area & radar charts since there is only one scale + target[key] = helpers.merge(tval, [scaleService.getScaleDefaults(sval.type), sval]); + } else { + helpers._merger(key, target, source, options); + } + } + }); +} + function initConfig(config) { config = config || {}; - // Do NOT use configMerge() for the data object because this method merges arrays + // Do NOT use mergeConfig 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( + config.options = mergeConfig( defaults.global, defaults[config.type], config.options || {}); @@ -59,7 +113,7 @@ function updateConfig(chart) { layouts.removeBox(chart, scale); }); - newOptions = helpers.configMerge( + newOptions = mergeConfig( defaults.global, defaults[chart.config.type], newOptions); @@ -981,3 +1035,21 @@ Chart.Controller = Chart; * @private */ Chart.types = {}; + +/** + * Provided for backward compatibility, not available anymore. + * @namespace Chart.helpers.configMerge + * @deprecated since version 2.8.0 + * @todo remove at version 3 + * @private + */ +helpers.configMerge = mergeConfig; + +/** + * Provided for backward compatibility, not available anymore. + * @namespace Chart.helpers.scaleMerge + * @deprecated since version 2.8.0 + * @todo remove at version 3 + * @private + */ +helpers.scaleMerge = mergeScaleConfig; diff --git a/src/core/core.helpers.js b/src/core/core.helpers.js index 82f2c9811b7..e6a2d2d9139 100644 --- a/src/core/core.helpers.js +++ b/src/core/core.helpers.js @@ -11,60 +11,6 @@ module.exports = function() { // -- Basic js utility methods - helpers.configMerge = function(/* objects ... */) { - return helpers.merge(helpers.clone(arguments[0]), [].slice.call(arguments, 1), { - merger: function(key, target, source, options) { - var tval = target[key] || {}; - var sval = source[key]; - - if (key === 'scales') { - // scale config merging is complex. Add our own function here for that - target[key] = helpers.scaleMerge(tval, sval); - } else if (key === 'scale') { - // used in polar area & radar charts since there is only one scale - target[key] = helpers.merge(tval, [scaleService.getScaleDefaults(sval.type), sval]); - } else { - helpers._merger(key, target, source, options); - } - } - }); - }; - - helpers.scaleMerge = function(/* objects ... */) { - return helpers.merge(helpers.clone(arguments[0]), [].slice.call(arguments, 1), { - merger: function(key, target, source, options) { - if (key === 'xAxes' || key === 'yAxes') { - var slen = source[key].length; - var i, type, scale; - - if (!target[key]) { - target[key] = []; - } - - for (i = 0; i < slen; ++i) { - scale = source[key][i]; - type = helpers.valueOrDefault(scale.type, key === 'xAxes' ? 'category' : 'linear'); - - if (i >= target[key].length) { - target[key].push({}); - } - - if (!target[key][i].type || (scale.type && scale.type !== target[key][i].type)) { - // new/untyped scale or type changed: let's apply the new defaults - // then merge source scale to correctly overwrite the defaults. - helpers.merge(target[key][i], [scaleService.getScaleDefaults(type), scale]); - } else { - // scales type are the same - helpers.merge(target[key][i], scale); - } - } - } else { - helpers._merger(key, target, source, options); - } - } - }); - }; - helpers.where = function(collection, filterCallback) { if (helpers.isArray(collection) && Array.prototype.filter) { return collection.filter(filterCallback); diff --git a/src/helpers/helpers.core.js b/src/helpers/helpers.core.js index 4464eb7baba..1b798c925c8 100644 --- a/src/helpers/helpers.core.js +++ b/src/helpers/helpers.core.js @@ -192,7 +192,7 @@ var helpers = { /** * The default merger when Chart.helpers.merge is called without merger option. - * Note(SB): this method is also used by configMerge and scaleMerge as fallback. + * Note(SB): also used by mergeConfig and mergeScaleConfig as fallback. * @private */ _merger: function(key, target, source, options) { diff --git a/test/specs/core.controller.tests.js b/test/specs/core.controller.tests.js index 9bcb45d1f52..62c54b75ce1 100644 --- a/test/specs/core.controller.tests.js +++ b/test/specs/core.controller.tests.js @@ -159,6 +159,120 @@ describe('Chart', function() { }); }); + describe('when merging scale options', function() { + beforeEach(function() { + Chart.helpers.merge(Chart.defaults.scale, { + _jasmineCheckA: 'a0', + _jasmineCheckB: 'b0', + _jasmineCheckC: 'c0' + }); + + Chart.helpers.merge(Chart.scaleService.defaults.logarithmic, { + _jasmineCheckB: 'b1', + _jasmineCheckC: 'c1', + }); + }); + + afterEach(function() { + delete Chart.defaults.scale._jasmineCheckA; + delete Chart.defaults.scale._jasmineCheckB; + delete Chart.defaults.scale._jasmineCheckC; + delete Chart.scaleService.defaults.logarithmic._jasmineCheckB; + delete Chart.scaleService.defaults.logarithmic._jasmineCheckC; + }); + + it('should default to "category" for x scales and "linear" for y scales', function() { + var chart = acquireChart({ + type: 'line', + options: { + scales: { + xAxes: [ + {id: 'foo0'}, + {id: 'foo1'} + ], + yAxes: [ + {id: 'bar0'}, + {id: 'bar1'} + ] + } + } + }); + + expect(chart.scales.foo0.type).toBe('category'); + expect(chart.scales.foo1.type).toBe('category'); + expect(chart.scales.bar0.type).toBe('linear'); + expect(chart.scales.bar1.type).toBe('linear'); + }); + + it('should correctly apply defaults on central scale', function() { + var chart = acquireChart({ + type: 'line', + options: { + scale: { + id: 'foo', + type: 'logarithmic', + _jasmineCheckC: 'c2', + _jasmineCheckD: 'd2' + } + } + }); + + // let's check a few values from the user options and defaults + + expect(chart.scales.foo.type).toBe('logarithmic'); + expect(chart.scales.foo.options).toBe(chart.options.scale); + expect(chart.scales.foo.options).toEqual( + jasmine.objectContaining({ + _jasmineCheckA: 'a0', + _jasmineCheckB: 'b1', + _jasmineCheckC: 'c2', + _jasmineCheckD: 'd2' + })); + }); + + it('should correctly apply defaults on xy scales', function() { + var chart = acquireChart({ + type: 'line', + options: { + scales: { + xAxes: [{ + id: 'foo', + type: 'logarithmic', + _jasmineCheckC: 'c2', + _jasmineCheckD: 'd2' + }], + yAxes: [{ + id: 'bar', + type: 'time', + _jasmineCheckC: 'c2', + _jasmineCheckE: 'e2' + }] + } + } + }); + + expect(chart.scales.foo.type).toBe('logarithmic'); + expect(chart.scales.foo.options).toBe(chart.options.scales.xAxes[0]); + expect(chart.scales.foo.options).toEqual( + jasmine.objectContaining({ + _jasmineCheckA: 'a0', + _jasmineCheckB: 'b1', + _jasmineCheckC: 'c2', + _jasmineCheckD: 'd2' + })); + + expect(chart.scales.bar.type).toBe('time'); + expect(chart.scales.bar.options).toBe(chart.options.scales.yAxes[0]); + expect(chart.scales.bar.options).toEqual( + jasmine.objectContaining({ + _jasmineCheckA: 'a0', + _jasmineCheckB: 'b0', + _jasmineCheckC: 'c2', + _jasmineCheckE: 'e2' + })); + }); + }); + describe('config.options.responsive: false', function() { it('should not inject the resizer element', function() { var chart = acquireChart({ diff --git a/test/specs/core.helpers.tests.js b/test/specs/core.helpers.tests.js index 796148aaf6c..b0c7431c859 100644 --- a/test/specs/core.helpers.tests.js +++ b/test/specs/core.helpers.tests.js @@ -6,172 +6,6 @@ describe('Core helper tests', function() { helpers = window.Chart.helpers; }); - it('should merge a normal config without scales', function() { - var baseConfig = { - valueProp: 5, - arrayProp: [1, 2, 3, 4, 5, 6], - objectProp: { - prop1: 'abc', - prop2: 56 - } - }; - - var toMerge = { - valueProp2: null, - arrayProp: ['a', 'c'], - objectProp: { - prop1: 'c', - prop3: 'prop3' - } - }; - - var merged = helpers.configMerge(baseConfig, toMerge); - expect(merged).toEqual({ - valueProp: 5, - valueProp2: null, - arrayProp: ['a', 'c'], - objectProp: { - prop1: 'c', - prop2: 56, - prop3: 'prop3' - } - }); - }); - - it('should merge scale configs', function() { - var baseConfig = { - scales: { - prop1: { - abc: 123, - def: '456' - }, - prop2: 777, - yAxes: [{ - type: 'linear', - }, { - type: 'log' - }] - } - }; - - var toMerge = { - scales: { - prop1: { - def: 'bbb', - ghi: 78 - }, - prop2: null, - yAxes: [{ - type: 'linear', - axisProp: 456 - }, { - // pulls in linear default config since axis type changes - type: 'linear', - position: 'right' - }, { - // Pulls in linear default config since axis not in base - type: 'linear' - }] - } - }; - - var merged = helpers.configMerge(baseConfig, toMerge); - expect(merged).toEqual({ - scales: { - prop1: { - abc: 123, - def: 'bbb', - ghi: 78 - }, - prop2: null, - yAxes: [{ - type: 'linear', - axisProp: 456 - }, { - display: true, - - gridLines: { - color: 'rgba(0, 0, 0, 0.1)', - drawBorder: true, - drawOnChartArea: true, - drawTicks: true, // draw ticks extending towards the label - tickMarkLength: 10, - lineWidth: 1, - offsetGridLines: false, - display: true, - zeroLineColor: 'rgba(0,0,0,0.25)', - zeroLineWidth: 1, - zeroLineBorderDash: [], - zeroLineBorderDashOffset: 0.0, - borderDash: [], - borderDashOffset: 0.0 - }, - position: 'right', - offset: false, - scaleLabel: Chart.defaults.scale.scaleLabel, - ticks: { - beginAtZero: false, - minRotation: 0, - maxRotation: 50, - mirror: false, - padding: 0, - reverse: false, - display: true, - callback: merged.scales.yAxes[1].ticks.callback, // make it nicer, then check explicitly below - autoSkip: true, - autoSkipPadding: 0, - labelOffset: 0, - minor: {}, - major: {}, - }, - type: 'linear' - }, { - display: true, - - gridLines: { - color: 'rgba(0, 0, 0, 0.1)', - drawBorder: true, - drawOnChartArea: true, - drawTicks: true, // draw ticks extending towards the label, - tickMarkLength: 10, - lineWidth: 1, - offsetGridLines: false, - display: true, - zeroLineColor: 'rgba(0,0,0,0.25)', - zeroLineWidth: 1, - zeroLineBorderDash: [], - zeroLineBorderDashOffset: 0.0, - borderDash: [], - borderDashOffset: 0.0 - }, - position: 'left', - offset: false, - scaleLabel: Chart.defaults.scale.scaleLabel, - ticks: { - beginAtZero: false, - minRotation: 0, - maxRotation: 50, - mirror: false, - padding: 0, - reverse: false, - display: true, - callback: merged.scales.yAxes[2].ticks.callback, // make it nicer, then check explicitly below - autoSkip: true, - autoSkipPadding: 0, - labelOffset: 0, - minor: {}, - major: {}, - }, - type: 'linear' - }] - } - }); - - // Are these actually functions - expect(merged.scales.yAxes[1].ticks.callback).toEqual(jasmine.any(Function)); - expect(merged.scales.yAxes[2].ticks.callback).toEqual(jasmine.any(Function)); - }); - it('should filter an array', function() { var data = [-10, 0, 6, 0, 7]; var callback = function(item) { From dac95fd49b1153a8c4214b0681f5240927e614d6 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Mon, 28 Jan 2019 22:37:56 +0100 Subject: [PATCH 2/5] Add deprecation tests (and fix lint error) --- src/core/core.helpers.js | 1 - test/specs/global.deprecations.tests.js | 12 ++++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/core/core.helpers.js b/src/core/core.helpers.js index e6a2d2d9139..a30b3723b07 100644 --- a/src/core/core.helpers.js +++ b/src/core/core.helpers.js @@ -5,7 +5,6 @@ var color = require('chartjs-color'); var defaults = require('./core.defaults'); var helpers = require('../helpers/index'); -var scaleService = require('../core/core.scaleService'); module.exports = function() { diff --git a/test/specs/global.deprecations.tests.js b/test/specs/global.deprecations.tests.js index d358742a594..d9e705a1c32 100644 --- a/test/specs/global.deprecations.tests.js +++ b/test/specs/global.deprecations.tests.js @@ -42,6 +42,18 @@ describe('Deprecations', function() { expect(Chart.types).toEqual({}); }); }); + + describe('Chart.helpers.configMerge', function() { + it('should be defined as a function', function() { + expect(typeof Chart.helpers.configMerge).toBe('function'); + }); + }); + + describe('Chart.helpers.scaleMerge', function() { + it('should be defined as a function', function() { + expect(typeof Chart.helpers.scaleMerge).toBe('function'); + }); + }); }); describe('Version 2.7.3', function() { From 1d4b41a0573b3bd54ed2793ebd24db1578f503d8 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Tue, 29 Jan 2019 09:53:33 +0100 Subject: [PATCH 3/5] Prevent explicit first argument deep copy --- src/core/core.controller.js | 18 +++++++++++++---- test/specs/core.controller.tests.js | 30 +++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 5a17d1afb64..d73bb4e4934 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -35,8 +35,13 @@ defaults._set('global', { responsiveAnimationDuration: 0 }); -function mergeScaleConfig(/* objects ... */) { - return helpers.merge(helpers.clone(arguments[0]), [].slice.call(arguments, 1), { +/** + * Recursively merge the given config objects as the `scales` options by + * incorporating scale defaults in `xAxes` and `yAxes` array items, then + * returns a deep copy of the result, thus doesn't alter inputs. + */ +function mergeScaleConfig(/* config objects ... */) { + return helpers.merge({}, Array.prototype.slice.call(arguments), { merger: function(key, target, source, options) { if (key === 'xAxes' || key === 'yAxes') { var slen = source[key].length; @@ -70,8 +75,13 @@ function mergeScaleConfig(/* objects ... */) { }); } -function mergeConfig(/* objects ... */) { - return helpers.merge(helpers.clone(arguments[0]), [].slice.call(arguments, 1), { +/** + * Recursively merge the given config objects as the root options by handling + * default scale options for the `scales` and `scale` properties, then returns + * a deep copy of the result, thus doesn't alter inputs. + */ +function mergeConfig(/* config objects ... */) { + return helpers.merge({}, Array.prototype.slice.call(arguments), { merger: function(key, target, source, options) { var tval = target[key] || {}; var sval = source[key]; diff --git a/test/specs/core.controller.tests.js b/test/specs/core.controller.tests.js index 62c54b75ce1..819074393b1 100644 --- a/test/specs/core.controller.tests.js +++ b/test/specs/core.controller.tests.js @@ -271,6 +271,36 @@ describe('Chart', function() { _jasmineCheckE: 'e2' })); }); + + it('should not alter defaults when merging config', function() { + var chart = acquireChart({ + type: 'line', + options: { + _jasmineCheck: 42, + scales: { + xAxes: [{ + id: 'foo', + type: 'linear', + _jasmineCheck: 42, + }], + yAxes: [{ + id: 'bar', + type: 'category', + _jasmineCheck: 42, + }] + } + } + }); + + expect(chart.options._jasmineCheck).toBeDefined(); + expect(chart.scales.foo.options._jasmineCheck).toBeDefined(); + expect(chart.scales.bar.options._jasmineCheck).toBeDefined(); + + expect(Chart.defaults.line._jasmineCheck).not.toBeDefined(); + expect(Chart.defaults.global._jasmineCheck).not.toBeDefined(); + expect(Chart.scaleService.defaults.linear._jasmineCheck).not.toBeDefined(); + expect(Chart.scaleService.defaults.category._jasmineCheck).not.toBeDefined(); + }); }); describe('config.options.responsive: false', function() { From 445cf19c1cfdaca59ed59e47edaef4099e742c54 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Tue, 29 Jan 2019 11:45:24 +0100 Subject: [PATCH 4/5] [].slice instead of Array.prototype.slice --- src/core/core.controller.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index d73bb4e4934..231fa5cce64 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -41,7 +41,7 @@ defaults._set('global', { * returns a deep copy of the result, thus doesn't alter inputs. */ function mergeScaleConfig(/* config objects ... */) { - return helpers.merge({}, Array.prototype.slice.call(arguments), { + return helpers.merge({}, [].slice.call(arguments), { merger: function(key, target, source, options) { if (key === 'xAxes' || key === 'yAxes') { var slen = source[key].length; @@ -53,7 +53,7 @@ function mergeScaleConfig(/* config objects ... */) { for (i = 0; i < slen; ++i) { scale = source[key][i]; - type = helpers.valueOrDefault(scale.type, key === 'xAxes' ? 'category' : 'linear'); + type = valueOrDefault(scale.type, key === 'xAxes' ? 'category' : 'linear'); if (i >= target[key].length) { target[key].push({}); @@ -81,7 +81,7 @@ function mergeScaleConfig(/* config objects ... */) { * a deep copy of the result, thus doesn't alter inputs. */ function mergeConfig(/* config objects ... */) { - return helpers.merge({}, Array.prototype.slice.call(arguments), { + return helpers.merge({}, [].slice.call(arguments), { merger: function(key, target, source, options) { var tval = target[key] || {}; var sval = source[key]; From 104991be4e5a0a33ff64f20ffcce24538f3f7cb6 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Tue, 29 Jan 2019 16:26:11 +0100 Subject: [PATCH 5/5] Update comment per code review --- src/core/core.controller.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/core.controller.js b/src/core/core.controller.js index 231fa5cce64..1d85ff1ba8e 100644 --- a/src/core/core.controller.js +++ b/src/core/core.controller.js @@ -36,8 +36,8 @@ defaults._set('global', { }); /** - * Recursively merge the given config objects as the `scales` options by - * incorporating scale defaults in `xAxes` and `yAxes` array items, then + * Recursively merge the given config objects representing the `scales` option + * by incorporating scale defaults in `xAxes` and `yAxes` array items, then * returns a deep copy of the result, thus doesn't alter inputs. */ function mergeScaleConfig(/* config objects ... */) {