From d96620968b66d7051367cd8ae1db31ca2e21e034 Mon Sep 17 00:00:00 2001 From: Paul Falgout Date: Tue, 3 Oct 2017 14:44:35 +0900 Subject: [PATCH] Use add at end perf only when sortWithCollection is false (#3471) Because a collection can be modified without the full understanding on the collectionview, the optimistic adding that reduces perf is not ideal when `sortWithCollection: true` This opens the door to all kinds of edge cases. So we should only apply it when the collectionview itself is maintaining the sort. However this brought up a related issue. If a collection was `sortWithCollection: false` the _default_ `viewComparator` was still sorting with collection on add or update which mean that setting it to false only prevented a resort if `collection.sort()` was called directly, but not for any other circumstance. If the collectionview is not maintaining the collection's sort, this prevents the sort from occurring at all. --- src/next-collection-view.js | 17 +++-- .../collection-view-children.spec.js | 3 + .../collection-view-data.spec.js | 54 ++++++++++++---- .../collection-view-sorting.spec.js | 64 ++++++++++++------- 4 files changed, 95 insertions(+), 43 deletions(-) diff --git a/src/next-collection-view.js b/src/next-collection-view.js index 1e6d67860a..cc2a0b5bcb 100644 --- a/src/next-collection-view.js +++ b/src/next-collection-view.js @@ -364,11 +364,11 @@ const CollectionView = Backbone.View.extend({ // Sorts views by viewComparator and sets the children to the new order _sortChildren() { - if (this.viewComparator === false) { return; } + let viewComparator = this.getComparator(); - this.triggerMethod('before:sort', this); + if (!viewComparator) { return; } - let viewComparator = this.getComparator(); + this.triggerMethod('before:sort', this); if (_.isFunction(viewComparator)) { // Must use native bind to preserve length @@ -404,13 +404,18 @@ const CollectionView = Backbone.View.extend({ // Additionally override this function to provide custom // viewComparator logic getComparator() { - return this.viewComparator || this._viewComparator; + if (this.viewComparator) { return this.viewComparator } + + if (!this.sortWithCollection || this.viewComparator === false || !this.collection) { + return false; + } + + return this._viewComparator; }, // Default internal view comparator that order the views by // the order of the collection _viewComparator(view) { - if (!this.collection) { return; } return this.collection.indexOf(view.model); }, @@ -439,7 +444,7 @@ const CollectionView = Backbone.View.extend({ delete this._addedViews; if (!viewFilter) { - if (addedViews && _.every(addedViews, _.bind(this._isAddedAtEnd, this))) { + if (!this.sortWithCollection && addedViews && _.every(addedViews, _.bind(this._isAddedAtEnd, this))) { return addedViews; } diff --git a/test/unit/next-collection-view/collection-view-children.spec.js b/test/unit/next-collection-view/collection-view-children.spec.js index 4b153cfff7..5079c41558 100644 --- a/test/unit/next-collection-view/collection-view-children.spec.js +++ b/test/unit/next-collection-view/collection-view-children.spec.js @@ -494,6 +494,9 @@ describe('next CollectionView Children', function() { it('should only append the added child', function() { this.sinon.stub(myCollectionView, 'attachHtml'); + + // Only true if not maintaining collection sort + myCollectionView.sortWithCollection = false; myCollectionView.addChildView(anotherView); const callArgs = myCollectionView.attachHtml.args[0]; const attachHtmlEls = callArgs[0]; diff --git a/test/unit/next-collection-view/collection-view-data.spec.js b/test/unit/next-collection-view/collection-view-data.spec.js index 7cc60b797b..977dbe38c6 100644 --- a/test/unit/next-collection-view/collection-view-data.spec.js +++ b/test/unit/next-collection-view/collection-view-data.spec.js @@ -209,26 +209,52 @@ describe('next CollectionView Data', function() { let myCollectionView; let collection; - beforeEach(function() { - collection = new Backbone.Collection([{ id: 1 }, { id: 2 }, { id: 3 }]); + describe('when sortWithCollection is true', function() { + beforeEach(function() { + collection = new Backbone.Collection([{ id: 1 }, { id: 2 }, { id: 3 }]); - myCollectionView = new MyCollectionView({ collection }); - myCollectionView.render(); - }); + myCollectionView = new MyCollectionView({ collection }); + myCollectionView.render(); + }); + + it('should append all of the children', function() { + this.sinon.stub(myCollectionView, 'attachHtml'); + collection.add([{ id: 4 }, { id: 5 }]); + + const callArgs = myCollectionView.attachHtml.args[0]; + const attachHtmlEls = callArgs[0]; + expect($(attachHtmlEls).children()).to.have.lengthOf(5); + }); - it('should only append the added children', function() { - this.sinon.stub(myCollectionView, 'attachHtml'); - collection.add([{ id: 4 }, { id: 5 }]); + it('should still have all children attached', function() { + collection.add([{ id: 4 }, { id: 5 }]); - const callArgs = myCollectionView.attachHtml.args[0]; - const attachHtmlEls = callArgs[0]; - expect($(attachHtmlEls).children()).to.have.lengthOf(2); + expect(myCollectionView.$el.children()).to.have.lengthOf(5); + }); }); - it('should still have all children attached', function() { - collection.add([{ id: 4 }, { id: 5 }]); + describe('when sortWithCollection is false', function() { + beforeEach(function() { + collection = new Backbone.Collection([{ id: 1 }, { id: 2 }, { id: 3 }]); + + myCollectionView = new MyCollectionView({ collection, sortWithCollection: false }); + myCollectionView.render(); + }); + + it('should only append the added children', function() { + this.sinon.stub(myCollectionView, 'attachHtml'); + collection.add([{ id: 4 }, { id: 5 }]); + + const callArgs = myCollectionView.attachHtml.args[0]; + const attachHtmlEls = callArgs[0]; + expect($(attachHtmlEls).children()).to.have.lengthOf(2); + }); + + it('should still have all children attached', function() { + collection.add([{ id: 4 }, { id: 5 }]); - expect(myCollectionView.$el.children()).to.have.lengthOf(5); + expect(myCollectionView.$el.children()).to.have.lengthOf(5); + }); }); }); diff --git a/test/unit/next-collection-view/collection-view-sorting.spec.js b/test/unit/next-collection-view/collection-view-sorting.spec.js index de155b2686..43267db5ba 100644 --- a/test/unit/next-collection-view/collection-view-sorting.spec.js +++ b/test/unit/next-collection-view/collection-view-sorting.spec.js @@ -100,40 +100,58 @@ describe('NextCollectionView - Sorting', function() { describe('when viewComparator is falsy but not false', function() { let myCollectionView; - beforeEach(function() { - myCollectionView = new MyCollectionView({ - sortWithCollection: false, - collection + describe('when sortWithCollection is true', function() { + beforeEach(function() { + myCollectionView = new MyCollectionView({ collection }); + + myCollectionView.render(); }); - myCollectionView.render(); - }); + it('should sort the collection by the collection index', function() { + expect(myCollectionView.$el.text()).to.equal(noSortText); + }); - it('should sort the collection by the collection index', function() { - expect(myCollectionView.$el.text()).to.equal(noSortText); - }); + it('should call "before:sort" event', function() { + expect(myCollectionView.onBeforeSort) + .to.have.been.calledOnce + .and.calledWith(myCollectionView); + }); - it('should call "before:sort" event', function() { - expect(myCollectionView.onBeforeSort) - .to.have.been.calledOnce - .and.calledWith(myCollectionView); - }); + it('should call "sort" event', function() { + expect(myCollectionView.onSort) + .to.have.been.calledOnce + .and.calledWith(myCollectionView); + }); - it('should call "sort" event', function() { - expect(myCollectionView.onSort) - .to.have.been.calledOnce - .and.calledWith(myCollectionView); + describe('when resorting the collection', function() { + it('should sort the collectionView by the collection index', function() { + collection.comparator = 'sort'; + collection.sort(); + + myCollectionView.render(); + + expect(myCollectionView.$el.text()).to.equal(sortText); + }); + }); }); - describe('when resorting the collection', function() { - it('should sort the collectionView by the collection index', function() { - collection.comparator = 'sort'; - collection.sort(); + describe('when sortWithCollection is false', function() { + beforeEach(function() { + myCollectionView = new MyCollectionView({ + sortWithCollection: false, + collection + }); myCollectionView.render(); + }); + + it('should not call "before:sort" event', function() { + expect(myCollectionView.onBeforeSort).to.not.be.called; + }); - expect(myCollectionView.$el.text()).to.equal(sortText); + it('should not call "sort" event', function() { + expect(myCollectionView.onSort).to.not.be.called; }); }); });