Error caused by collection.add when collection is changing #1604

Closed
philfreo opened this Issue Aug 31, 2012 · 3 comments

Projects

None yet

2 participants

@philfreo

Is what I'm trying to do unreasonable? It doesn't seem to be. Broken on 0.9.2 and master.

http://jsfiddle.net/philfreo/PzQt2/1/

// create a collection with some decent length
var collection = new Backbone.Collection([
    new Backbone.Model(),
    new Backbone.Model()
]);

// in some cases when we add a new model we may want to remove
// some existing one
collection.on('add', function() {
    collection.at(0).destroy();
});

// adding a new model at a certain position breaks things!
collection.add(new Backbone.Model(), { at: 0 });

Uncaught TypeError: Cannot read property 'cid' of undefined, backbone.js:637

@philfreo

I'm not sure this is the best solution, but making this change Collection.add (here) solves the problem for me.

From:

if (!cids[(model = this.models[i]).cid]) continue;

To:

model = this.models[i];
if (!model || !cids[model.cid]) continue;

Would appreciate any advice. Or if this looks correct I can make a PR + test case.

@philfreo
philfreo commented Sep 4, 2012

@braddunbar @documentcloud does my fix seem correct, or do you suggest some other workaround?

@caseywebdev caseywebdev added a commit to caseywebdev/backbone that referenced this issue Sep 7, 2012
@caseywebdev caseywebdev #1604 - Account for model removal during add. 3636039
@caseywebdev caseywebdev added a commit to caseywebdev/backbone that referenced this issue Sep 12, 2012
@caseywebdev caseywebdev Fixes #1638 and #1604 - Account for order change in `add` 7cc05bd
@braddunbar
Collaborator

Hi guys! I've addressed this in #1667. Would you mind giving that a spin?

@braddunbar braddunbar added a commit that closed this issue Sep 26, 2012
@braddunbar braddunbar Fix #1604 - Refactor add, remove index option.
* Avoid temporary constructs by splicing duplicates during traversal.
* Remove `index` option.
* Avoid creating empty options.
142d2ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment