backbone icon indicating copy to clipboard operation
backbone copied to clipboard

Is the idea of a collection's modelId functionality valid?

Open ksurakka opened this issue 8 years ago • 2 comments

We are running quite old version of the Backbone in a production and it has served well, thank you guys. Now some other library requires a newer version of the Backbone and we have a small issue with the new version. We can fix the issue quite easily, but as I searched a reason for it, I begun to think that is this modelId functionality of the Collection valid idea at all? (A related commit can be found here)

So the issue is that one cannot use the get function of Backbone.Collection if the model property of the collection is not set and models inside the collection have a idAttribute overridden.

E.g. following code logs undefined, although I would expect it to log the model object (code "works" in the version 1.1.2)

var MyModel = Backbone.Model.extend({ 
	idAttribute: 'myid'
});
var m = new MyModel({myid:'xxx'});
var c = new Backbone.Collection([m]); 
var byId = c.get(m.id); 
console.log(byId); // byId is undefined

If I define the model property for a collection, it works as I expect, but I think that a collection should not need to know about models it contains.

I think that modelId functionality breaks quite a fundamental behavior (get by id) to solve an edge case issue. And if this functionality is valid, I think that a API documentation should mention that the model of the Collection must be defined.

What do you think?

And thank you for your work, you have made the world better.

ksurakka avatar Dec 05 '16 14:12 ksurakka

modelId broke backwards compatablity, see this: https://github.com/jashkenas/backbone/pull/3966

bruslim avatar Feb 15 '17 16:02 bruslim

#3966 fixed this and was even merged, but seems to have been reverted on present master. Will need to investigate what has happened.

jgonggrijp avatar Dec 22 '21 02:12 jgonggrijp

Good news! I don't remember why I thought that #3966 was reverted when I wrote that, but if it was true, it is not true anymore. All the changes are in present master and I can confirm from a quick test in the console on https://backbonejs.org that everything works the way it should.

jgonggrijp avatar Jul 27 '23 21:07 jgonggrijp