scttnlsn / backbone.io

Backbone.js sync via Socket.IO
http://scttnlsn.github.io/backbone.io
541 stars 66 forks source link

Backbone's isNew() check preventing model deletion when mongooseStore is used #37

Closed richardassar closed 11 years ago

richardassar commented 11 years ago

If you use the mongooseStore then only an _id is set on created models, when I attempt to .destroy() a model this fails (without error) because backbone's isNew() does...

isNew: function() {
    return this.id == null;
},

I ended up having to place this block of code in my model's initialize function

// Set id when _id is set
this.bind("change:_id", function() {
    this.set("id", this.get("_id"));
});

// Set id if _id is already set upon instantiation
var _id = this.get("_id");

if(_id)
    this.set("id", _id);

Which is pretty ugly, better if the mongooseStore rewrites the mongodb _id to id behind the scenes.

I'll work on a patch for this and submit.

scttnlsn commented 11 years ago

http://backbonejs.org/#Model-idAttribute

richardassar commented 11 years ago

Thanks (and apologies)

:)

On 9 March 2013 23:42, Scott Nelson notifications@github.com wrote:

http://backbonejs.org/#Model-idAttribute

— Reply to this email directly or view it on GitHubhttps://github.com/scttnlsn/backbone.io/issues/37#issuecomment-14673117 .

scttnlsn commented 11 years ago

No problem! Hope you got this before going too far with a patch.

richardassar commented 11 years ago

It didn't take me long. I'll push it and let you take a look.

I'm still thinking that the mongooseStore should do this, as it avoids the responsibility of adding an idAttribute for each and every model (which might be a pain if people switch between drivers).

Perhaps for conformance it's best to rewrite, the patch I wrote also pulls out __v.

I also spotted that one has to invoke .toObject() on the mongodb item, without which you end up copying a bunch of stuff over in your for...in loop you had in there.

Anyway, will push and let you be the judge.

Cheers,

Richard

On 9 March 2013 23:44, Scott Nelson notifications@github.com wrote:

No problem! Hope you got this before going too far with a patch.

— Reply to this email directly or view it on GitHubhttps://github.com/scttnlsn/backbone.io/issues/37#issuecomment-14673160 .

richardassar commented 11 years ago

richardassar/backbone.io@9f3cd344a8425e5b107d7720f5ba3ac3fd3f519a