Add AMD registration as an option.#688
Add AMD registration as an option.#688jrburke wants to merge 4 commits intojashkenas:masterfrom jrburke:optamd
Conversation
Uses a common factory function with adapters for CommonJS and default browser env to register appropriately based on the environment.
Missing semicolon that was in original file, and consistent use of 'id'
There was a problem hiding this comment.
is there any way to break out this define function? putting it inline seems weird to me.
There was a problem hiding this comment.
@tbranyen, here is alternate path that could be taken for the patch. It still means passing a function into an anonymous, immediately executed function, but it may be aesthetically pleasing:
(function (factory){
var root = this,
previousBackbone = root.Backbone;
if (typeof exports !== 'undefined') {
// CommonJS
var $;
try {
$ = require('jquery');
} catch (e) {
// ignore, it is ok in node if it fails.
}
factory(root, previousBackbone, exports, require('underscore'), $);
} else if (typeof define === 'function' && define.amd) {
// AMD
define('backbone', ['underscore', 'jquery', 'exports'], function (_, $, exports) {
factory(root, previousBackbone, exports, _, $);
});
} else {
// Browser globals
factory(root, previousBackbone, (root.Backbone = {}), root._, (root.jQuery || root.Zepto || root.ender));
}
}(function (root, previousBackbone, Backbone, _, $) {
//Backbone code goes in here
});If you would rather see this approach taken, I'll do a different branch with this approach today.
There was a problem hiding this comment.
I went ahead and did a branch that uses this different registration approach, you can do the compare here. It passes the tests, including the simple node one and the requirejs-based one I have in a test branch.
If you prefer I do a pull with this new optamd2 branch instead of this pull request, then I will close out this pull request and open another with that branch.
|
You should avoid |
|
Thanks for the catch, fixed by calling initial setup with global this in this commit. |
|
This pull request is obsolete, see #710 instead. |
Uses a common factory function with adapters for CommonJS and default browser env to register appropriately based on the environment.
I placed the define() call on the same line as the function(define) { so that a level of indent in the backbone.js code would trigger a noisy diff. However, I can place it on its own line and do an indent of the code if that is preferred.
I also have an optamd-withtest branch that has a test-requirejs.html file showing it working with an AMD loader. I also tested the code in Node, doing a simple test with a Backbone.Model. I did not commit that Node test since it was just a simple code check.
Backbone probably does not want the test page, so I just put the core registration changes in this pull request branch. However I have no problem merging in the test page if that is preferred. The test uses underscore 1.2.1 and jQuery 1.7rc1, which both call define() to register as AMD modules.
Related to Issue #684, where it highlights the usefulness of this patch. Also see the similar ticket for underscore. In short, AMD registration is useful because: