Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for CommonJS, AMD, and global namespace usage methods. #110

Merged
merged 5 commits into from
Jan 30, 2014
Merged

Add support for CommonJS, AMD, and global namespace usage methods. #110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e1ec181
Add support for CommonJS, AMD, and global namespace usage methods.
bartek Jan 29, 2014
1fbebb2
Add jquery, underscore to package.json dependencies
bartek Jan 29, 2014
73dd489
Resolve jshint warnings.
bartek Jan 29, 2014
f3dd12e
Add test to ensure NestedModel is attached to Backbone when using req…
bartek Jan 29, 2014
27634ee
Use grunt-mocha-test instead of grunt-tape due to node 0.8 compatibil…
bartek Jan 30, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ module.exports = function(grunt) {
qunit: {
index: ['test/index.html']
},
mochaTest: {
test: {
src: ['test/mocha-*.js']
}
},
watch: {
files: ['<config:lint.files>', 'test/**'],
tasks: ['jshint', 'qunit']
Expand All @@ -32,7 +37,8 @@ module.exports = function(grunt) {

grunt.loadNpmTasks('grunt-contrib-jshint');
grunt.loadNpmTasks('grunt-contrib-qunit');
grunt.loadNpmTasks('grunt-mocha-test');

// Default task.
grunt.registerTask('default', ['jshint', 'qunit']);
grunt.registerTask('default', ['jshint', 'qunit', 'mochaTest']);
};
18 changes: 15 additions & 3 deletions backbone-nested.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,19 @@
* Copyright (c) 2011-2012 Aidan Feldman
* MIT Licensed (LICENSE)
*/
/*global $, _, Backbone */
(function(){
/*global define, require, module */
(function(root, factory){
if (typeof exports !== 'undefined') {
// Define as CommonJS export:
module.exports = factory(require("jquery"), require("underscore"), require("backbone"));
} else if (typeof define === 'function' && define.amd) {
// Define as AMD:
define(["jquery", "underscore", "backbone"], factory);
} else {
// Just run it:
factory(root.$, root._, root.Backbone);
}
}(this, function($, _, Backbone) {
'use strict';

var _delayedTriggers = [],
Expand Down Expand Up @@ -359,4 +370,5 @@

});

}());
return Backbone;
}));
6 changes: 5 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@
"test": "grunt"
},
"dependencies": {
"jquery": ">=1.8.3",
"underscore": ">=1.5.0",
"backbone": ">=0.9.2"
},
"devDependencies": {
"grunt": "~0.4.1",
"grunt-contrib-qunit": "~0.2.2",
"grunt-contrib-jshint": "~0.6.4",
"grunt-mocha-test": "~0.8.2",
"grunt-contrib-watch": "~0.5.3",
"jslitmus": "~0.1.0"
"jslitmus": "~0.1.0",
"mocha": "~1.17.1"
},
"peerDependencies": {
"grunt-cli": "*",
Expand Down
11 changes: 11 additions & 0 deletions test/mocha-require.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*global describe, it*/
var assert = require('assert');

var Backbone = require('backbone');
require('../backbone-nested');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking that it shouldn't modify the Backbone object if a module loader is detected (and maybe not at all, come to think of it) – not very Node-like, y'know? However, could be convinced that this is ok for consistency's sake, and also fine for initial Node support. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might be a good approach, I'm not a fan of the mysterious, in-place modification but that's how it was done with globals so I stuck to it.

Perhaps something like this?

 var Backbone = require('backbone');
 Backbone.NestedModel = require('backbone-nested');

If you're keen on not modifying it in the long term, that might be a better approach, but I feel to be consistent, and for this initial support, we can continue modifying it in-place, and open another PR for that issue.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's reasonable – @gkatsev?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option is to pass in a Backbone object into backbone-nested which would then modify it.
Anyway, it seems like most others of these types do modify backbone.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something to consider in the future is having backbone-nested return a function that accepts a backbone object to modify.
so, you do:

var Backbone = require('backbone');
require('backbone-nested')(Backbone);
console.log(Backbone.NestedModel);

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modifying the Backbone object is probably a Bad Idea ™️ anyway – I've been thinking we should move to just having it create a NestedModel object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@afeld Love that approach. The in-place modification of Backbone looks like it might become inconsistent with how other modules are going.

For example, when using Backbone with browserify, the suggested approach to hooking up jQuery is now:

 Backbone.$ = require('jquery');

As the zen of Python states, "Explicit is better than implicit" :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My latest suggestion modifies backbone explicitly. It just does it for the user, explicitly, when the user wants it done, and on whatever specific object the user wants it done to.
Since we're just adding a new property to the backbone object, just returning NestedModel is more than reasonable.


describe("CommonJS support", function() {
it('should attach to Backbone when require backbone-nested', function() {
assert.ok(Backbone.NestedModel);
});
});