ded / morpheus

A Brilliant Animator
504 stars 57 forks source link

Morpheus errors when run as an AMD module with RequireJs #19

Closed Integralist closed 11 years ago

Integralist commented 12 years ago

Just to be clear first that Morpheus runs fine with RequireJs (by @jrburke) before running a build script.

But after running a build script (e.g. when Morpheus is inserted as a module into a single file along with other modules) the script errors when you try to execute the morpheus function. It simply states that: morpheus is not a function?

I managed to get this working though by changing Morpheus' define method to match that of when.js (by @briancavalier).

So for example I replaced...

!function (name, definition) {
  if (typeof define == 'function') define(definition)
  else if (typeof module != 'undefined') module.exports = definition()
  else this[name] = definition()
}('morpheus', function () {

    // MORPHEUS CODE HERE

})

...with this...

(function(define) {
define('morpheus',[], function() {

    // MORPHEUS CODE HERE

}); // define
})(typeof define != 'undefined'
    // use define for AMD if available
    ? define
    // If no define, look for module to export as a CommonJS module.
    // If no define or module, attach to current context.
    : typeof module != 'undefined'
    ? function(deps, factory) { module.exports = factory(); }
    : function(deps, factory) { this.when = factory(); }
);

Just thought you might want to look at changing this so it's compatible. I would have done a fork/pull request but wasn't sure if I needed to do any unit tests (as it seems the majority of repo admins request you do before submitting). So thought it easier just to flag up the issue in case you were interested.

Kind regards, Mark

briancavalier commented 12 years ago

Hey @integralist, thanks for the mention!

I've been trying out a couple variations on that UMD boilerplate as well, so I figured I'd pass them along in case you want to try them out to see if they also solve the problem. They can be a bit smaller, but the caveat is that I haven't tested them in all browsers (read: older IE) yet.

This first one is very compact if you don't need a browser global fallback. It works with AMD and Node with the caveats that it doesn't strictly follow the suggested AMD check (typeof define == 'function' && define.amd), but rather just relies on try/catch.

(function(factory){try{define(factory);}catch(e){module.exports = factory();}})(function(){

    // MORPHEUS CODE HERE

});

If you need the browser global fallback, it's easy to add:

//(function(factory){ try { define(factory); } catch(e) { try {  module.exports = factory(); } catch(e) { this.morpheus = factory(); }}})(function() {

    // MORPHEUS CODE HERE

});

Cheers!

jrburke commented 12 years ago

The issue is that the requirejs optimizer uses an AST to find define() calls that look like AMD define calls. Since define(definition) is a bit too generic it skips it and puts in an empty module for the 'morpheus' name.

Two possible easy fixes:

1) Use a function literal:

    if (typeof define == 'function') define(function(){return definition()});

2) Or just pass name to the function:

   if (typeof define == 'function') define(name, definition)

I have become more convinced that using the anonymous signature with the function literal is better, as it allows users of the code to use another name than 'morpheus', but I can appreciate it ends up with more bytes in the file. In any case, either one will work.

briancavalier commented 12 years ago

Hey James, any thoughts about the use of try/catch vs. typeof define == 'function' ? Try/catch is slightly smaller, and it feels like trying to call define() and catching a failure is essentially the same check as typeof define == 'function' (unless, of course, you add && define.amd to the check!)

jrburke commented 12 years ago

@briancavalier I do not like using try/catch for things that can be capability-detected, but instead reserve try/catch for errors. It is also a bit more expensive for the JS VM to do, although you probably will not be able to measure the difference.

briancavalier commented 12 years ago

In this case it doesn't bother me, since it's just boilerplate. In some languages/VMs (e.g. Java), try/catch is free unless an exception actually occurs, but I'm guessing that's not the case in most (all?) JS VMs. Even so, yeah, seems like unless you had a ton of modules, you probably wouldn't notice the difference. Of course, older IEs might be worse.

danro commented 11 years ago

@jrburke I think this issue has been resolved now for a while, thanks to your work on the rjs optimizer.