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

Already on GitHub? Sign in to your account

Backbone.Collection.nest() #614

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
8 participants

geddski commented Sep 14, 2011

Models often need nested collections. Given the example in the FAQ:

var Mailbox = Backbone.Model.extend({
  initialize: function() {
    this.messages = new Messages;
  }
});

var myBox = new Mailbox;

Let's say you're using a NoSQL DB like mongodb, and are pulling down all the data for a given mailbox on page load. So your model's toJSON() output looks like this:

{
mailboxName: "432 West",
messages: [
  {from: "Mom", title: "Learn your JavaScript"},
  {from: "City Hall", title: "Your dogs bark too loud"}
]
}

Now in your app you change or add a message:

var momsMessage = myBox.messages.at(1);
momsMessage.set({title: "return to sender"});

Well now we have a big problem. myBox.toJSON() contains the original data, not the updated data. Your 'return to sender' title won't get saved to the server unless you override Mailbox's toJSON function. What a pain, bloating all our models with overridden toJSON functions and change events.

Backbone should be much smarter about nesting collections. The model's underlying data should point to the same data as the nested collection. This is easy with JS thanks to reference types (objects, arrays, etc.)
I created a simple static function called Backbone.Collection.nest. You pass it the model, the attribute name, and collection instance. It returns the collection instance for convenience. Example usage:

var Mailbox = Backbone.Model.extend({
  initialize: function() {
    this.messages = Backbone.Collection.nest(this, 'messages', new Messages(this.get('messages')));
  }
});

var myBox = new Mailbox;

Now when you render myBox in a template or save() it to the server, it will always have the right data, all without overriding toJSON or any other trickery.

The only real complaint I've heard about backbone is that it's complex and difficult to nest collections. Problem solved.

yuchi commented Sep 14, 2011

I think this is simply awesome. I do not have time to check the code or the new APIs, but sounds really good.

yuchi commented Sep 14, 2011

Looked at the code. I do not like at all how the collection and the json inside the model are synced. the concept here seems promising but instead of creating a method inside Backbone.Collection i would create a method inside Backbone.Model.

Backbone.Model.prototype.nest = function (attributeName, collection) {
  (this.nested || (this.nested = []))[attrbuteName] = collection;
  this[attributeName] = collection;
}

Backbone.Model.prototype.toJSON = function (nesting) {
  var jsonObj = _.clone(this.attributes);
  if (nesting) {
    _.each(this.nested, function(nested,attributeName){
      jsonObj[attributeName] = nested.toJSON();
    }
  }
  return jsonObj;
}

Backbone.Collection.prototype.toJSON = function () {
  // is nesting necessary? probably a deepness count would be better.
  return this.map(function(model){ return model.toJSON(false) });
}

geddski commented Sep 14, 2011

@yuchi please explain what you don't like about it. It's not so much that the " collection and the json inside the model are synced", but rather they are two references to the same object.

There are likely a ton of ways to solve this same problem. Have you tried out/tested your idea? One thing I'd change is having to pass myModel.toJSON(true) doesn't seem ideal. Certainly a flag could be set in the model if the nesting is used, then you check for that:

if(this._hasNested){
...
}

But the real question is which way is more performant? Your solution would call toJSON() on every nested object, cloning every single one and combining the output. I can't help but think that won't scale.

My solution simply loops through each collection and reassigns references - should be much faster than calling toJSON() for every nested object and cloning each one. Also mine only happens once, during initialize(). After that, any call to toJSON() will be extremely fast. Your solution happens up front during toJSON() (usually in the render() method) AND happens every time toJSON() is called on the model, slowing down template rendering and model saving each time.

Thanks for the input! It's great to get some more eyes on this.

yuchi commented Sep 14, 2011

My simple example lacks some completeness, that's true :)

What actually scares me of your implementation is the way you access to the model attribute. If you add something to the collection it gets added to model.attributes[attributeName] too, and that's ok. But what happens when you set something on one of the models in the nested collection? It fires a change:attribute event that we can trap and use to update the models back to the model attribute. The same must be told about deletion. Another problem here is also remotion vs destruction of the object. If the nested collection just represents a portion of the real collection (which lives only in the server) then things get messed up really fast.

If you add to my example model.unset(attributeName) and a parsing function to update the nested collection on fetch/set/fetch-after-update/fetch-after-creation actions you get a perfect sync between the two, and not something partial (and possibly confusing) like your actual implementation is. But you can't set a parse function, without the risk to the function itself being overwritten. So you should rely on change:attributeName to update what you have to update and then re-unset it.

PS: I thing the worst thing can happen here on Github is a pull request without feedback, or only with positive feedback by users and negative from maintainers. A situation that shows that users cannot explain their motivation correctly or the maintainers do not understand the problem or users do not understand the long term objectives of the project. And are not completely mutually exclusive!! :)

Owner

jashkenas commented Sep 14, 2011

We can leave this ticket open for a little while longer for feedback, but...

These sort of considerations are precisely why nested model support isn't something that Backbone forces on you -- rather you have the building blocks to make it fit your app (including changing the source code -- that's why it's all annotated).

Trying to push out a one-size-fits-all implementation of nested models would hurt far more people than it would help.

http://documentcloud.github.com/backbone/#FAQ-tim-toady

yuchi commented Sep 14, 2011

And that's why I stopped myself more than a few times from implementing and proposing a nesting solution.

But in the last months I've been building a large social network (see at the bottom of this comment) UI on top of Backbone and jQuery and so many times I hoped for a concise standard.

The fight here is between simpleness and extensiveness. Another issue preventing a definitive solution to surface from the community in the form of an extension is the wacky way to extend BB. Sure, you can BB.Model::something, but seems to much of an hack than a standard way. I still think the power of jQuery was and is in the ease of building plugins.

If there's not a "native" implementation plugins and BB extensions simply will not be able to use that feature. Probably PaulUithol/Backbone-relational could become more "official"?

BTW you can find the social network here http://www.mastersoup.com/ , to register simply use FB Connect or go to http://www.mastersoup.com/signup/

geddski commented Sep 15, 2011

@yuchi you asked:

But what happens when you set something on one of the models in the nested collection?

I believe you've misunderstood. With my solution the model's data and the nested collection's data point to the same object. Pass by reference, remember? So by calling set on a nested collection item, you are updating the model's item too. No need to bind to the change event. Allow me to illustrate.

Before

When you create a nested model like so this.messages = new Message(this.get('messages')) you create a new object that is separate from your model's underlying data. It now looks like this:
Before

Hence the problem: you update your nested collection, and your model's data is out of date because they are different objects.

After

The solution I am proposing simply changes the model's attribute data to point to the nested collection's data:

after

Also whenever the nested collection adds/removes an item, that same item data gets added back to the model data. It's simple and elegantly solves the nesting problem. To see more examples of usage, see my tests.

@jashkenas I agree about backbone.js providing just the building blocks. Backbone provides models and collections, but is missing an essential and core building block for linking them together. This provides a sensible default, and is still opt-in. It's a static method on the Backbone.Controller method that you have to explicitly invoke:
this.messages = Backbone.Collection.nest(this, 'messages', new Messages(this.get('messages')));

Even if this patch doesn't get merged in, I do think backbone should keep the data references pointing to the same objects. Where backbone first starts to deviate from this is in the Backbone.Model:

//creates a new object based on attributes, rather than modifying the attributes object that was passed in to the model
var attributes = _.extend({}, defaults, attributes);

The problem of nesting collections and having duplicate, stale data is a big one. I don't care how it's solved - my proposal, you modifying backbone models to point to the original objects passed in, whatever. I just know this is backbone's #1 weakness, and it would be a piece of cake to provide a building block for it.

Collaborator

tbranyen commented Sep 15, 2011

@GEDDesign Backbone already provides excellent support for nested collections/models inside of attributes. The issue becomes how to work with toJSON() for templating/serialization and then more importantly, how does this play into persistence.

In my mind a solution needs to solve the problem which is keeping a property in attributes for toJSON, but not for persistence (necessarily). Its very confusing. I believe a filter step in-between may help, to allow a developer to strip out anything they don't want persisted. In fact this may already be possible, I'm just not aware of how.

I like this idea. Nesting is biggest area I've frustrating I've found in Backbone.

If nothing else, it would make a nice plugin.

yuchi commented Sep 17, 2011

@GEDDesign ok, I totally missed the part where you swap every object inside model.attributes[attributeName] with their nestedCollection.at(i).attributes counterpart. But this still allows for nestedCollection.add(obj,{silent:true}) to mess up the architecture quite easily.

That's why I propose to remove the model.attributes[attributeName] completely and in getJSON retrieve them from the nested collection. By the other hand this breaks the model.get(attributeName) usage.

I'd like to create a big cons vs pros of every possible choices.

If there was a Backbone.Base to work with you could use it as a base for the nesting, allowing both Collections and Models to be nested each other. Yes, a nested model in a collection would break toJSON, but sounds interesting to me.

The granularity of this seems confused. What is the problem with creating the Messages collection of Message models in your Mailbox.parse and then keeping it updated with change events? Deduplicate the message data from the Mailbox attributes. Then there is no problem saving individual messages or a changed subset of the collection.

rxgx commented Sep 23, 2011

This isn't a problem with collection or record (a.k.a. Backbone.Model) relationships, but a formatting problem. The AJAX/JSON service you are using should be handling "nested" JSON object creation/parsing via the back-end Domain/Model or, at least, Controller in a way like ActiveRecord does.

arbales commented Sep 23, 2011

@yuchi wrote…

Sure, you can BB.Model::something, but seems to much of an hack than a standard way. I still think the power of jQuery was and is in the ease of building plugins.

If there's not a "native" implementation plugins and BB extensions simply will not be able to use that feature. Probably PaulUithol/Backbone-relational could become more "official"?

Although this is tangential to the discussion at hand, I'd like to mention that using prototypes and inheritance is a standard way to extend backbone. At Do, our Do.Models.Base extends Backbone.Model with numerous enhancements for our use case. I believe you should also be able to use _.extend to add methods from an object or prototype to distribute your work for others to take advantage of.

On topic at hand, I agree that taking advantage of references to ensure more predictable behavior would be a great thing, as @GEDDesign points out.

Owner

jashkenas commented Sep 23, 2011

++ @arbales.

... and when Do has a public demo out, be sure to drop me a line so we can get y'all hooked up in the #examples on the homepage.

yuchi commented Sep 23, 2011

OT: @arbales I think I've explained myself in a horrible way. BB.Thing::somethingelse is in fact the best way to extend BB.Thing, that's sure. What I meant is that there aren't many places where you can hook in, IMO.

arbales commented Sep 23, 2011

@yuchi, hmmm… I'd say you could start by subclassing Backbone.Model for your use case. We do this to create transient unsynced properties, but you do have to override some methods. Fortunately, super in coffee script makes this a breeze. It does seem that complications could arise if you wanted to abstract this behavior as a mixin for others to use, if that's your complaint, I think it's a fair point. Extensions like Momento provide a manager class you initialize in your models or base and apply behaviors from there, that seems like the lightest solution.

On-topic, I'd say this is a place where Backbone's behavior in serialization is unexpected, but it doesn't prompt me to ask for a plugin API. As others have mentioned, it seems like this is more of a serialization problem than anything else.

@ghost

ghost commented Jan 12, 2012

While it's fine if Backbone wants to adopt the position that nested model support is the user's responsibility, it would be nice if it at least made an effort to not destroy anyone's chances at implementing such a thing successfully.

Case in point: Model.clone() copies objects by invoking this.constructor(), and assuming you don't want to duplicate all the functionality Model() gives you in your subclass, you'll probably be invoking it through super and going on about your own initialization later.

Model() initializes your object's attributes with a simple _.extend(), which is obviously never going to instantiate nested Model objects of an appropriate type or call their initializers for you. You have zero say in this process, so that means Backbone just broke everything and you have no choice but to rewrite a significant portion of Model yourself from the ground up.

Again this can be addressed by putting the logic regarding cloning/instantiating or aliasing the sub-models in your Model.parse method. Now that the constructor calls parse (#773) it would be simple to override clone to pass {parse:true}. In fact, it should probably do so as the default if you would like to open a specific issue about that.

@ghost

ghost commented Jan 12, 2012

Interesting. The current release definitely does not invoke parse() at any point in Model(), and even if it did, the fact that I have to explicitly ask for that behavior means it's not going to happen when I need it most--i.e. whenever Backbone decides to instantiate an object on my behalf. Seem like a bit of a false start, but hopefully a step in the right direction.

But sure, I can override and wrap a lot of things. At this point I have completely replaced 11 of Model's 27 documented methods--about 40%--and written a whole bunch of new ones just to get this particular feature. The methods I have not overridden are generally exceedingly trivial one-liners of little importance and at this point I feel like I'm fighting the framework more than I'm actually using it. Conversations I've had with others have lead me to believe that this is really not an uncommon situation and that many Backbone adopters are spending a lot of time solving the exact same problems over and over again because there is no officially-sanctioned implementation publicly available and no clear guidance as to how one should be constructed. Instead we just have a lot of dismissive responses about how easy it should be and how annoying we are for asking why the examples aren't there to back these claims up. This is very discouraging.

I'm perfectly fine with the whole deficient-by-design mantra, but if things are going to be left as an exercise for the reader then I think the framework should either go to great lengths to get out of the way and directly facilitate such augmentations or clearly label itself as ultimately disposable scaffolding. The router's probably the only thing I'm directly using anymore and I sadly find I'm left with little choice but to wonder if it's worth bringing the whole file into memory just for that one piece.

Owner

jashkenas commented Jan 13, 2012

@nsdpt et al.: Backbone's mantra certainly isn't "deficient-by-design", but I still don't believe that it's wise to try and force a one-size-fits-all solution for all users of Backbone. Here are a number of reasons why:

  • We don't want to privilege NoSQL dbs over SQL dbs (quite the opposite, actually).
  • List of ids work in some cases, but not all.
  • Ranges of ids work in some cases, but not all.
  • Direct references to complete other tables as collections work in some cases, but not all.
  • Requesting joined models as needed from the server works in some cases, but not all.
  • Nested JSON documents work in some cases, but not all.
  • And so on, and so on.

Ideally, Backbone allows you to use any of these approaches fairly easily. The .nest() function in this pull request would make for a great plugin.

@jashkenas jashkenas closed this Jan 13, 2012

geddski commented Jan 14, 2012

@jashkenas fair enough. Is there a central place you'd like me to post this to as a plugin?

Owner

jashkenas commented Jan 14, 2012

Yep, the Plugins and Extensions page on the wiki.

geddski commented Jan 14, 2012

Ok. I've moved the function into its own gist file: nesting.js and updated the plugins page of the wiki. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment