walmartlabs / thorax

Strengthening your Backbone
http://thoraxjs.org/
Other
1.32k stars 129 forks source link

CollectionView not removing items from this.children on sort event #404

Closed bengladwell closed 9 years ago

bengladwell commented 9 years ago

TLDR; a sort event on a CollectionView's collection adds new ItemViews for each model to this.children and to the DOM, but does not remove the old ItemViews from this.children; the old ItemViews are retained indefinitely.

Here is a jsfiddle showing the problem.

I'm unsure if this is a bug or if I am doing something wrong.


sort event on collection -> CollectionView#onCollectionReset() CollectionView#onCollectionReset() -> CollectionView#renderCollection() CollectionView#renderCollection() -> CollectionView#appendItem() for each model

appendItem uses CollectionView#renderItem to produce a new view for the model. https://github.com/walmartlabs/thorax/blob/v3.0.0-beta.5/src/collection.js#L226

itemView = this.renderItem.call(this, model, index);

If the new view has a cid, View#_addChild adds the new view to this.children, among other things. https://github.com/walmartlabs/thorax/blob/v3.0.0-beta.5/src/collection.js#L230-233

if (itemView.cid) {
  this._addChild(itemView);
  itemView.ensureRendered();
}

Why not reuse the existing ItemViews in this.children in response to a sort event? Am I missing something?

In general, it looks like old ItemViews are not removed in CollectionView#onCollectionReset.

kpdecker commented 9 years ago

I don't see anything wrong with your code. Will take a look and see if there is anything telling.