Don't mutate the source #2418

dmlap · 2015-07-30T20:41:49Z

mergeOptions did not mutate its first argument in video.js 4.x. Restore this behavior.

misteroneill · 2015-07-30T20:45:37Z

src/js/utils/merge-options.js

@@ -2,47 +2,46 @@
 * @file merge-options.js
 */
 import merge from 'lodash-compat/object/merge';
+import isArray from 'lodash-compat/lang/isArray';


This appears to be extraneous.

Good catch.

pam · 2015-07-30T20:48:06Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 86ddb70
Build details: https://travis-ci.org/pam/video.js/builds/73451274

(Please note that this is a fully automated comment.)

mergeOptions did not mutate its first argument in video.js 4.x. Restore this behavior.

isArray isn't used in this module.

pam · 2015-07-31T19:26:47Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 0d90544
Build details: https://travis-ci.org/pam/video.js/builds/73609517

(Please note that this is a fully automated comment.)

heff · 2015-08-03T17:33:34Z

src/js/utils/merge-options.js

@@ -3,46 +3,44 @@
 */
 import merge from 'lodash-compat/object/merge';

-function isPlain(obj) {
+const isPlain = function(obj) {


Why change this?

We had been using var f = function() {} bindings most everywhere else. I was just reverting this to what I thought was the current recommendation.

Ok no problem, but if we're going by the Airbnb guid then we should move towards declarations.
https://github.com/airbnb/javascript#7.1
I've been using them for the smaller module functions at least.
https://github.com/videojs/video.js/blob/master/src/js/utils/format-time.js

I'm still pro-Airbnb-guide. I'll update this.

heff · 2015-08-03T17:54:30Z

This was on purpose, to match how lodash merge works. See #2182 point 3.

As is, it'll break this amazing one-liner: https://github.com/videojs/video.js/blob/master/src/js/video.js#L379

misteroneill · 2015-08-03T19:24:51Z

This is another place where documentation would help - namely, that people should do mergeOptions({}, a, b) where they previously did mergeOptions(a, b) to achieve the same behavior.

mergeOptions is used everywhere in plugins and a bunch of them broke because it was now mutating the first argument.

dmlap · 2015-08-03T19:40:12Z

@heff that one liner is pretty cool but breaking all the existing users doesn't seem worth it to me. This is probably the main utility function that plugins were using in 4.x

heff · 2015-08-03T21:50:32Z

src/js/utils/merge-options.js

-  return object;
+export default function mergeOptions() {
+  let args = Array.prototype.slice.call(arguments);
+  args.unshift({});


Could we add some comments to these lines? Takes a bit to grok as is.

heff · 2015-08-03T21:55:11Z

Ok if that's the case then I'm ok with reverting that change. Would love some code comments and we'll need to fix the addLanguage piece, but otherwise looks good to me.

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().

dmlap · 2015-08-04T15:39:10Z

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({


The fact this does a merge and not an overwrite strikes me as really odd but that's a separate discussion.

I agree it feels a little weird, but if it did an overwrite you'd have to include every option every time instead of being able to just set one or a few.

The converse of that is you can no longer remove an option that has been set.

dmlap · 2015-08-04T15:40:42Z

@heff updated. The language stuff actually works correctly because it's using lodash's mutating merge() instead of mergeOptions(). I grepped for other uses of merge() or mergeOptions() as a sanity check and found a problem with setGlobalOptions. That is fixed in the update commit.

pam · 2015-08-04T15:44:35Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 1ca358b
Build details: https://travis-ci.org/pam/video.js/builds/74085343

(Please note that this is a fully automated comment.)

heff · 2015-08-04T16:50:06Z

I don't think we can use just merge for the global options. That will make the options merging (e.g. not merging arrays) different from other options merging.

misteroneill · 2015-08-05T21:18:09Z

Yeah, it looks like setGlobalOptions should use mergeOptions (deep) instead of merge (shallow).

I did a quick sweep for uses of mergeOptions on this branch as well.

With mergeOptions no longer mutating the first argument, there are a few calls with an empty {} that should no longer be necessary:

Other things I noticed:

component.js:57-61: In the second call, this.options_ gets set again; so the first line seems pointless.

heff · 2015-08-06T00:04:40Z

Yeah, all of those would just be using it as a deepCopy now (but only copying plain objects). Leaving the first object in might make it more clear what's happening since the function name doesn't match what it's doing, but I could go either way. Or we could make another function just for copying, that shares some logic with mergeOptions.

The last example would be redundant now. The important thing is that the instance.options_ no longer points to the same object as instance.prototype.options_ so the defaults are protected. So maybe keep the comment of the first line since it's not super obvious that's what's happening.

dmlap · 2015-08-10T16:47:38Z

I think the transition to a getter/setter model for global options is creating more problems than it is solving. If we go back to using videojs.options, we don't have to worry about whether the merge is deep or shallow, or how array conflicts are handled. I can continue trying to massage this thing into place but it seems worth considering whether new semantics for global options is worth it. @videojs/core-committers thoughts?

heff · 2015-08-10T16:54:38Z

I think if we switched to mergeOptions for global options this could be merged in now, if the goal is to get this merged in quickly. I could also be ok with switching back to a normal object for options, since the setting process is not as intuitive with a function.

gkatsev · 2015-08-10T17:48:40Z

Also, we could still have set and get globalOptions while still having videojs.options.

imbcmdth · 2015-08-11T15:32:00Z

Proposed changes to @dmlap PR above: a9b4221495fc874e22c03d8f6fb778e3f8ee2542\

There is value in a mutating version of merge that has the same semantics as the non-mutating version (namely array handling.)

The secondary issue of not being able to remove properties from global options could be side-stepped by simply setting properties you don't want around any more to null (unfortunately, undefined has a special meaning in lodash's merge). While less than ideal, it would only be a problem if you use in to determine if a property exists without ever checking it's value directly. A quick search and I didn't see any instances of that particular use-case for global options.

heff · 2015-08-12T22:08:29Z

Can this be closed now that #2461 is in?

misteroneill reviewed Jul 30, 2015
View reviewed changes

misteroneill mentioned this pull request Jul 30, 2015

VideoJS 5 Compatibility videojs/videojs-errors#28

Closed

dmlap added 2 commits July 31, 2015 15:17

Don't mutate the source

3018fcf

mergeOptions did not mutate its first argument in video.js 4.x. Restore this behavior.

Remove extra import

0d90544

isArray isn't used in this module.

dmlap force-pushed the non-mutating-merge branch from 86ddb70 to 0d90544 Compare July 31, 2015 19:19

misteroneill mentioned this pull request Jul 31, 2015

Updates for VideoJS 5.0 videojs/videojs-playlist-ui#7

Closed

heff reviewed Aug 3, 2015
View reviewed changes

Setting global options should perform a mutating merge

1ca358b

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().

dmlap reviewed Aug 4, 2015
View reviewed changes

dmlap mentioned this pull request Aug 11, 2015

Make global options a simple property #2461

Closed

dmlap closed this Aug 13, 2015

dmlap deleted the non-mutating-merge branch August 13, 2015 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't mutate the source #2418

Don't mutate the source #2418

dmlap commented Jul 30, 2015

misteroneill Jul 30, 2015

dmlap Jul 31, 2015

pam commented Jul 30, 2015

pam commented Jul 31, 2015

heff Aug 3, 2015

dmlap Aug 3, 2015

heff Aug 3, 2015

dmlap Aug 3, 2015

heff commented Aug 3, 2015

misteroneill commented Aug 3, 2015

dmlap commented Aug 3, 2015

heff Aug 3, 2015

dmlap Aug 3, 2015

heff commented Aug 3, 2015

dmlap Aug 4, 2015

heff Aug 4, 2015

dmlap Aug 10, 2015

dmlap commented Aug 4, 2015

pam commented Aug 4, 2015

heff commented Aug 4, 2015

misteroneill commented Aug 5, 2015

heff commented Aug 6, 2015

dmlap commented Aug 10, 2015

heff commented Aug 10, 2015

gkatsev commented Aug 10, 2015

imbcmdth commented Aug 11, 2015

heff commented Aug 12, 2015

Don't mutate the source #2418

Don't mutate the source #2418

Conversation

dmlap commented Jul 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Jul 30, 2015

pam commented Jul 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Aug 3, 2015

misteroneill commented Aug 3, 2015

dmlap commented Aug 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Aug 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlap commented Aug 4, 2015

pam commented Aug 4, 2015

heff commented Aug 4, 2015

misteroneill commented Aug 5, 2015

heff commented Aug 6, 2015

dmlap commented Aug 10, 2015

heff commented Aug 10, 2015

gkatsev commented Aug 10, 2015

imbcmdth commented Aug 11, 2015

heff commented Aug 12, 2015