computed.sort works incorrectly with empty sort properties array

[onechiporenko]

Problem: computed.sortmay do some "sorting" even if there aren't properties to sort.
Demo: https://ember-twiddle.com/83ed55ef7169c8dacefe?openFiles=application.controller.js%2C (see second column)
Possible solution: Check if normalizedSortProperties is empty, don't do any sorting (https://github.com/emberjs/ember.js/blob/v2.4.1/packages/ember-runtime/lib/computed/reduce_computed_macros.js#L670)

[pixelhandler]

@onechiporenko Can you explain in more detail? It appears to me that the empty array used as sortDefinition for the bigger set is being treated like the set needs to be sorted as strings. My first thought is don't sort by an empty set. What is the use case that you would need to do a sort with nothing? Seems like if the setup to sort by (the 2nd arg to computed.sort, sortDefinition) is empty don't call computed sort?

[onechiporenko]

Em.computed.sort takes two parameters - array to sort and a dependent key to an array of sort properties.

Ember.Controller.extend({
   list: [{a: 1}, {a: 2}, {a: 3}, {a: 4}, {a: 5}, {a: 6}, {a: 7}, {a: 8}, {a: 9}, {a: 10}, {a: 11}, {a: 12}, {a: 13}, {a: 14}, {a: 15}, {a: 16}, {a: 17}, {a: 18}, {a: 19}, {a: 20}],
   sortBy: [],
   sorted: Em.computed.sort('list', 'sortBy')
});

In this case sorted is:

[{a: 11}, {a: 1}, {a: 3}, {a: 4}, {a: 5}, {a: 6}, {a: 7}, {a: 8}, {a: 9}, {a: 10},  {a: 2}, {a: 12}, {a: 13}, {a: 14}, {a: 15}, {a: 16}, {a: 17}, {a: 18}, {a: 19}, {a: 20}]

11 is set first and 2 is set between 10 and 12

---

Another example:

Ember.Controller.extend({
   list: [{a: 1}, {a: 2}, {a: 3}, {a: 4}, {a: 5}, {a: 6}, {a: 7}, {a: 8}, {a: 9}, {a: 10}],
   sortBy: [],
   sorted: Em.computed.sort('list', 'sortBy')
});

In this case sorted is:

[{a: 1}, {a: 2}, {a: 3}, {a: 4}, {a: 5}, {a: 6}, {a: 7}, {a: 8}, {a: 9}, {a: 10}]

---

So, small collection wasn't changed but big one was changed.
It doesn't look like it's sorted like strings.

---

I've checked my example in few browsers and found out next:

Windows 8:
Chrome 48.0.2564.109 m - sorted array is 11,1,2,3,4,5,6,7,8,9,10,2,12,13,14,15,16,17,18,19,20
Opera 35.0.2066.37 - sorted array is 11,1,2,3,4,5,6,7,8,9,10,2,12,13,14,15,16,17,18,19,20
Firefox 43.0.1 - sorted array is 1,2..,19,20 (equal to the initial one)

Ubuntu 15.10:
Chrome 48.0.2564.109 (64-bit) - sorted array is 11,1,2,3,4,5,6,7,8,9,10,2,12,13,14,15,16,17,18,19,20
Opera 35.0.2066.68 - sorted array is 11,1,2,3,4,5,6,7,8,9,10,2,12,13,14,15,16,17,18,19,20
Firefox 44.0.2 - sorted array is 1,2..,19,20 (equal to the initial one)

---

What is the use case that you would need to do a sort with nothing?

I have ember-addon (ember-models-table) that is a table with multi-columns sorting. And initially table is not sorted by any column. In this case I expect that data is shown in order that it is in the array. But its order is changed when Ember try to sort it by 'nothing' .

[mmun]

@onechiporenko Thanks for reporting this. The current behaviour is definitely incorrect. If the sort properties are empty it should either throw an error or simply not sort the array. The latter option seems much better.

[mmun]

A bit more details... The current implementation is effectively doing

[1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20].sort(() => 0);

which surprisingly returns

[11, 1, 3, 4, 5, 6, 7, 8, 9, 10, 2, 12, 13, 14, 15, 16, 17, 18, 19, 20]

in Chrome (though not in Safari or Firefox) because Array#sort is not guaranteed to be a stable sort.

Regardless, running Array#sort when we don't need is pointless. I'm happy with @onechiporenko's bugfix PR.

[rwjblue]

I guess I don't really understand. Why would computed.sort be stable when Array#sort isn't? Why use computed.sort at all if you don't need to sort?

[onechiporenko]

@rwjblue, I'm working on addon called ember-models-table. It is a table where user is able to sort its content by column (one or many). This sorting is done with Ember.computed.sort (here). But initially table-data is not sorted and sortProperties is an empty array. So, I expect that content won't be changed if there is no fields to sort it. Actual behavior is in the ember@1.13.* and it was even in the ember@2.* (as I see in the my travis builds). But suddenly this behavior was changed.

[stefanpenner]

Lets see if I understand this correctly, @mmun et al, feel free to let me know if I missed a step

given:

const User = Ember.Object.extend({ 
  bar: Ember.computed.sort('foo', 'propsToSort')
})

let foo =  ['c','b','c'];
let user = User.create({ foo })

deepEquals(user.get('bar'), foo); // proposed
sometimesDeepEquals(user.get('bar'), foo); // current which depends on unstable sort

We likely expect bar to mirror foo's ordering, until such time as propsToSort changes. (which is I believe what @onechiporenko PR is changing) I believe I am +1 on that change.

[rwjblue]

The current behavior makes more sense to me (do whatever Array#sort does), and l I don't think this is specified behavior for computed.sort (it isn't documented to be a noop if no sort properties exist).

However, if we decide that this was a breaking change for a scenario that we support I can get behind the PR, but I'd prefer if we deprecate/warn when calling sort without sort props to make it clearer that this is not a good use case for computed.sort.

[stefanpenner]

@rwjblue although not documented, I think it is least surprising for no-sort properties to equate to no change in the ordering from input. We may need to consider this a breaking change over a bugfix, at which point we may need a release plan.

[rwjblue]

Not sure if it is least surprising. I would expect computed.sort to behave the same as Array#sort, but be aware of changes in the upstream array.

Like I said, I'm fine with this being something we decide is supported behavior, it just seems odd to me. We either need to fully document that computed.sort is a noop with an empty sort props array, or deprecate this behavior. I am not a fan of the idea that every thing that has ever happened to work must be supported forever. If this is a thing we want to support, then let's document it. If this is a thing we must support because it is considered a breaking change (but we would prefer not to diverge from native Array#sort behavior) then we should fix and deprecate when sort props is an empty array.

[mmun]

@rwjblue I strongly believe that sortProperties being falsy OR an empty array ought to be a no-op AND that it should be a documented behaviour. @onechiporenko's table example seems like a natural motivating use case to me.

[stefanpenner]

Not sure if it is least surprising. I would expect computed.sort to behave the same as Array#sort, but be aware of changes in the upstream array.

I can buy this, but today this also doesn't happen.

sort without comparator, i belive coerces args to strings, then compares them

let me illustrate

[1,2,3,4,5,6,7,8,9,10,11].sort() => [1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9] // <- what a "default sort would do"
[1,2,3,4,5,6,7,8,9,10,11].sort(() => 0) [6, 1, 3, 4, 5, 2, 7, 8, 9, 10, 11] // <-- basically what we do today

Before @mmuns patch:

1. if Array.isArray(sortProps) === false, no sort (but we forgot a slice)
2. if Array.isArray(sortProps) === true, always sort (same algo i believe as today)

After @mmuns patch:

1. always sort (same algo i believe as today)

And I am unsure what the behavior was in 1.13, but it may be different again. We should likely also take a look at 1.13.x behavior, just so we can get a full picture.

The proposed change would extend the "no sort" clause to be basically Ember.isEmpty(sortProps) creating yet another permutation

[mmun]

Comparing Array#sort to computed.sort is a bit subtle. The API analog of

• [1,2,3].sort() is Ember.computed.sort('arrayKey')
• [1,2,3].sort(fn) is Ember.computed.sort('arrayKey', 'sortFn') where the sortFn property contains a function

There is no analog to Ember.computed.sort('arrayKey', 'sortProperties'). Therefore we can define our own semantics for what an empty array of sortProperties means.

[rwjblue]

Like I said, I'm fine with this being something we decide to support. It should be covered with explicit tests and documented so I can't get confused in the future. 😸

[stefanpenner]

@rwjblue totally. This is obviously a place where we underspecified and are now paying the price.

[Ghostavio]

any status change on this?

[pixelhandler]

@rwjblue @stefanpenner @mmun adding the label "Bug" for now, looks like the PR is in progress

[vinilios]

I confirm the inconstant behaviour, here is another example to showcase the issue

https://ember-twiddle.com/4a1d8e3aa1bef71dd657431fd59cb445

it seems that array listeners are only set when sort properties array is not empty.

[vnazarenko]

Any news?

[cbou]

I have a similar issue, I'm not sure if I need to open a new issue.

I have a list that I sort twice. Once by date and then a second time in order to get the folder at the top of the list (I know I should use two lists ;) ). I noticed that the second sort is modifying the sort of the first one even if the sort function always returns 0.

The two following codes does not produce the same result, to me they should. The first snippet produces the correct results.

		// Sort according to user selection
		sortedItems: Ember.computed.sort('filteredItems', 'sortBy'),
		
		finalItems: Ember.computed.alias('sortedItems')

		// Sort according to user selection
		sortedItems: Ember.computed.sort('filteredItems', 'sortBy'),

		// Ensure that folders are placed on top
		separatedItems: Ember.computed.sort('sortedItems', function(a, b) {
			return 0;
		}),
		
		finalItems: Ember.computed.alias('separatedItems')

Should I open a new issue with a twiddle?