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

[bartek]

I want to use modules like browserify with backbone-nested, but found I could not in its current state. This adjustment adds CommonJS, AMD, and keeps the global window namespace support.

Some things I'm curious about:

• Tests for this are possible, but I'm only familiar with mocha. The tests you have within QUnit seem to continue passing, though.
• Wonder if we can remove the jQuery dependency by finding a different method to deepClone?

I am fairly new to using CommonJS modules in the browser but the pattern I used below is the same one used by other backbone modules like backbone.epoxy

Thanks for your time, and I hope this helps!

[afeld]

Nice! Just a couple things:

• Needs to run Backbone and NestedModel (and maybe all of Backbone) tests against NestedModel
• JSHint warnings need to be resolved

/cc #73

[afeld]

See also: https://github.com/umdjs/umd

[bartek]

@afeld Thanks! Can you clarify what you mean by running those tests? Doesn't your test suite already do that?

[afeld]

No, it's running on PhantomJS via Node, so not actually testing the code via a require().

On second thought, I think it would be fine to just have a simple test to check that requireing the module in Node works as expected (and maybe that it loads the dependencies as expected) – it doesn't need to run all the model tests on top of that.

[bartek]

@afeld Got ya! I'll get that sent up sometime tomorrow. Thanks for the quick response :)

[gkatsev]

The umd shim available in the umd org is still broken >.<.
It checks for AMD first which can cause problems. The way done here is the correct way of checking exports first and then AMD and then global. (A la this umd)

[gkatsev]

The other option to checking this code in is to use browserify as a devDependency and when publishing to npm we first run browserify --standalone on it. Then we add it as an artifact to a release so that bower's git stuff can pick up the file as well.

[gkatsev]

Also, as much as getting rid of a dependency would be awesome, since backbone requires jquery as a dep, I think it's better to just use jquery's deep extend rather than trying to implement our own.

[afeld]

Agreed.

[bartek]

Backbone is annoying in that it doesn't say it hard requires jQuery like underscore. This is obviously because it can support a variety of other DOM/ajax libraries (zepto, ender, etc., anything that can do those basic tasks)

I know it's pretty much a given that if you're using backbone-nested, you're using jQuery, but I still don't like the prescription to it in the package.json. The unfortunate part is by removing it, we add a block of code just to replicate the deepExtend function. Alas..

We'll probably just have to wait for underscore to implement deep extend, like lodash already has, and then we can remove it :)

[afeld]

Let's just get it working first, and then we can worry about relaxing the dependency.

[bartek]

@afeld I agree.

I pushed a new test. It requires some new test dependencies, as mixing this kind of require() test into qunit proved to be a pain. Given the nature of npm install -D, this shouldn't add any confusion.

It is currently failing on node 0.8, but that just seems to be the tape module. I need to tackle my day job now, so I'll get back to this later. If you have any recommendations for test harnesses you'd prefer I use, let me know. Cheers

[gkatsev]

tape is good. Tape should work with node 0.8. Travis is saying that grunt can't find the tape task for some reason.

[bartek]

@gkatsev Yeah, that's what it seemed like. It might require an adjustment to the grunt task.

[bartek]

@afeld

New tests! tape was failing on node 0.8 because of this issue:

tapjs/tap-parser#9 (comment)

Let me know how you feel about the latest changes, cheers :)

[afeld]

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?

[bartek]

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.

[afeld]

I think that's reasonable – @gkatsev?

[gkatsev]

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.

[gkatsev]

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);

[afeld]

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.

[bartek]

@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" :-)

[gkatsev]

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.

[afeld]

Awesome! Will do another PR with a README change and a release.