Possible issue with inherited _onModelEvent

[bpartridge]

Just started using this, and I've been seeing an exception thrown where if model.destroy() is called, the "destroy" event triggers _onModelEvent on both the original Collection and on the FilteredCollection. When it's called on the FilteredCollection, it attempts to call this.remove(...), which is made to throw "Do not invoke directly." By copying in the functionality of _onModelEvent, without the clause that calls this.remove(...), everything seems to work fine.

In essence, the FilteredCollection is getting told to remove each model twice: from the model itself via a "destroy" event, and from the original collection's "remove" event.

I'll need to isolate a test case first (I'm using the Parse.com version of Backbone, so I'm not 100% sure that this is a bug in FilteredCollection), but thought I'd submit the issue to track it.

[dlikhten]

How interesting. The idea here is to remove the element from the original collection via the original collection's event and then modify the filtered one to remove it as well (thus propagating the event and synchronizing). So yes, two events would be fired (in the grand scheme of things).

Sorry I don't use parse's backbone at all. I hope there does not need to be a fork for the parse version.

In any case let me know what u find, thanks for investigating.

[bpartridge]

No problem. I think it's doing something like this:

"destroy" event on model
  originalCollection.remove
    "remove" event on originalCollection
      filteredCollection.removeModel
        filteredCollection._forceRemoveModel
  filteredCollection.remove
    throw error

Because the FilteredCollection, whenever it adds an item, is still calling Backbone.Collection.prototype.add, which is still doing (model = models[i]).on('all', this._onModelEvent, this);. And _onModelEvent is unchanged.

From what I see, there's no test case for model.destroy, only collection.remove. And model.destroy is the only thing in the original Backbone framework that calls Backbone.sync('delete', ...) AFAIK, so it's by far the easiest way to implement a delete button on an item view.

[dlikhten]

Gotcha. Let me know if you make any modifications, or a failing spec.

[dlikhten]

FYI: I am not using this in any current project, sorry, so it will be a bit of time before I look into this if you want me to fix it.

[bpartridge]

No worries, I've already fixed it, but I did the quick thing which was to copy the file into my project, change indentation to match my settings, add a quick line for Parse functionality. So it'll be a bit of time before I can make a pull request or a diff, but I'll take care of doing so as soon as I have the chance.

[dlikhten]

No problem. Fork away. If you can just give me your test case, I think it would be already useful.

Dmitriy Likhten

On Tuesday, November 20, 2012 at 12:34 PM, Brenton Partridge wrote:

No worries, I've already fixed it, but I did the quick thing which was to copy the file into my project, change indentation to match my settings, add a quick line for Parse functionality. So it'll be a bit of time before I can make a pull request or a diff, but I'll take care of doing so as soon as I have the chance.

—
Reply to this email directly or view it on GitHub (#3 (comment)).

[dlikhten]

If you can fork and show me your fixes, it would be helpful. I'm seing the same problem. I'll probably wind up fixing it too.

[dlikhten]

Ah, fix is simple. _onModelEvent should be a noop for a filtered collection. Makes life simpler. That code makes sense because if the model is "deleted" it should be removed for all collections that contain the model. But an add/remove can occur on any collection, really, and should not affect collections that are not the ones added/removed from.

Our collection is special, it does not really contain elements, in the classical sense, it uses some collection mechanisms but in the end it behaves like a collection, except all "containership" work is done by the underlying collection.