Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added vjs.extend method #1909

Closed
wants to merge 3 commits into from
Closed

Added vjs.extend method #1909

wants to merge 3 commits into from

Conversation

carpasse
Copy link
Contributor

Added extend method to be able to easily merge options when you create a plugin.

@carpasse carpasse changed the title Added vjs.extend method Added vjs.extend method. Fixes #1856 Feb 28, 2015
@carpasse carpasse changed the title Added vjs.extend method. Fixes #1856 Added vjs.extend method Feb 28, 2015
@heff heff mentioned this pull request Mar 16, 2015
@heff
Copy link
Member

heff commented Mar 16, 2015

@carpasse, FYI, there's a related conversation in #1232 if you have any thoughts.

@heff
Copy link
Member

heff commented Mar 30, 2015

Just commenting on the status of this. We're working on a major refactor for 5.0. As part of that we're going to be reorganizing utils, and pulling in lodash to replace a lot of the custom functions. This may or may not be affected by that, but we need to wait to see how that plays out first. #1232 is where discussions around that will happen.

@heff heff added this to the v5.0.0 milestone May 11, 2015
@mmcc
Copy link
Member

mmcc commented May 11, 2015

@carpasse So I think we've landed on pulling in lodash for whatever utilities we can, so this is a pretty prime usage. I think step 1 is to make use of your extensive tests to ensure that it's a drop in replacement (should be), and if all is well have you start the process of importing / exporting lodash methods.

@heff is currently working on splitting apart the lib stuff into different files, so you guys might want to coordinate on timing of this.

@gkatsev
Copy link
Member

gkatsev commented May 11, 2015

Actually, for extend specifically, we should use Object.assign. There's a shim for it available on npm: https://www.npmjs.com/package/object.assign
It allows local usage of object.assign without shimming it globally:

var assign = Object.assign || require('object.assign');

I think this is the best course of action, if possible. lodash has a lot of useful stuff but if the language has it (or is getting it), we should try and use it instead.

@heff
Copy link
Member

heff commented May 27, 2015

Ok, so as part of the 5.0 work we exported videojs.mergeOptions, and we'll be exporting videojs.assign also. @carpasse I think that covers everything here, but if it doesn't let me know and we'll reopen. I'm just trying to move quickly through the remaining 5.0 tasks so we can get it released.

@heff heff closed this May 27, 2015
@carpasse
Copy link
Contributor Author

@heff I thing it looks good. Thanks for the efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants