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

@@ -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;
       }
 

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];

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);
+      });
     });
   });
 

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;
         });
       });
     });