Changed the anonymous define into a named define

[csupnig]

Changed the anonymous define into a named define so the script can still be included via script tag when using requirejs.

[mmcc]

Thanks for the PR! Sorry if I'm missing something obvious here, but would you mind giving an example of what exactly this fixes?

[csupnig]

Hi!
Thanks for the response. I created a simple example in this jsfiddle: http://jsfiddle.net/NssGv/21/ . You will see, that anonymous defines will break the script if you use AMD and include the script directly (without loading it over require). I know that this is only an edge case, since you would almost always load it over require if you use AMD but I came across this problem, when I was including the script from a location that would not let me use require to load it.

I hope that my example clarifies the intent of my pull request. Wether the module's name should be videojs or video, should better be decided by you.

[heff]

I actually don't have enough experience with AMD to confirm this. @jnwng, can you confirm this change at least doesn't break your AMD setup?

[jnwng]

i think underscore.js has a good rationale for naming versus not naming their module here, quoted below:

  // AMD registration happens at the end for compatibility with AMD loaders
  // that may not enforce next-turn semantics on modules. Even though general
  // practice for AMD registration is to be anonymous, underscore registers
  // as a named module because, like jQuery, it is a base library that is
  // popular enough to be bundled in a third party lib, but not be part of
  // an AMD load request. Those cases could generate an error when an
  // anonymous define() is called outside of a loader request.
  if (typeof define === 'function' && define.amd) {
    define('underscore', [], function() {
      return _;
    });
  }

to put it simply, since so many third-party libraries refer to an explicit need for underscore or jquery, its simply easier to name the module out of the box.

@csupnig's case is very much a corner-case, and i think there are quite a few more well-iterated arguments against naming modules (this and this), both of which lead me to advise against naming the module.

this all being said, we explicitly set all of our modules to be anonymous because we have various legacy pieces working at the same time. we avoid name collisions all together through the use of anonymous modules, and we explicitly remove the named define calls in all of our external third-party libraries, so the decision made here wouldn't affect us (we would remove the name in our fork).

[heff]

That's good feedback, thanks @jnwng.

Is there no way we can determine the context that it's being loaded in? i.e. skip the define if we know it's not being used with AMD? Or what about just a try/catch?

[jnwng]

i didn't know this off the top of my head; i dug a little and found a few things:

1. obligatory stackoverflow question/answers
2. obligatory follow up on require.js google group
3. obligatory issue on jrburke/require.js

the short of it is, no you cannot distinguish whether or not a module is being loaded via AMD or being included via a script tag.

you can however determine whether or not its being loaded through AMD via Require.js. Require.js exposes a function called specified that will let you determine whether or not this module got included via require.js, allowing you to then define it anonymously. seems oddly specific to implement in the general case, as it does not follow an AMD API and therefore may have unintended consequences on AMD loaders that are not require.js

as for try/catch, it looks like require.js opts to catch errors internally to stop the loading of things, so a traditional try/catch block surrounding the define call won't be sufficient. you're allowed to specify a require.onError function to capture errors (they are conveniently namespaced), but again, this is require.js specific and not something that would make sense to put here.

[heff]

I'm not really sure what to do with this one. It sounds like there's no great option here. I tried adding a try/catch and like @jnwng said, no luck. Using the onError might be ok but it's not an event listener, it's an actual stored callback, meaning we might overwrite someone else's onError function.

Any other opinions?

[heff]

@csupnig, what if we checked for a var like videojsSkipDefine, that if you set it to true before loading the lib, it wouldn't try to define it. Would that help your specific use case?

[csupnig]

I think it would help in my specific use case, but it would be a very wonky solution.
Maybe it is such an edge case that it should not be addressed at all.

[heff]

I agree it's wonky... I think what we'll do for now is close the issue and wait to see what other interest we get in it. Thanks again for putting this together though.

One last thought I had was if there was a workaround (outside of video.js) where you modify define() so it catches this issue. Something like:

origDefine = define;
define = function(){
   // do something to detect issue and fix
   origDefine.apply(this, arguments);
};

Also super wonky, but maybe it'd help anyone else that runs into this issue...