model method without prototype on Backbone.Collection causes crash

[johnfn]

This was discussed previously in #2080; however it was discussed in the context of _.bindAll(this) and I think that confused the discussion, because the issue is not about _.bindAll(this) at all. It actually happens whenever the model function lacks a prototype property. For example:

var myCollection = Backbone.Collection.extend({ 
    initialize: function() { 
        this.model.prototype = null;
    }, 

    model: function(attrs, options) { 
        return new Backbone.Model(attrs, options); 
    }
});

new myCollection([{ a: 1 }]); 

Backbone errors on this line https://github.com/jashkenas/backbone/blob/master/backbone.js#L939 because prototype doesn't exist.

Obviously, few people would write this.model.prototype = null, but a lot of people might write this.model = _.bind(this.model), which also removes the prototype. Backbone should be able to handle this case.

It seems like it'd be pretty straightforward to have line 939 also check for the existence of prototype. What do you think?

[jamiebuilds]

I think the bigger issue here is that when model is a function that returns an instance modelId will not do what it is meant to. This could be fixed by making modelId:

modelId: function(model, attrs) {
  return attrs[model.idAttribute || 'id'];
}

Since all the uses of it have a reference to model handy and since it's not released it would be easy to make this change.

[jamiebuilds]

Actually the only issue would be with the collection.get() method.

[jridgewell]

I was always a little curious why #modelId didn't receive a model instance. That might be a handy PR.

As for setting model.prototype = null, I don't think supporting it is really necessary. Functions have a prototype property, and it is an object. Anything else doesn't seem like good JavaScript.

[johnfn]

@jridgewell _.bind and _.bindAll return functions without a prototype property, and I'm sure there are other functions that do the same thing. Saying "Anything else doesn't seem like good JavaScript" flies in the face of underscore.js as a library.

[jamiebuilds]

@johnfn But why are you trying to bind the model function anyways?

[johnfn]

@thejameskyle I wanted to use a property on this that I set inside initialize.

[jamiebuilds]

Hmm, does it make sense to potentially change the constructor in _prepareModel to:

var model;
if (this._isModel(this.model.prototype) || this.model === Model) {
  model = new this.model(attrs, options);
} else {
  model = this.model(attrs, options);
}

This way it doesn't new functions that return model instances, and you have the correct this.

[jridgewell]

Obviously, few people would write this.model.prototype = null, but a lot of people might write this.model = _.bind(this.model), which also removes the prototype. Backbone should be able to handle this case.

I read the code and jumped into the discussion. Sorry, I should have finished reading.

Changing #modelId wouldn't be the worse thing?

function modelId(attrs) {
    return _.result(this.model.prototype, 'idAttribute', 'id');
}

[jamiebuilds]

function modelId(attrs) {
    return _.result(this.model.prototype, 'idAttribute', 'id');
}

@jridgewell I don't see how that helps?

[jridgewell]

If this.model.prototype == null, _.result will guard against a null obj and return the default. Though now that I think about it, default is only on edge.

[jamiebuilds]

That means it'd still be a problem if you have a custom idAttribute with a bound model method or a function that returns a model instance.

[jridgewell]

That was deemed acceptable in #2836. I was just trying to get rid of the error in the base #modelId.

I think it's going to end up being left to you to define a custom #modelId if you're not going to provide a true constructor function. It's a hell of a lot better than setting bound.prototype = {idAttribute: 'id'}; before #modelId landed.

[jamiebuilds]

Since modelId has not been released, there is the potential to pass it the model modelId(model, attrs) and handle this use case better. Although, I'm not sure how to handle the get use case other than storing a reference to the various idAttribute's in the container and looking for each of them.

[jridgewell]

I realize now I forgot the attrs[] access in my #modelId...

function modelId(attrs) {
    return attrs[_.result(this.model.prototype, 'idAttribute', 'id')];
}

Also, after thinking about it, it does make sense that #modelId only takes the model's attributes since the id should really only be dependent on the attributes ("{attrs.id}", "{attrs.type}-{attrs.id}", etc). There may be some places where using this.modelId(model.attributes) is unnecessary, though, since model.id should always be up to date?

[jamiebuilds]

@jridgewell That's essentially what exists today, this update was so that Collections could determine uniqueness (ids) instead of models (See: 4e2d209).

[akre54]

Moral of the story, avoid _.bindAll where you can, and especially don't bind modelId. It's unnecessary (all calls to modelId already get passed the correct this) and native Function.prototype.bind doesn't give the result a proper prototype instance.

Do we need to rethink the way modelId works with Collection#get at all?

[jamiebuilds]

@akre54 He wasn't binding modelId, he was binding model because it always gets called with new.

[akre54]

Ok so what's the issue? If @johnfn's collection looked like this instead, you'd also expect it not to work, right?

var myCollection = Backbone.Collection.extend({ 
    initialize: function() { 
        this.model.prototype = null;
    }, 

    model: Backbone.Model
});

[jamiebuilds]

@akre54 Shrug, like I mentioned above I think the bigger issue is specifying model as a method that returns an instance.

var Library = Backbone.Collection.extend({
  model: function(attrs, options) {
    if (condition) {
      return new PublicDocument(attrs, options);
    } else {
      return new PrivateDocument(attrs, options);
    }
  }
});

This will make modelId fail silently. If we can fix the other case along with this, that's great.

[akre54]

This will make modelId fail silently.

Only if you're using an idAttribute other than "id". If you're using a factory model with a different idAttribute, you have to override modelId too. It's a bit confusing but nothing some documentation can't fix.

[jamiebuilds]

It's a bit confusing but nothing some documentation can't fix.

👍 I wasn't aware of this prior to this issue, so that'd be good.

Do we need to rethink the way modelId works with Collection#get at all?

I think so, modelId makes much more sense to me if it's modelId(model, attrs).

[akre54]

We usually add documentation when we cut a release.

I've had a 1.2.0 pull open for a while now (#3285), which may or may not get used for docs, but I'll add this in just in case.

modelId makes much more sense to me if it's modelId(model, attrs).

Can you explain? How does it make sense to pass both the model and its attributes? Of the 6 instances modelId is called, 4 use model.attributes, 1 uses model.previousAttributes(), and 1 (in get) uses this._isModel(obj) ? obj.attributes : obj. What's the benefit?

[jridgewell]

I agree #modelId should probably accept a model instance (master does this.modelId(model.attributes) 4 different times). It shouldn't need to accept attrs, since you're getting the model. There are a few edge cases though:

1. We would need to drop support for this.get({id: 123}) and this.get({type: 'a', id: 123}) in favor of this.get(123) and this.get('a-123') (I think this.get(123) is the standard use case, anyways).
2. We would need some way perform bookkeeping for changing a model's id. If we separated out this._byCid from this._byId, that could be done pretty easily. This would also fix Collisions between id and cid within collection cause mayhem #2920.

[akre54]

We would need to drop support for this.get({id: 123}) and this.get({type: 'a', id: 123}) in favor of this.get(123) and this.get('a-123')

We want to be able to call get with unparsed attributes (in set and in the Collection attributeMethods).

This would also fix #2920.

Is #2920 a real issue though? I think the argument time and again has been that you shouldn't use arbitrary strings for your ids. It should be given to you by your persistence layer (as an integer or uuid, etc).

[jamiebuilds]

We would need to drop support for this.get({id: 123}) and this.get({type: 'a', id: 123}) in favor of this.get(123) and this.get('a-123')

Yeah, I don't want to get rid of this either.

I think this is once again an issue of multiple model types, and wanting to know what type you're dealing with. Although I don't know how common having multiple types in a collection with different idAttributes is.

[jridgewell]

We want to be able to call get with unparsed attributes (in set..)

Damn, I forgot to search for places using #get. We could get around that by instantiating the model before L718, but it'd mean an unnecessary object if it finds an existing.

Collection attributeMethods

What are the collection attributeMethods?

Is #2920 a real issue though?

No, but we need to be able to quickly grab the model's old id (eg. this._byCid[model.cid] = this.modelId(model)). It would kill two birds.

[jridgewell]

Actually, we could instantiate the model in #get...

function get(obj) {
    if (obj == null) return void 0;
    return this._byId[obj] || this._byId[this.modelId(this._isModel(obj) ? obj : new this.model(obj)) || this._byId[obj.cid];
}

[akre54]

Strongly disagree. get shouldn't have any side effects, this change completely breaks its contract.

[jridgewell]

I don't view instantiating an object as a side effect? We're not adding the model to the collection, just creating it so that it can be passed to #modelId. It was really just a thought, so that we could still support Edge Case 1.

[jridgewell]

Scratch my #get comments, it won't work out well due to the options argument #initialize may require. The closest I'm able to get is by letting #modelId determine if the argument is a model or an attributes hash.

diff --git backbone.js backbone.js
index 963fb15..14b6ce4 100644
--- backbone.js
+++ backbone.js
@@ -674,7 +674,7 @@
       for (var i = 0, length = models.length; i < length; i++) {
         var model = models[i] = this.get(models[i]);
         if (!model) continue;
-        var id = this.modelId(model.attributes);
+        var id = this.modelId(model);
         if (id != null) delete this._byId[id];
         delete this._byId[model.cid];
         var index = this.indexOf(model);
@@ -736,7 +736,7 @@
         // Do not add multiple models with the same `id`.
         model = existing || model;
         if (!model) continue;
-        id = this.modelId(model.attributes);
+        id = this.modelId(model);
         if (order && (model.isNew() || !modelMap[id])) {
           order.push(model);

@@ -837,8 +837,7 @@
     // Get a model from the set by id.
     get: function(obj) {
       if (obj == null) return void 0;
-      var id = this.modelId(this._isModel(obj) ? obj.attributes : obj);
-      return this._byId[obj] || this._byId[id] || this._byId[obj.cid];
+      return this._byId[obj] || this._byId[obj.cid] || this._byId[this.modelId(obj)];
     },

     // Get the model at the given index.
@@ -935,8 +934,9 @@
     },

     // Define how to uniquely identify models in the collection.
-    modelId: function (attrs) {
-      return attrs[this.model.prototype.idAttribute || 'id'];
+    modelId: function (model) {
+      var idAttribute = _.result(this.model.prototype, 'idAttribute') || 'id';
+      return this._isModel(model) ? model.id : model[idAttribute];
     },

     // Private method to reset all internal state. Called when the collection
@@ -971,7 +971,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);
       if (id != null) this._byId[id] = model;
       model.on('all', this._onModelEvent, this);
     },
@@ -991,7 +991,7 @@
       if (event === 'destroy') this.remove(model, options);
       if (event === 'change') {
         var prevId = this.modelId(model.previousAttributes());
-        var id = this.modelId(model.attributes);
+        var id = this.modelId(model);
         if (prevId !== id) {
           if (prevId != null) delete this._byId[prevId];
           if (id != null) this._byId[id] = model;
diff --git test/collection.js test/collection.js
index 27c87b0..38cfa57 100644
--- test/collection.js
+++ test/collection.js
@@ -1422,6 +1422,9 @@
       },

       modelId: function (attrs) {
+        if (this._isModel(attrs)) {
+            attrs = attrs.attributes;
+        }
         return attrs.type + '-' + attrs.id;
       }
     });

[akre54]

Although I don't know how common having multiple types in a collection with different idAttributes is.

Agreed. Though as a counter anecdote to this I recently worked on a project where tracks were keyed in the db by track_id, artists by artist_id, albums by album_id, etc. but had to live in the same Backbone Collection. Depends on your backend.

We could get around that by instantiating the model before

Nope. We've gone down that path before and it's horrible. We built the class creator at Skillshare using an older version of Backbone that had this behavior. We had to hack around correctly parsing ids (and even temporarily persisting cids) to get models to show up correctly. Not the behavior you want at all and I'm grateful that it was eventually fixed.

Your change seems natural, but I'm not a fan of polluting modelId in this way. It makes its interface ugly (and I especially don't like the part about sometimes passing a model and sometimes passing attributes).

[jridgewell]

Nope. We've gone down that path before and it's horrible.

I was finding that out. I'm 👍 for leaving modelId method signature as is, though we may want check for the existence of prototype before trying to grab the idAttribute, and leave it to the end dev to override it when they use a factory.

function modelId(attrs) {
    return attrs[_.result(this.model.prototype, 'idAttribute') || 'id'];
}

---

We built the class creator at Skillshare using an older version of Backbone that had this behavior.

I forgot, @danapplegate mentioned you worked at Skillshare.

[akre54]

Why would idAttribute ever be a function? What's the goal of using _.result there?

If it's just for the guard we can do that in plain js.

[jridgewell]

Just a guard.

[akre54]

Ok how about

modelId: function (attrs) {
  var idAttr = (this.model.prototype && this.model.prototype.idAttribute) || 'id';
  return attrs[idAttr];
}

or

modelId: function (attrs) {
  var modelProto = this.model.prototype;
  return attrs[(modelProto && modelProto.idAttribute) || 'id'];
}

To be sure, it looks like we're back where we started. Which is to say it looks like this case is only useful when you're overriding model anyway. I'll add in the doc note.

[jridgewell]

I prefer the second option, personally.

To be sure, it looks like we're back where we started. Which is to say it looks like this case is only useful when you're overriding model anyway. I'll add in the doc note.

I think this that's appropriate. 👍