Fix _removeModels regression

Fixes #3693.

This leaves open the question of whether events triggered on the model
during a ‘remove’ listener should also trigger on the model. Just
something to revisit for V2.

```js
col.on('other', (model) => {
// Should this be triggered?
});

col.on('remove', (model) => {
// If the model is really "removed" (we can't `#get` it anymore)
// by the time this listener is called, then I'd argue that this
// shouldn't trigger the 'other' event on the collection...
model.trigger('other');
});
```
This commit is contained in:
Justin Ridgewell
2015-09-23 11:23:53 -04:00
parent cf73162b9a
commit 4b64eebcba
2 changed files with 8 additions and 1 deletions

View File

@@ -1088,6 +1088,12 @@
this.models.splice(index, 1);
this.length--;
// Remove references before triggering 'remove' event to prevent an
// infinite loop. #3693
delete this._byId[model.cid];
var id = this.modelId(model.attributes);
if (id != null) delete this._byId[id];
if (!options.silent) {
options.index = index;
model.trigger('remove', model, this, options);

View File

@@ -298,12 +298,13 @@
deepEqual(col.pluck('id'), [1, 2, 3]);
});
test("remove", 10, function() {
test("remove", 11, function() {
var removed = null;
var result = null;
col.on('remove', function(model, col, options) {
removed = model.get('label');
equal(options.index, 3);
equal(col.get(model), undefined, '#3693: model cannot be fetched from collection');
});
result = col.remove(d);
equal(removed, 'd');