Export video.js as a named AMD module #1844

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@dmlap
Member

dmlap commented Feb 3, 2015

If video.js is packaged in a file with another AMD module, tools like RequireJS may not be able to resolve a name for anonymous module definitions. Using the named module syntax should allow video.js to be packaged in whatever format is convenient and still work with AMD loaders. For #1843

@@ -170,7 +170,7 @@ vjs.players = {};
* compiler compatible, so string keys are used.
*/
if (typeof define === 'function' && define['amd']) {
- define([], function(){ return videojs; });
+ define('videojs', [], function(){ return videojs; });

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2015

Member

I wonder if it's better to export it as video.js to be consistent with npm, but otherwise, LGTM.

@gkatsev

gkatsev Feb 3, 2015

Member

I wonder if it's better to export it as video.js to be consistent with npm, but otherwise, LGTM.

This comment has been minimized.

@tomjohnson916

tomjohnson916 Feb 3, 2015

Contributor

I would argue that videojs is better than video.js since most of the plugins start w videojs-X. LGTM also.

@tomjohnson916

tomjohnson916 Feb 3, 2015

Contributor

I would argue that videojs is better than video.js since most of the plugins start w videojs-X. LGTM also.

This comment has been minimized.

@dmlap

dmlap Feb 3, 2015

Member

I also went through great emotional turmoil thinking about this one. I landed on videojs because that's how it's currently exported on window.

@dmlap

dmlap Feb 3, 2015

Member

I also went through great emotional turmoil thinking about this one. I landed on videojs because that's how it's currently exported on window.

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2015

Member

Yeah, not stating whether videojs is better or worse than video.js, just that consistency is better than no consistency. Changing the npm name is a lot harder than how we will be exporting it for AMD here.

@gkatsev

gkatsev Feb 3, 2015

Member

Yeah, not stating whether videojs is better or worse than video.js, just that consistency is better than no consistency. Changing the npm name is a lot harder than how we will be exporting it for AMD here.

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2015

Member

great emotional turmoil

feels

@gkatsev

gkatsev Feb 3, 2015

Member

great emotional turmoil

feels

This comment has been minimized.

@gkatsev

gkatsev Feb 3, 2015

Member

(and yeah, good to go, no strong objections, here).

@gkatsev

gkatsev Feb 3, 2015

Member

(and yeah, good to go, no strong objections, here).

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 3, 2015

Member

Have you seen the previous PR on this topic? Not positive if it's relevant but I know this topic has come up before.

Member

heff commented Feb 3, 2015

Have you seen the previous PR on this topic? Not positive if it's relevant but I know this topic has come up before.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 3, 2015

Member

The error we were seeing was exactly related to the comment from underscore.

Member

gkatsev commented Feb 3, 2015

The error we were seeing was exactly related to the comment from underscore.

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Feb 3, 2015

Member

@heff no, I hadn't seen that PR before. It is the same issue and I don't see any alternatives other than stripping the UMD stuff entirely. This seems like the lesser of two evils to me.

Member

dmlap commented Feb 3, 2015

@heff no, I hadn't seen that PR before. It is the same issue and I don't see any alternatives other than stripping the UMD stuff entirely. This seems like the lesser of two evils to me.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 3, 2015

Member

I haven't caught up on the details, but you're saying DO move forward with this PR, right?

@jnwng head up since I think this affects you

Member

heff commented Feb 3, 2015

I haven't caught up on the details, but you're saying DO move forward with this PR, right?

@jnwng head up since I think this affects you

@dmlap

This comment has been minimized.

Show comment
Hide comment
@dmlap

dmlap Feb 3, 2015

Member

@heff yes, I would say move forward with this PR.

Member

dmlap commented Feb 3, 2015

@heff yes, I would say move forward with this PR.

@jnwng

This comment has been minimized.

Show comment
Hide comment
@jnwng

jnwng Feb 3, 2015

Member

@heff thanks for the heads up, y'all should move forward with this

Member

jnwng commented Feb 3, 2015

@heff thanks for the heads up, y'all should move forward with this

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Feb 3, 2015

Member

Cool, sounds like we have approval and enough reviews. @dmlap, npm install -g contrib && contrib feature accept.

Member

heff commented Feb 3, 2015

Cool, sounds like we have approval and enough reviews. @dmlap, npm install -g contrib && contrib feature accept.

@dmlap dmlap closed this in 42fbd4c Feb 5, 2015

@dmlap dmlap deleted the dmlap:named-amd-module branch Feb 5, 2015

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