From c71336641ee0354776556cdcfa5522f23ca14abf Mon Sep 17 00:00:00 2001 From: Adam Miller Date: Tue, 9 Feb 2016 13:02:02 -0800 Subject: [PATCH 1/2] Update to `modelId()` - modelId now takes the model in question as the second argument - modelId will look at the model in question's idAttribute before falling back to Collection.prototype.model.modelId and then 'id' - Makes basic polymorphic collection work out of the box while retaining backwards compatability --- backbone.js | 16 ++++++++-------- test/collection.js | 25 +++++++++++++++++-------- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/backbone.js b/backbone.js index d0cad3eb6..c136a3796 100644 --- a/backbone.js +++ b/backbone.js @@ -984,7 +984,7 @@ get: function(obj) { if (obj == null) return void 0; return this._byId[obj] || - this._byId[this.modelId(obj.attributes || obj)] || + this._byId[this.modelId(obj.attributes || obj, obj)] || obj.cid && this._byId[obj.cid]; }, @@ -1088,8 +1088,8 @@ }, // Define how to uniquely identify models in the collection. - modelId: function(attrs) { - return attrs[this.model.prototype.idAttribute || 'id']; + modelId: function(attrs, model) { + return attrs[(model && model.idAttribute) || this.model.prototype.idAttribute || 'id']; }, // Private method to reset all internal state. Called when the collection @@ -1129,7 +1129,7 @@ // Remove references before triggering 'remove' event to prevent an // infinite loop. #3693 delete this._byId[model.cid]; - var id = this.modelId(model.attributes); + var id = this.modelId(model.attributes, model); if (id != null) delete this._byId[id]; if (!options.silent) { @@ -1152,7 +1152,7 @@ // Internal method to create a model's ties to a collection. _addReference: function(model, options) { this._byId[model.cid] = model; - var id = this.modelId(model.attributes); + var id = this.modelId(model.attributes, model); if (id != null) this._byId[id] = model; model.on('all', this._onModelEvent, this); }, @@ -1160,7 +1160,7 @@ // Internal method to sever a model's ties to a collection. _removeReference: function(model, options) { delete this._byId[model.cid]; - var id = this.modelId(model.attributes); + var id = this.modelId(model.attributes, model); if (id != null) delete this._byId[id]; if (this === model.collection) delete model.collection; model.off('all', this._onModelEvent, this); @@ -1175,8 +1175,8 @@ if ((event === 'add' || event === 'remove') && collection !== this) return; if (event === 'destroy') this.remove(model, options); if (event === 'change') { - var prevId = this.modelId(model.previousAttributes()); - var id = this.modelId(model.attributes); + var prevId = this.modelId(model.previousAttributes(), model); + var id = this.modelId(model.attributes, model); if (prevId !== id) { if (prevId != null) delete this._byId[prevId]; if (id != null) this._byId[id] = model; diff --git a/test/collection.js b/test/collection.js index a0f6bf662..0c53db379 100644 --- a/test/collection.js +++ b/test/collection.js @@ -1642,12 +1642,20 @@ QUnit.test('modelId', function(assert) { var Stooge = Backbone.Model.extend(); - var StoogeCollection = Backbone.Collection.extend({model: Stooge}); + var StoogeCollection = Backbone.Collection.extend(); - // Default to using `Collection::model::idAttribute`. + // Default to using `id` if `model::idAttribute` and `Collection::model::idAttribute` not present. assert.equal(StoogeCollection.prototype.modelId({id: 1}), 1); + + // Default to using `model::idAttribute` if present. Stooge.prototype.idAttribute = '_id'; + var model = new Stooge({_id: 1}); + assert.equal(StoogeCollection.prototype.modelId(model.attributes, model), 1); + + // Default to using `Collection::model::idAttribute` if model::idAttribute not present. + StoogeCollection.prototype.model = Stooge; assert.equal(StoogeCollection.prototype.modelId({_id: 1}), 1); + }); QUnit.test('Polymorphic models work with "simple" constructors', function(assert) { @@ -1702,7 +1710,7 @@ assert.equal(collection.at(1), collection.get('b-1')); }); - QUnit.test('Collection with polymorphic models receives default id from modelId', function(assert) { + QUnit.test('Collection with polymorphic models receives id from modelId using model instance idAttribute', function(assert) { assert.expect(6); // When the polymorphic models use 'id' for the idAttribute, all is fine. var C1 = Backbone.Collection.extend({ @@ -1715,7 +1723,8 @@ assert.equal(c1.modelId({id: 1}), 1); // If the polymorphic models define their own idAttribute, - // the modelId method should be overridden, for the reason below. + // the modelId method will use the model's idAttribute property before the + // collection's model constructor's. var M = Backbone.Model.extend({ idAttribute: '_id' }); @@ -1725,12 +1734,12 @@ } }); var c2 = new C2({'_id': 1}); - assert.equal(c2.get(1), void 0); - assert.equal(c2.modelId(c2.at(0).attributes), void 0); + assert.equal(c2.get(1), c2.at(0)); + assert.equal(c2.modelId(c2.at(0).attributes, c2.at(0)), 1); var m = new M({'_id': 2}); c2.add(m); - assert.equal(c2.get(2), void 0); - assert.equal(c2.modelId(m.attributes), void 0); + assert.equal(c2.get(2), m); + assert.equal(c2.modelId(m.attributes, m), 2); }); QUnit.test('#3039 #3951: adding at index fires with correct at', function(assert) { From b3a28132901f3acc881bfa1efcc4acb04b2061b9 Mon Sep 17 00:00:00 2001 From: Adam Miller Date: Wed, 24 Feb 2016 17:36:51 -0800 Subject: [PATCH 2/2] Only pass idAttribute to collection.modelId() --- backbone.js | 16 ++++++++-------- test/collection.js | 6 +++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/backbone.js b/backbone.js index c136a3796..00e684052 100644 --- a/backbone.js +++ b/backbone.js @@ -984,7 +984,7 @@ get: function(obj) { if (obj == null) return void 0; return this._byId[obj] || - this._byId[this.modelId(obj.attributes || obj, obj)] || + this._byId[this.modelId(obj.attributes || obj, obj.idAttribute)] || obj.cid && this._byId[obj.cid]; }, @@ -1088,8 +1088,8 @@ }, // Define how to uniquely identify models in the collection. - modelId: function(attrs, model) { - return attrs[(model && model.idAttribute) || this.model.prototype.idAttribute || 'id']; + modelId: function(attrs, idAttribute) { + return attrs[idAttribute || this.model.prototype.idAttribute || 'id']; }, // Private method to reset all internal state. Called when the collection @@ -1129,7 +1129,7 @@ // Remove references before triggering 'remove' event to prevent an // infinite loop. #3693 delete this._byId[model.cid]; - var id = this.modelId(model.attributes, model); + var id = this.modelId(model.attributes, model.idAttribute); if (id != null) delete this._byId[id]; if (!options.silent) { @@ -1152,7 +1152,7 @@ // Internal method to create a model's ties to a collection. _addReference: function(model, options) { this._byId[model.cid] = model; - var id = this.modelId(model.attributes, model); + var id = this.modelId(model.attributes, model.idAttribute); if (id != null) this._byId[id] = model; model.on('all', this._onModelEvent, this); }, @@ -1160,7 +1160,7 @@ // Internal method to sever a model's ties to a collection. _removeReference: function(model, options) { delete this._byId[model.cid]; - var id = this.modelId(model.attributes, model); + var id = this.modelId(model.attributes, model.idAttribute); if (id != null) delete this._byId[id]; if (this === model.collection) delete model.collection; model.off('all', this._onModelEvent, this); @@ -1175,8 +1175,8 @@ if ((event === 'add' || event === 'remove') && collection !== this) return; if (event === 'destroy') this.remove(model, options); if (event === 'change') { - var prevId = this.modelId(model.previousAttributes(), model); - var id = this.modelId(model.attributes, model); + var prevId = this.modelId(model.previousAttributes(), model.idAttribute); + var id = this.modelId(model.attributes, model.idAttribute); if (prevId !== id) { if (prevId != null) delete this._byId[prevId]; if (id != null) this._byId[id] = model; diff --git a/test/collection.js b/test/collection.js index 0c53db379..7bdee03f6 100644 --- a/test/collection.js +++ b/test/collection.js @@ -1650,7 +1650,7 @@ // Default to using `model::idAttribute` if present. Stooge.prototype.idAttribute = '_id'; var model = new Stooge({_id: 1}); - assert.equal(StoogeCollection.prototype.modelId(model.attributes, model), 1); + assert.equal(StoogeCollection.prototype.modelId(model.attributes, model.idAttribute), 1); // Default to using `Collection::model::idAttribute` if model::idAttribute not present. StoogeCollection.prototype.model = Stooge; @@ -1735,11 +1735,11 @@ }); var c2 = new C2({'_id': 1}); assert.equal(c2.get(1), c2.at(0)); - assert.equal(c2.modelId(c2.at(0).attributes, c2.at(0)), 1); + assert.equal(c2.modelId(c2.at(0).attributes, c2.at(0).idAttribute), 1); var m = new M({'_id': 2}); c2.add(m); assert.equal(c2.get(2), m); - assert.equal(c2.modelId(m.attributes, m), 2); + assert.equal(c2.modelId(m.attributes, m.idAttribute), 2); }); QUnit.test('#3039 #3951: adding at index fires with correct at', function(assert) {