Don't mutate the source

src/js/utils/merge-options.js

@@ -11,38 +11,50 @@ function isPlain(obj) {
 }
 
 /**
- * Merge two options objects, recursively merging **only* * plain object
- * properties.  Previously `deepMerge`.
+ * Merge customizer. video.js simply overwrites non-simple objects
+ * (like arrays) instead of attempting to overlay them.
+ * @see https://lodash.com/docs#merge
+ */
+const customizer = function(destination, source) {
+  // If we're not working with a plain object, copy the value as is
+  // If source is an array, for instance, it will replace destination
+  if (!isPlain(source)) {
+    return source;
+  }
+
+  // If the new value is a plain object but the first object value is not
+  // we need to create a new object for the first object to merge with.
+  // This makes it consistent with how merge() works by default
+  // and also protects from later changes the to first object affecting
+  // the second object's values.
+  if (!isPlain(destination)) {
+    return mergeOptions({}, source);
+  }
+};
+
+/**
+ * Merge one or more options objects, recursively merging **only**
+ * plain object properties.  Previously `deepMerge`.
  *
- * @param  {Object} object    The destination object
- * @param  {...Object} source One or more objects to merge into the first
- * @returns {Object}          The updated first object
+ * @param  {...Object} source One or more objects to merge
+ * @returns {Object}          a new object that is the union of all
+ * provided objects
  * @function mergeOptions
  */
-export default function mergeOptions(object={}) {
-
-  // Allow for infinite additional object args to merge
-  Array.prototype.slice.call(arguments, 1).forEach(function(source){
-
-    // Recursively merge only plain objects
-    // All other values will be directly copied
-    merge(object, source, function(a, b) {
-
-      // If we're not working with a plain object, copy the value as is
-      if (!isPlain(b)) {
-        return b;
-      }
-
-      // If the new value is a plain object but the first object value is not
-      // we need to create a new object for the first object to merge with.
-      // This makes it consistent with how merge() works by default
-      // and also protects from later changes the to first object affecting
-      // the second object's values.
-      if (!isPlain(a)) {
-        return mergeOptions({}, b);
-      }
-    });
-  });
-
-  return object;
+export default function mergeOptions() {
+  // contruct the call dynamically to handle the variable number of
+  // objects to merge
+  let args = Array.prototype.slice.call(arguments);
+
+  // unshift an empty object into the front of the call as the target
+  // of the merge
+  args.unshift({});
+
+  // customize conflict resolution to match our historical merge behavior
+  args.push(customizer);
+
+  merge.apply(null, args);
+
+  // return the mutated result object
+  return args[0];
 }

src/js/video.js

@@ -142,7 +142,7 @@ videojs.options = createDeprecationProxy(globalOptions, {
  * @method setGlobalOptions
  */
 videojs.setGlobalOptions = function(newOptions) {
-  return mergeOptions(globalOptions, newOptions);
+  return merge(globalOptions, newOptions);
 };
 
 /**
@@ -264,7 +264,7 @@ videojs.TOUCH_ENABLED = browser.TOUCH_ENABLED;
 videojs.extends = extendsFn;
 
 /**
- * Merge two options objects recursively
+ * Merge options objects recursively
  * Performs a deep merge like lodash.merge but **only merges plain objects**
  * (not arrays, elements, anything else)
  * Other values will be copied directly from the second object.
@@ -288,9 +288,7 @@ videojs.extends = extendsFn;
  *     // result.bar.b = [4,5,6];
  * ```
  *
- * @param {Object} The options object whose values will be overriden
- * @param {Object} The options object with values to override the first
- * @param {Object} Any number of additional options objects
+ * @param  {...Object} source One or more objects to merge
  *
  * @return {Object} a new object with the merged values
  * @mixes videojs

test/unit/player.test.js

@@ -142,7 +142,7 @@ test('should asynchronously fire error events during source selection', function
       { 'src': 'http://vjs.zencdn.net/v/oceans.mp4', 'type': 'video/mp4' }
     ]
   });
-  ok(player.options_['techOrder'][0] === 'foo', 'Foo listed as the only tech');
+  equal(player.options_['techOrder'][0], 'foo', 'Foo listed as the only tech');
 
   player.on('error', function(e) {
     ok(player.error().code === 4, 'Source could not be played error thrown');

test/unit/utils/merge-options.test.js

@@ -27,3 +27,16 @@ test('should merge options objects', function(){
     d: true
   }, 'options objects merged correctly');
 });
+
+test('should not mutate the first argument', function(){
+  let obj = {
+    prop: {
+      a: 'a'
+    }
+  };
+  let merged = mergeOptions(obj, { prop: { b: 'b' } });
+
+  ok(obj !== merged, 'did not return the original');
+  deepEqual(merged, { prop: { a: 'a', b: 'b' } }, 'merged the sub object');
+  ok(!('b' in obj.prop), 'did not mutate the original');
+});

test/unit/video.test.js

@@ -59,6 +59,15 @@ test('should add the value to the languages object with lower case lang code', f
   deepEqual(result, globalOptions['languages'][code.toLowerCase()], 'should also match');
 });
 
+test('should allow setting global options', function() {
+  videojs.setGlobalOptions({
+    nonsense: true
+  });
+
+  equal(videojs.getGlobalOptions().nonsense, true, 'set the new option');
+  deepEqual(videojs.options, videojs.getGlobalOptions(), 'updated the options property');
+});
+
 test('should expose plugin registry function', function() {
   var pluginName, pluginFunction, player;