Skip to content

Commit

Permalink
Setting global options should perform a mutating merge
Browse files Browse the repository at this point in the history
The global options setter was returning a new object after mergeOptions() was converted to be non-mutating. Use lodash's merge() so that the setting changes persist. Add comments to mergeOptions().
  • Loading branch information
dmlap committed Aug 4, 2015
1 parent 0d90544 commit 1ca358b
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 9 deletions.
22 changes: 18 additions & 4 deletions src/js/utils/merge-options.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,18 @@
*/
import merge from 'lodash-compat/object/merge';

const isPlain = function(obj) {
function isPlain(obj) {
return !!obj
&& typeof obj === 'object'
&& obj.toString() === '[object Object]'
&& obj.constructor === Object;
};
}

/**
* 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
Expand All @@ -31,16 +36,25 @@ const customizer = function(destination, 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
* @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() {
// 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];
}
8 changes: 3 additions & 5 deletions src/js/video.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ videojs.options = createDeprecationProxy(globalOptions, {
* @method setGlobalOptions
*/
videojs.setGlobalOptions = function(newOptions) {
return mergeOptions(globalOptions, newOptions);
return merge(globalOptions, newOptions);
};

/**
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
9 changes: 9 additions & 0 deletions test/unit/video.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit 1ca358b

Please sign in to comment.