From ad4cf24a8ad32ec73902bc5a0ddeb88f2427c5ba Mon Sep 17 00:00:00 2001
From: Simon Brunel <simonbrunel@users.noreply.github.com>
Date: Sat, 24 Jun 2017 18:46:35 +0200
Subject: [PATCH] Rewrite the clone and merge helpers

The `clone` method now accepts any type of input but also recursively perform a deep copy of the array items. Rewrite the `configMerge` and `scaleMerge` helpers which now rely on a new generic and customizable `merge` method, that one accepts a target object in which multiple sources are deep copied. Note that the target (first argument) is not cloned and will be modified after calling `merge(target, sources)`. Add a `mergeIf` helper which merge the source properties only if they do not exist in the target object.
---
 src/core/core.helpers.js         | 111 ++++++++++----------------
 src/core/core.scaleService.js    |   2 +-
 src/helpers/helpers.core.js      | 104 ++++++++++++++++++++++++
 src/plugins/plugin.legend.js     |   2 +-
 src/plugins/plugin.title.js      |   2 +-
 test/specs/core.helpers.tests.js |  19 -----
 test/specs/helpers.core.tests.js | 133 +++++++++++++++++++++++++++++++
 7 files changed, 282 insertions(+), 91 deletions(-)

diff --git a/src/core/core.helpers.js b/src/core/core.helpers.js
index 08470fa7b47..e66d339d32b 100644
--- a/src/core/core.helpers.js
+++ b/src/core/core.helpers.js
@@ -8,19 +8,7 @@ module.exports = function(Chart) {
 	var helpers = Chart.helpers;
 
 	// -- Basic js utility methods
-	helpers.clone = function(obj) {
-		var objClone = {};
-		helpers.each(obj, function(value, key) {
-			if (helpers.isArray(value)) {
-				objClone[key] = value.slice(0);
-			} else if (typeof value === 'object' && value !== null) {
-				objClone[key] = helpers.clone(value);
-			} else {
-				objClone[key] = value;
-			}
-		});
-		return objClone;
-	};
+
 	helpers.extend = function(base) {
 		var setFn = function(value, key) {
 			base[key] = value;
@@ -30,75 +18,60 @@ module.exports = function(Chart) {
 		}
 		return base;
 	};
-	// Need a special merge function to chart configs since they are now grouped
-	helpers.configMerge = function(_base) {
-		var base = helpers.clone(_base);
-		helpers.each(Array.prototype.slice.call(arguments, 1), function(extension) {
-			helpers.each(extension, function(value, key) {
-				var baseHasProperty = base.hasOwnProperty(key);
-				var baseVal = baseHasProperty ? base[key] : {};
+
+	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
-					base[key] = helpers.scaleMerge(baseVal, value);
+					// 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
-					base[key] = helpers.configMerge(baseVal, Chart.scaleService.getScaleDefaults(value.type), value);
-				} else if (baseHasProperty
-						&& typeof baseVal === 'object'
-						&& !helpers.isArray(baseVal)
-						&& baseVal !== null
-						&& typeof value === 'object'
-						&& !helpers.isArray(value)) {
-					// If we are overwriting an object with an object, do a merge of the properties.
-					base[key] = helpers.configMerge(baseVal, value);
+					// used in polar area & radar charts since there is only one scale
+					target[key] = helpers.merge(tval, [Chart.scaleService.getScaleDefaults(sval.type), sval]);
 				} else {
-					// can just overwrite the value in this case
-					base[key] = value;
+					helpers._merger(key, target, source, options);
 				}
-			});
+			}
 		});
-
-		return base;
 	};
-	helpers.scaleMerge = function(_base, extension) {
-		var base = helpers.clone(_base);
-
-		helpers.each(extension, function(value, key) {
-			if (key === 'xAxes' || key === 'yAxes') {
-				// These properties are arrays of items
-				if (base.hasOwnProperty(key)) {
-					helpers.each(value, function(valueObj, index) {
-						var axisType = helpers.valueOrDefault(valueObj.type, key === 'xAxes' ? 'category' : 'linear');
-						var axisDefaults = Chart.scaleService.getScaleDefaults(axisType);
-						if (index >= base[key].length || !base[key][index].type) {
-							base[key].push(helpers.configMerge(axisDefaults, valueObj));
-						} else if (valueObj.type && valueObj.type !== base[key][index].type) {
-							// Type changed. Bring in the new defaults before we bring in valueObj so that valueObj can override the correct scale defaults
-							base[key][index] = helpers.configMerge(base[key][index], axisDefaults, valueObj);
+
+	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, defaults;
+
+					if (!target[key]) {
+						target[key] = [];
+					}
+
+					for (i = 0; i < slen; ++i) {
+						scale = source[key][i];
+						type = helpers.valueOrDefault(scale.type, key === 'xAxes'? 'category' : 'linear');
+						defaults = Chart.scaleService.getScaleDefaults(type);
+
+						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], [defaults, scale]);
 						} else {
-							// Type is the same
-							base[key][index] = helpers.configMerge(base[key][index], valueObj);
+							// scales type are the same
+							helpers.merge(target[key][i], scale);
 						}
-					});
+					}
 				} else {
-					base[key] = [];
-					helpers.each(value, function(valueObj) {
-						var axisType = helpers.valueOrDefault(valueObj.type, key === 'xAxes' ? 'category' : 'linear');
-						base[key].push(helpers.configMerge(Chart.scaleService.getScaleDefaults(axisType), valueObj));
-					});
+					helpers._merger(key, target, source, options);
 				}
-			} else if (base.hasOwnProperty(key) && typeof base[key] === 'object' && base[key] !== null && typeof value === 'object') {
-				// If we are overwriting an object with an object, do a merge of the properties.
-				base[key] = helpers.configMerge(base[key], value);
-
-			} else {
-				// can just overwrite the value in this case
-				base[key] = value;
 			}
 		});
-
-		return base;
 	};
 
 	helpers.where = function(collection, filterCallback) {
diff --git a/src/core/core.scaleService.js b/src/core/core.scaleService.js
index 92e8136508b..58eafc9cbb1 100644
--- a/src/core/core.scaleService.js
+++ b/src/core/core.scaleService.js
@@ -22,7 +22,7 @@ module.exports = function(Chart) {
 		},
 		getScaleDefaults: function(type) {
 			// Return the scale defaults merged with the global settings so that we always use the latest ones
-			return this.defaults.hasOwnProperty(type) ? helpers.scaleMerge(Chart.defaults.scale, this.defaults[type]) : {};
+			return this.defaults.hasOwnProperty(type) ? helpers.merge({}, [Chart.defaults.scale, this.defaults[type]]) : {};
 		},
 		updateScaleDefaults: function(type, additions) {
 			var defaults = this.defaults;
diff --git a/src/helpers/helpers.core.js b/src/helpers/helpers.core.js
index 269851fe90d..d0e4b14267c 100644
--- a/src/helpers/helpers.core.js
+++ b/src/helpers/helpers.core.js
@@ -147,6 +147,110 @@ module.exports = function(Chart) {
 			}
 
 			return true;
+		},
+
+		/**
+		 * Returns a deep copy of `source` without keeping references on objects and arrays.
+		 * @param {*} source - The value to clone.
+		 * @returns {*}
+		 */
+		clone: function(source) {
+			if (helpers.isArray(source)) {
+				return source.map(helpers.clone);
+			}
+
+			if (helpers.isObject(source)) {
+				var target = {};
+				var keys = Object.keys(source);
+				var klen = keys.length;
+				var k = 0;
+
+				for (; k<klen; ++k) {
+					target[keys[k]] = helpers.clone(source[keys[k]]);
+				}
+
+				return target;
+			}
+
+			return source;
+		},
+
+		/**
+		 * 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.
+		 * @private
+		 */
+		_merger: function(key, target, source, options) {
+			var tval = target[key];
+			var sval = source[key];
+
+			if (helpers.isObject(tval) && helpers.isObject(sval)) {
+				helpers.merge(tval, sval, options);
+			} else {
+				target[key] = helpers.clone(sval);
+			}
+		},
+
+		/**
+		 * Merges source[key] in target[key] only if target[key] is undefined.
+		 * @private
+		 */
+		_mergerIf: function(key, target, source) {
+			var tval = target[key];
+			var sval = source[key];
+
+			if (helpers.isObject(tval) && helpers.isObject(sval)) {
+				helpers.mergeIf(tval, sval);
+			} else if (!target.hasOwnProperty(key)) {
+				target[key] = helpers.clone(sval);
+			}
+		},
+
+		/**
+		 * Recursively deep copies `source` properties into `target` with the given `options`.
+		 * IMPORTANT: `target` is not cloned and will be updated with `source` properties.
+		 * @param {Object} target - The target object in which all sources are merged into.
+		 * @param {Object|Array(Object)} source - Object(s) to merge into `target`.
+		 * @param {Object} [options] - Merging options:
+		 * @param {Function} [options.merger] - The merge method (key, target, source, options)
+		 * @returns {Object} The `target` object.
+		 */
+		merge: function(target, source, options) {
+			var sources = helpers.isArray(source)? source : [source];
+			var ilen = sources.length;
+			var merge, i, keys, klen, k;
+
+			if (!helpers.isObject(target)) {
+				return target;
+			}
+
+			options = options || {};
+			merge = options.merger || helpers._merger;
+
+			for (i=0; i<ilen; ++i) {
+				source = sources[i];
+				if (!helpers.isObject(source)) {
+					continue;
+				}
+
+				keys = Object.keys(source);
+				for (k=0, klen = keys.length; k<klen; ++k) {
+					merge(keys[k], target, source, options);
+				}
+			}
+
+			return target;
+		},
+
+		/**
+		 * Recursively deep copies `source` properties into `target` *only* if not defined in target.
+		 * IMPORTANT: `target` is not cloned and will be updated with `source` properties.
+		 * @param {Object} target - The target object in which all sources are merged into.
+		 * @param {Object|Array(Object)} source - Object(s) to merge into `target`.
+		 * @returns {Object} The `target` object.
+		 */
+		mergeIf: function(target, source) {
+			return helpers.merge(target, source, {merger: helpers._mergerIf});
 		}
 	};
 
diff --git a/src/plugins/plugin.legend.js b/src/plugins/plugin.legend.js
index 341e828e0bd..0e3b1e66530 100644
--- a/src/plugins/plugin.legend.js
+++ b/src/plugins/plugin.legend.js
@@ -524,7 +524,7 @@ module.exports = function(Chart) {
 			var legend = chart.legend;
 
 			if (legendOpts) {
-				legendOpts = helpers.configMerge(Chart.defaults.global.legend, legendOpts);
+				helpers.mergeIf(legendOpts, Chart.defaults.global.legend);
 
 				if (legend) {
 					layout.configure(chart, legend, legendOpts);
diff --git a/src/plugins/plugin.title.js b/src/plugins/plugin.title.js
index e2371bd91c1..c8f815d763e 100644
--- a/src/plugins/plugin.title.js
+++ b/src/plugins/plugin.title.js
@@ -224,7 +224,7 @@ module.exports = function(Chart) {
 			var titleBlock = chart.titleBlock;
 
 			if (titleOpts) {
-				titleOpts = helpers.configMerge(Chart.defaults.global.title, titleOpts);
+				helpers.mergeIf(titleOpts, Chart.defaults.global.title);
 
 				if (titleBlock) {
 					layout.configure(chart, titleBlock, titleOpts);
diff --git a/test/specs/core.helpers.tests.js b/test/specs/core.helpers.tests.js
index c1d1e8710b8..d0182cc6c7c 100644
--- a/test/specs/core.helpers.tests.js
+++ b/test/specs/core.helpers.tests.js
@@ -6,25 +6,6 @@ describe('Core helper tests', function() {
 		helpers = window.Chart.helpers;
 	});
 
-	it('should clone an object', function() {
-		var testData = {
-			myProp1: 'abc',
-			myProp2: ['a', 'b'],
-			myProp3: {
-				myProp4: 5,
-				myProp5: [1, 2]
-			}
-		};
-
-		var clone = helpers.clone(testData);
-		expect(clone).toEqual(testData);
-		expect(clone).not.toBe(testData);
-
-		expect(clone.myProp2).not.toBe(testData.myProp2);
-		expect(clone.myProp3).not.toBe(testData.myProp3);
-		expect(clone.myProp3.myProp5).not.toBe(testData.myProp3.myProp5);
-	});
-
 	it('should extend an object', function() {
 		var original = {
 			myProp1: 'abc',
diff --git a/test/specs/helpers.core.tests.js b/test/specs/helpers.core.tests.js
index bd3c9631ae6..bc79057b3ad 100644
--- a/test/specs/helpers.core.tests.js
+++ b/test/specs/helpers.core.tests.js
@@ -236,4 +236,137 @@ describe('Chart.helpers.core', function() {
 			expect(helpers.arrayEquals([o0, o1, o2], [o0, o1, o2])).toBeTruthy();
 		});
 	});
+
+	describe('clone', function() {
+		it('should clone primitive values', function() {
+			expect(helpers.clone()).toBe(undefined);
+			expect(helpers.clone(null)).toBe(null);
+			expect(helpers.clone(true)).toBe(true);
+			expect(helpers.clone(42)).toBe(42);
+			expect(helpers.clone('foo')).toBe('foo');
+		});
+		it('should perform a deep copy of arrays', function() {
+			var o0 = {a: 42};
+			var o1 = {s: 's'};
+			var a0 = ['bar'];
+			var a1 = [a0, o0, 2];
+			var f0 = function() {};
+			var input = [a1, o1, f0, 42, 'foo'];
+			var output = helpers.clone(input);
+
+			expect(output).toEqual(input);
+			expect(output).not.toBe(input);
+			expect(output[0]).not.toBe(a1);
+			expect(output[0][0]).not.toBe(a0);
+			expect(output[1]).not.toBe(o1);
+		});
+		it('should perform a deep copy of objects', function() {
+			var a0 = ['bar'];
+			var a1 = [1, 2, 3];
+			var o0 = {a: a1, i: 42};
+			var f0 = function() {};
+			var input = {o: o0, a: a0, f: f0, s: 'foo', i: 42};
+			var output = helpers.clone(input);
+
+			expect(output).toEqual(input);
+			expect(output).not.toBe(input);
+			expect(output.o).not.toBe(o0);
+			expect(output.o.a).not.toBe(a1);
+			expect(output.a).not.toBe(a0);
+		});
+	});
+
+	describe('merge', function() {
+		it('should update target and return it', function() {
+			var target = {a: 1};
+			var result = helpers.merge(target, {a: 2, b: 'foo'});
+			expect(target).toEqual({a: 2, b: 'foo'});
+			expect(target).toBe(result);
+		});
+		it('should return target if not an object', function() {
+			expect(helpers.merge(undefined, {a: 42})).toEqual(undefined);
+			expect(helpers.merge(null, {a: 42})).toEqual(null);
+			expect(helpers.merge('foo', {a: 42})).toEqual('foo');
+			expect(helpers.merge(['foo', 'bar'], {a: 42})).toEqual(['foo', 'bar']);
+		});
+		it('should ignore sources which are not objects', function() {
+			expect(helpers.merge({a: 42})).toEqual({a: 42});
+			expect(helpers.merge({a: 42}, null)).toEqual({a: 42});
+			expect(helpers.merge({a: 42}, 42)).toEqual({a: 42});
+		});
+		it('should recursively overwrite target with source properties', function() {
+			expect(helpers.merge({a: {b: 1}}, {a: {c: 2}})).toEqual({a: {b: 1, c: 2}});
+			expect(helpers.merge({a: {b: 1}}, {a: {b: 2}})).toEqual({a: {b: 2}});
+			expect(helpers.merge({a: [1, 2]}, {a: [3, 4]})).toEqual({a: [3, 4]});
+			expect(helpers.merge({a: 42}, {a: {b: 0}})).toEqual({a: {b: 0}});
+			expect(helpers.merge({a: 42}, {a: null})).toEqual({a: null});
+			expect(helpers.merge({a: 42}, {a: undefined})).toEqual({a: undefined});
+		});
+		it('should merge multiple sources in the correct order', function() {
+			var t0 = {a: {b: 1, c: [1, 2]}};
+			var s0 = {a: {d: 3}, e: {f: 4}};
+			var s1 = {a: {b: 5}};
+			var s2 = {a: {c: [6, 7]}, e: 'foo'};
+
+			expect(helpers.merge(t0, [s0, s1, s2])).toEqual({a: {b: 5, c: [6, 7], d: 3}, e: 'foo'});
+		});
+		it('should deep copy merged values from sources', function() {
+			var a0 = ['foo'];
+			var a1 = [1, 2, 3];
+			var o0 = {a: a1, i: 42};
+			var output = helpers.merge({}, {a: a0, o: o0});
+
+			expect(output).toEqual({a: a0, o: o0});
+			expect(output.a).not.toBe(a0);
+			expect(output.o).not.toBe(o0);
+			expect(output.o.a).not.toBe(a1);
+		});
+	});
+
+	describe('mergeIf', function() {
+		it('should update target and return it', function() {
+			var target = {a: 1};
+			var result = helpers.mergeIf(target, {a: 2, b: 'foo'});
+			expect(target).toEqual({a: 1, b: 'foo'});
+			expect(target).toBe(result);
+		});
+		it('should return target if not an object', function() {
+			expect(helpers.mergeIf(undefined, {a: 42})).toEqual(undefined);
+			expect(helpers.mergeIf(null, {a: 42})).toEqual(null);
+			expect(helpers.mergeIf('foo', {a: 42})).toEqual('foo');
+			expect(helpers.mergeIf(['foo', 'bar'], {a: 42})).toEqual(['foo', 'bar']);
+		});
+		it('should ignore sources which are not objects', function() {
+			expect(helpers.mergeIf({a: 42})).toEqual({a: 42});
+			expect(helpers.mergeIf({a: 42}, null)).toEqual({a: 42});
+			expect(helpers.mergeIf({a: 42}, 42)).toEqual({a: 42});
+		});
+		it('should recursively copy source properties in target only if they do not exist in target', function() {
+			expect(helpers.mergeIf({a: {b: 1}}, {a: {c: 2}})).toEqual({a: {b: 1, c: 2}});
+			expect(helpers.mergeIf({a: {b: 1}}, {a: {b: 2}})).toEqual({a: {b: 1}});
+			expect(helpers.mergeIf({a: [1, 2]}, {a: [3, 4]})).toEqual({a: [1, 2]});
+			expect(helpers.mergeIf({a: 0}, {a: {b: 2}})).toEqual({a: 0});
+			expect(helpers.mergeIf({a: null}, {a: 42})).toEqual({a: null});
+			expect(helpers.mergeIf({a: undefined}, {a: 42})).toEqual({a: undefined});
+		});
+		it('should merge multiple sources in the correct order', function() {
+			var t0 = {a: {b: 1, c: [1, 2]}};
+			var s0 = {a: {d: 3}, e: {f: 4}};
+			var s1 = {a: {b: 5}};
+			var s2 = {a: {c: [6, 7]}, e: 'foo'};
+
+			expect(helpers.mergeIf(t0, [s0, s1, s2])).toEqual({a: {b: 1, c: [1, 2], d: 3}, e: {f: 4}});
+		});
+		it('should deep copy merged values from sources', function() {
+			var a0 = ['foo'];
+			var a1 = [1, 2, 3];
+			var o0 = {a: a1, i: 42};
+			var output = helpers.mergeIf({}, {a: a0, o: o0});
+
+			expect(output).toEqual({a: a0, o: o0});
+			expect(output.a).not.toBe(a0);
+			expect(output.o).not.toBe(o0);
+			expect(output.o.a).not.toBe(a1);
+		});
+	});
 });