Incorrect sorting when using a comparator attribute that is undefined on the model

[melnikov-s]

var Model = require('ampersand-model');
var Collection = require('ampersand-collection').extend({model: Model, comparator: 'attribute'});


var models = [new Model, new Model, new Model];
var collection = new Collection();
collection.reset(models);
console.log(models.map(function(m){return m.cid}));
console.log(collection.models.map(function(m){return m.cid}));

results:
['state1','state2','state3']
['state3', 'state2', 'state1']

Models will get sorted in reverse order when the comparator attribute is undefined on a model.

This result can be replicated in chrome and firefox but not phantom js.

This works correctly in Backbone.Collection as it uses the underscore sortBy method and not Array.prototype.sort .

Swapping these two lines https://github.com/AmpersandJS/ampersand-collection/blob/v1.3.17/ampersand-collection.js#L227-228 (and https://github.com/AmpersandJS/ampersand-collection/blob/v1.3.17/ampersand-collection.js#L235-236) seems to do the trick for chrome and firefox but breaks phantom js.

[melnikov-s]

After further investigation I found this: ariya/phantomjs#11090

Seems like for whatever reason phantomjs reverses the parameters passed into sort and this is something that might get resolved in phantomjs 2.0 .

So perhaps swapping the aforementioned lines might be the solution to this bug. Especially if you're not using phantomjs to run your automated tests.

[aaronmccall]

This is an interesting one. I wonder if maybe it would be better to simply return 0 when both left and right are undefined.

[melnikov-s]

https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

If compareFunction(a, b) returns 0, leave a and b unchanged with respect to each other, but sorted with respect to all different elements. Note: the ECMAscript standard does not guarantee this behaviour, and thus not all browsers (e.g. Mozilla versions dating back to at least 2003) respect this.

So I'm not so sure about return 0. I can create a PR for my suggested fix it should be fairly low impact.