pathable / supermodel

Supermodel - Minimal Model Tracking for Backbonejs
http://pathable.github.io/supermodel
MIT License
229 stars 36 forks source link

has().one's `id` option doesn't seem to be respected #37

Closed twalker closed 11 years ago

twalker commented 11 years ago

Thanks for such a great project, a welcome alternative to other libraries.

I may be misunderstanding the use of the id option, but it doesn't seem to be respected--forcing the associationname_id convention instead.

var Album = Supermodel.Model.extend({
  defaults: {
    id: null,
    name: null,
    artist: null // associated artist ID attribute
  }
});

var Albums = Backbone.Collection.extend({
  model: function(attrs, options){
    return Album.create(attrs, options);
  }
});

var Artist = Supermodel.Model.extend({
  defaults: {
    id: null,
    name: null,
    albums: null, // array of album id's
  }
});

Artist.has().many('albums', {
  collection: Albums.extend({
    url: function(){
      return '/api/artists/' + this.owner.id + '/albums';
    }
  }),
  inverse: 'artist'
});

Album.has().one('artist', {
  model: Artist,
  id: 'artist', // toJSON is be using `artist_id` instead?!
  inverse: 'albums'
});

var metallica = Artist.create({"id": "metallica","name": "Metallica", "albums": ["puppets"]});
var album = Album.create({"id":"puppets", "artist": "metallica", "name":"Master of Puppets"});

album.artist() === metallica;
// > true
album.toJSON();
// > Object {id: "puppets", name: "Master of Puppets", artist_id: "metallica"}

Why artist_id instead of artist?

braddunbar commented 11 years ago

Hi @twalker! Looks like a bug to me. :)

Fixed above in da8834e, let me know if it doesn't work for you.

twalker commented 11 years ago

Works splendidly, thank you.

twalker commented 11 years ago

In a related note, it looks like the many association is removing the associated property.

Using the example above:

var metallica = Artist.create({"id": "metallica","name": "Metallica", "albums": ["puppets"]});
metallica.toJSON();
// > Object {id: "metallica", name: "Metallica"}

The albums property which contained album id's was unset? Is this by design?

Thank you @braddunbar

braddunbar commented 11 years ago

Yep, that's definitely by design. The nested attributes are generally attributes of another model and keeping multiple copies tends to cause consistency problems.

twalker commented 11 years ago

Makes sense. In my case, I'm persisting associations through associated_id properties, not full model attributes. It looks like I can put the associated id properties back before POST/PUT/PATCH by overriding toJSON in the model. e.g:

Artist.prototype.toJSON = function(){
  this.set('albums', this.albums().pluck('id'));
  return Supermodel.Model.prototype.toJSON.apply(this, arguments);
}

I don't suppose my association by id properties is common enough to warrant a feature request. Thanks for your time and for such an elegant piece of work.

braddunbar commented 11 years ago

Yep, that's what I do as well if I need to save a nested property (somewhat rarely). However, I wouldn't suggest setting it back on the attributes. I would just attach it to the result of the super call, avoiding the doubled data problem.

Artist.prototype.toJSON = function() {
  _.extend(Supermodel.Model.prototype.toJSON.apply(this, arguments), {
    albums: this.albums().pluck('id')
  });
};