Closed millermedeiros closed 12 years ago
Hey, thanks for taking the time to submit this.
So obviously when this is loaded with an AMD script loader, accounting
is never declared globally or on the window
, which is good.
There may be people who want to have it defined globally (for better or worse) despite loading it in as a module, and it may cause confusion if they weren't aware of the way AMD works to avoid global pollution, but I suppose that's a case for an example in the docs, something along the lines of
define(['accounting'], function(accounting){
window.accounting = accounting;
});
Otherwise, this all looks great!
One other thing I'm wondering - if this is loaded in as an AMD module, and you also want to include an extension to it (i.e. accounting-units.js, although in theory there may be a bunch of 'plugin' type mixins later on), you'll get a ReferenceError: accounting is not defined
- as the extension assumes that accounting will be global.
I think this can be addressed though, and I was planning to decide on a more appropriate plugin/mixin structure for the library, when it became important enough.
yes, the plugin will also need to handle the different module formats... here is one way of doing it:
(function(def) {
def(function(accounting){
//accounting is available here in all envs
//plugin code should come here
});
}(function(factory){
if(typeof define === 'function' && define.amd){ //AMD
define(['accounting'], factory);
} else if (typeof require === 'function'){ //CommonJS
factory( require('accounting') );
} else { //Browser
factory(window['accounting']);
}
}));
the code could be simplified if we assume accounting is already loaded:
//this code will only work on an AMD loader if you make sure that you require
//accounting before loading the plugin. On RequireJS it can be easily done with
//the `priority` setting.
(function(global) {
//naive require shim, only work in AMD if dependency already loaded before
var require = (typeof require === 'function')? require : function(mod){
return global[mod];
};
var accounting = require('accounting');
//accounting is available here in all envs
//plugin code should come here
}(this));
note that we are exporting as a named AMD module
define('accounting', [], accounting);
it means that to load it in RequireJS you need to do:it's safer to use named modules since multiple modules can be concatenated into same file...