jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.39k forks source link

Make Backbone.Model.prototype.toJSON() recursive #483

Closed shesek closed 13 years ago

shesek commented 13 years ago

For cases where a model contains attributes which are either another model or a collection of models, calling toJSON() on the containing model should invoke toJSON() calls on the contained models / collections instead of returning them as-is.

Backbone.Model.prototype.toJSON = function() {
    var json = _.clone(this.attributes);
    _.each(json, function(value, name) {
        _.isFunction(value.toJSON) && (obj[name] = value.toJSON());
    });
    return json;
}

This could also be made optional,

Backbone.Model.prototype.toJSON = function(recursive) {
    var json = _.clone(this.attributes);
    recursive && _.each(json, function(value, name) {
        _.isFunction(value.toJSON) && (obj[name] = value.toJSON());
    });
    return json;
}

If you prefer, I can create a pull request for that.

PaulUithol commented 13 years ago

You should really watch out not to create infinite loops - like (simplest example) serializing model1.model = model2, and model2.model = model1 (there are countless other ways to create cycles in these type of graphs of course ;)). You might want to take a look at the toJSON implementation in Backbone-relational?

shesek commented 13 years ago

Great work on Backbone-relational, Paul! I didn't know about that, and its very useful for what I'm working on. Thanks :-)

An implementation of something similar to your solution for circular references:

Backbone.Model.prototype.toJSON = function() {
    if (this._isSerializing) {
        return this.id || this.cid;
    }
    this._isSerializing = true;
    var json = _.clone(this.attributes);
    _.each(json, function(value, name) {
        _.isFunction(value.toJSON) && (json[name] = value.toJSON());
    });
    this._isSerializing = false;
    return json;
}

edit: as others have noted, it should've been s/obj/json

jashkenas commented 13 years ago

I don't think this should be patched -- by default, you shouldn't have models or collections (or any other non-JSON object) as an attribute of a model. If you want to get fancier, a more complete plugin like Backbone-relational is the way to go.

shesek commented 13 years ago

Well, Backbone-relational is a full-blown solution that might not be suited for everyone. IMO, the current state of Backbone.js's does allow you handle that quite well if you're willing to handle some stuff manually, and I've been working on a project that makes heavy use of models/collections as attributes of models with success.

Backbone-relational sugar coats it and makes it much easier to handle, but I don't think it should be required for that - it's quite common to have relations between models and collections, and Backbone isn't very far from allowing that... I'm not suggesting to merge all of Backbone-relational features to core, but some features, like that, seems quite simple compared to the benefits you get.

turadg commented 13 years ago

@jashkenas, what's the logic behind "you shouldn't have models or collections (or any other non-JSON object) as an attribute of a model"?

@shesek, if your patch can't fit into this project and backbone-relational is too heavy, what about making a lightweight backbone-nested that does just this?

shesek commented 13 years ago

That could be a good idea, but what would you put in backbone-nested other than recursive toJSON() support? The only thing I can think of is allowing defaults: { nestedModel: someNestedModel, nestedCollection: someNestedCollection } which'll set nestedModel to new someNestedModel and nestedCollection to new someNestedCollection, instead of having to handle that manually using a defaults() function.

As far as I see, other than that, its pretty much up to you to use the model properties appropriately.

If those are the only features, it doesn't require a fork, it can be added easily as monkey patches.

tbranyen commented 13 years ago

@turadg i can't speak for @jashkenas, but my rationale for that, is that you shouldn't have anything in attributes that should not persist. sure you can filter on your own, but that's hacky instead of just using instance properties on a model.

var model = new Model({ ... });
model.subCollection = new SubCollection();

instead of

var model = new Model({ ... });
model.set({ subCollection: new SubCollection() });
turadg commented 13 years ago

At the risk of highjacking this ticket, but I think it's related and a common use case: What is the best practice for using Backbone with Rails nested models? E.g. in `routes.rb:

resources :task do
  resources :steps
end

And in task.rb:

class Task < ActiveRecord::Base
  has_many :steps, :order => "index ASC"
  accepts_nested_attributes_for :steps
end

Then with: var task = new Model({urlRoot: '/tasks'}

How best to set up the steps collection so that it loads up when the task is loaded and has the right paths for syncing?

jasonshah commented 12 years ago

@shesek In your sample, you indicate:

_.isFunction(value.toJSON) && (obj[name] = value.toJSON());

What is obj a reference to? Thanks.

AlexanderPavlenko commented 12 years ago

@jasonshah I got it move on replaced obj[name] with json[name]

j1anb1n commented 12 years ago

this also works

Backbone.Model.prototype.toJSON = function() {
    return JSON.parse(JSON.stringify(this.attributes));
}
machineghost commented 12 years ago

"you shouldn't have models or collections (or any other non-JSON object) as an attribute of a model"

I can see the new tag line already: "Backbone.js: for when you want a sophisticated object framework that can't actually handle objects within objects" ;-)

Seriously though: A) There is no mention of that anywhere (that I could find) in the Backbone documentation. Under attributes it just says "The attributes property is the internal hash containing the model's state." No "That model's state should not include other models" ...

B) Is this the final resolution of this ticket (closed, no fix)? It seems hard to believe that Backbone models with sub-models + JSON = impossible.

jashkenas commented 12 years ago

@machineghost -- it's not impossible at all. It's just nice to keep it clean and separate your model objects from your database row values, and not stuff them in a hash together. Instead: http://backbonejs.org/#FAQ-nested

machineghost commented 12 years ago

Thanks for the link :-) Despite going over the Backbone docs hundreds of times, I've somehow managed to always miss that FAQ (and a few days ago I was even specifically looking for the list of events that's included there ...).

However, while I can understand the Backbone devs not wanting to put in effort to "include direct support for nested models", I still don't see why they can't just prevent toJSON from exploding on nested models. To put it another way, not preventing nested models from working != enabling nested models. As the posters above showed, this would only require a few lines of code, and I can't see how doing this would interfere with any of the "number of good patterns for modeling structured data on the client side" that Backbone "provide[s] the foundation for" (which is the reason given in that FAQ for not supporting nesting).

jashkenas commented 12 years ago

However, while I can understand the Backbone devs not wanting to put in effort to "include direct support for nested models", I still don't see why they can't just prevent toJSON from exploding on nested models.

... because only supporting a half-baked solution is worse than not supporting it in the first place. We'll just have made the default implementation of toJSON dramatically slower, and folks who want true nested model support still won't be able to use it. Better for you to override toJSON to do the right thing for your app.

Some of the elements that would be needed for a serious patch/take:

Etc, etc.

machineghost commented 12 years ago

I didn't realize the full scope of the issue; fair enough.

machineghost commented 12 years ago

P.S. Thanks jashkenas for the explanation.

brett-shwom commented 11 years ago

In response to: @shesek 's comment https://github.com/documentcloud/backbone/issues/483#issuecomment-1594271

obj[name] should actually be json[name]

so:

Backbone.Model.prototype.toJSON = function() {
    if (this._isSerializing) {
        return this.id || this.cid;
    }
    this._isSerializing = true;
    var json = _.clone(this.attributes);
    _.each(json, function(value, name) {
        _.isFunction(value.toJSON) && (json[name] = value.toJSON());
    });
    this._isSerializing = false;
    return json;
}

is better

eyaleizenberg commented 9 years ago

@brett-shwom or anyone else who is using @brett-shwom 's solution.

if value is null, this fails.

I recommend doing this:

Backbone.Model.prototype.toJSON = function() {
    if (this._isSerializing) {
        return this.id || this.cid;
    }
    this._isSerializing = true;
    var json = _.clone(this.attributes);
    _.each(json, function(value, name) {
        _.isFunction((value || "").toJSON) && (json[name] = value.toJSON());
    });
    this._isSerializing = false;
    return json;
}