jashkenas / backbone

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

blocking issues with deep attributes on cloning models #155

Closed jguillod closed 12 years ago

jguillod commented 13 years ago

Hi, Consider the code below. When the attributes model have more than 1 level then changing the value of some deep level attributes of the clone (or the original) will be also changed in the original (or the clone) as they share the same deep-level attribute object. A deepClone() function could be not simply enhancement but bug fix because browsing through the code we can suspect that the clone() should be replaced by the deepClone() in other places it is used. For instance this code: this.previousAttributes = .clone(this.attributes) with the current shallow cloning results in unexpected behavior when attributes are not a single level object. If you change a deep attributes like this.attributes.address then this._previousAttributes.address points to the same object and will not report anymore the previous attributes state. Maybe you could think about a _.extendDeep() like in jQuery.

I survey this interesting library looking for some lightweight MVC and raise some questions. Is there some forum where to exchange about Backbone and underscore?

Thanks for the nice job. Joel

var bill = new Backbone.Model({
  name: "Bill Smith",
  address:{ city:"Boston", street:"Some"}
});

var john = bill.clone();
console.log(bill.attributes === john.attributes);
// => true

john.set({name:"John Gospel"});
console.log([bill.get("name"), john.get("name")]);
// => ["Bill Smith", "John Gospel"]
console.log(bill.attributes === john.attributes);
// => false
console.log(bill.attributes.address === john.attributes.address);
// => true

john.get("address")["city"] = "Paris";
console.log('EXPECTED : ["Boston", "Paris"], FOUND :',
      [bill.get("address")["city"],john.get("address")["city"]]);
// => EXPECTED : ["Boston", "Paris"], FOUND : ["Paris", "Paris"]
console.log(bill.attributes.address === john.attributes.address);
// => true
martindrapeau commented 13 years ago

Jeremy, I just hit this same issue. I had a model's set event never get triggered because of shallow cloning. Why not use deep cloning in Model's change method? Thanks, --Martin

shesek commented 13 years ago

There's an issue at Underscore.js about that (https://github.com/documentcloud/underscore/issues/162), it seems like they don't want to add support foor deep cloning because there's no real reliable way of doing that.

jashkenas commented 12 years ago

Yep -- deep cloning isn't well supported or (often) desirable in JS. Backbone attributes are intentionally shallow ... and if you're using them to reflect your database columns and rows, things should line up nicely.