From 4b64eebcba67d5eb23bbd62ae4722abfe612efda Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Wed, 23 Sep 2015 11:23:53 -0400 Subject: [PATCH] Fix _removeModels regression MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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'); }); ``` --- backbone.js | 6 ++++++ test/collection.js | 3 ++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/backbone.js b/backbone.js index 2aac5cbc9..a497b25a6 100644 --- a/backbone.js +++ b/backbone.js @@ -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); diff --git a/test/collection.js b/test/collection.js index c691bc70e..df916643a 100644 --- a/test/collection.js +++ b/test/collection.js @@ -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');