Should Backbone.Collection throw an error when client code attempts to add the same model twice?

[jgonggrijp]

While studying #4249, I found this old comment to another ticket by @jashkenas, in which he writes that Backbone once used to throw an error in this scenario: https://github.com/jashkenas/backbone/issues/2976#issuecomment-33711327.

If it were up to me, I would prefer that adding duplicate models would throw an error, though I imagine this would be a breaking change for many users.

That's how it used to be ... and this behavior was an "enhancement" ;)

It is ambiguous from this comment alone whether the throwing was an "enhancement", or the removal of the throwing. It would not be the first time that a feature is inadvertently removed. If aCollection.add([{id: 1}, {id: 1}]) was supposed to throw (which intuitively makes sense) and this was removed for no good reason, then I suggest reinstating this behavior in the future Backbone 2.0. First, though, we should investigate what happened exactly and why.

[paulfalgout]

https://github.com/jashkenas/backbone/commit/18fba5772430f667c0d3cfb88d880039381c25fa

[paulfalgout]

https://github.com/jashkenas/backbone/issues/413#issuecomment-2560333 https://github.com/jashkenas/backbone/issues/976 https://github.com/jashkenas/backbone/issues/1142#issuecomment-4694082

[jgonggrijp]

Thanks @paulfalgout. Seems like the old throwing behavior was not only on aCollection.add([{id: 1}, {id: 1}]), but also on just aCollection.add({id: 1}) if id 1 was already in the collection. Those are two quite different situations in my opinion.

What do you think? Should a collection throw in the first case but not the second? Or should we just stick to the current behavior?

[paulfalgout]

My initial reaction is to suspect that handling the first case and not the second is too costly a check to add to collection.add for the value of the error... and I'm not sure about what issues adding the error all of time would cause... clearly some didn't like it.

I might be running down a rabbit trail, but I started to wonder what the point of the merge option was at all.. particularly for add... on the way I found this interesting test: https://github.com/jashkenas/backbone/commit/61987e8debca6a80b5ac07eef8a0d774a150cda2#diff-523b866fbabfc8e9d2fbbd3551d6f455338a06d5baf8a48b5a5fcc1d4be7bcb4R1134

But then where the merge option was added https://github.com/jashkenas/backbone/pull/1220

Honestly I don't understand the use case of using add in #1220.. that seems like the perfect situation for set. But the existence of merge: true for add makes me hesitant to want to throw an error... though.. in the case of [{id: 1},{id:1}] merge does seemingly look worse.. but this'd be true for fetch/set as well?

[Rayraz]

Taking the effort to throw an error would imply it's bad practice, rather than simply an unusual use-case. Which makes me wonder, what specifically is the problem and why? Are multiple identical id's a problem? Are multiple copies of the same data a problem? Are multiple instances of the same model a problem?

I haven't used Backbone in a while so I might be a little rusty in terms of thinking of undesirable side-effects, but I can't think of a reason why in principle any of those should not be allowed.

I can think of a scenario that doesn't require multiple copies of the same record but still would see multiple models with the same ID. For example, one could push multiple new records onto a collection, each of which have not yet been assigned an ID by the server. One could then push the entire collection to the server to be committed and receive an updated version of the collection data in return where each record now has been assigned it's own unique ID.

While most likely not a common scenario at all, I would also hesitate to say the ability to have the same record present in a collection more than once is never going to be useful.

Finally, if backbone does allow multiple copies, why not allow multiple references to the same instance as well? This way if you update once, the entire collection remains up-to-date. It would be significantly more complicated to keep multiple copies up-to-date instead.

[paulfalgout]

For example, one could push multiple new records onto a collection

New models without ids won't be a concern here. That's currently possible.

Primarily I think the most striking reason for not having copies per id is that .get(fooId) needs to return one model from the collection.

If for some reason the user is expecting [{id: 1},{id: 1}], it won't but with no explanation given. That said, a quick google search is pretty explanatory.

[jgonggrijp]

@paulfalgout Right, nowadays add is just a proxy to set that defaults to merge: false. I used add as a handle but I meant to imply set and fetch as well. I thought the problem might be more obvious if calling add. It looks like at the time of #1220, add was still containing the logic that is now in set, so set might not have been a viable alternative at that time.

@Rayraz Backbone's contract is that model id's need not be set, but if they're set, they must be unique (that's mentioned in the documentation in the id and idAttribute sections over here). Having two copies or references of the same model within the same collection is absolutely out of the question, also because it would then become ambiguous which array index a particular id refers to. For this reason, I would argue that explicitly adding the same id within a single call is definitely a mistake.

[Rayraz]

The getter shouldn't be too much of a problem for major version update. It'd probably be relatively easy to implement something like get(fooId, startIndex) or getNext(fooId, startIndex).

However, if the index and id are always tied together then it indeed doesn't make sense to allow duplicate id's. I suppose if someone ever really does need copies it wouldn't be too much hassle to wrap them in some type of container.