_removeModels regression bug

[kmalakoff]

It looks like in 1.2.1, _removeModels was simplified, but introduced a regression bug.

    _removeModels: function(models, options) {
      var removed = [];
      for (var i = 0; i < models.length; i++) {
        var model = this.get(models[i]);
        if (!model) continue;
        // CODE REMOVED HERE

        var index = this.indexOf(model);
        this.models.splice(index, 1);
        this.length--;

        if (!options.silent) {
          options.index = index;
          model.trigger('remove', model, this, options);
        }

        removed.push(model);
        this._removeReference(model, options);
      }
      return removed.length ? removed : false;
    },

Removed the following lines after if (!model) continue;

        var id = this.modelId(model.attributes);
        if (id != null) delete this._byId[id];
        delete this._byId[model.cid];

What this means is that if you are listening to a remove event and you check for removal, you will get into an infinite loop because the event is now called before the id is removed from this._byId:

    if (current_related_model = collection.get(id)) collection.remove(current_related_model)

I think the _byId code should be put back so when events are fired, they represent the most up-to-date state.

[akre54]

We changed this in #3637.

This is what you mean, right?

var col = new Backbone.Collection([{id: 1}, {id: 2}, {id: 3}, {id: 4}]);
col.on('remove', function() {
  col.get(3); // exists, but shouldn't.
});
col.remove(3);

@jridgewell do we have a solution? I can't find the pull, but I think we changed the ordering of when model.collection and the collection's _onModelEvent were updated a little while back. Any way we can reconcile this?

[jridgewell]

We can just move the #_removeReference call above the trigger.

[akre54]

But then collection.on('remove') would fail because _removeReference unbinds the model listener, and the trigger happens on the model, not the collection.

Alternatively, we could move the _removeReference call above the trigger then trigger on both the model and the collection directly, juggling the args a bit to align it with _onModelEvent.

[kmalakoff]

@akre54 yes, that's the use case.

Unfortunately, the control flow is a little tricky. I would go with the one with the least side effects...regarding moving up the call and manually triggering, I'm not sure if there are cases where users or libraries might have registered something that will get cleared from the 'all' before triggering that would get skipped: model.off('all', this._onModelEvent, this).

Personally, I just would inline the cleanup code like before. Alternatively, you could update _removeReference to not unbind the model events and call off manually, or to take an optional parameter to skip clearing them and call it twice (once with and once without the option).

[jridgewell]

Is there a reason you're trying to get the model in a remove event? That part that doesn't make sense to me, we're already providing you with the model parameter.

---

But then collection.on('remove') would fail because _removeReference unbinds the model listener, and the trigger happens on the model, not the collection.

Agh, forgot about this. All of the above fixes would be acceptable to me. But I think the cleanup code should be left in #_removeReference, it just makes sense there.

[martynsmith]

@jridgewell a reason I've been doing it is that I'm binding to the collection like so:

myCollection.bind('add remove update', assertState)

In this way, I'm using the hooks to immutably update some other object. My handler is generic (i.e. doesn't use the parameters passed, instead just references the collection directly). It seems slightly misleading that inside the handler, this happens:

myCollection.length === 0 # true
myCollection.get('idJustRemoved') === undefined # false

[kmalakoff]

I have the same reason as @martynsmith - generic code for BackboneORM.

Any update on a fix?

[berniegp]

My use case:

During the collection's remove callback, collection._byId is out of sync with collection.models. This is a problem if a remove listener tries to remove the same model currently being removed from the collection. In that case, this occurs:

1. this.get(models[i]) uses collection._byId and determines that the model is contained in the collection
2. this.indexOf(model) returns -1 so the following splice clobbers the model array.

In a more generic sense, it seems wrong that collection.get() finds a model while collection.indexOf() does not. collection._onModelEvent() also uses _byId so is probably vulnerable.

If the whole _removeReference method is inlined before the callback, the model's collection property will have been unset in the callback. This could cause code that relies on that property to fail.