fix(ngRepeat): do not sort object keys alphabetically

[caitp]

BREAKING CHANGE:

Previously, the order of object properties was consistent and simple. Now, it's not guaranteed to
be any particular order at all.

The best approach is to convert Objects into Arrays by a filter such as https://github.com/petebacondarwin/angular-toArrayFilter
or some other mechanism, and then sort them manually in the order you need.

Closes #6210

[PatrickJS]

+1
Angular core really should have toArray if you guys keep pushing it as best practice. I would also have $key default off rather than on if you guys use that module

angular.toArray(object);

<li ng-repeat="item in itemHash | toArray">
 {{ item }}
</li>

[petebacondarwin]

Thanks @caitp!

We need to refactor those tests so that they don't rely on any particular ordering. Perhaps so that we iterate over the object in the tests as well so that the ordering differences per browser are cancelled out?

We also need to update the ngRepeat docs to make this change clear.

[petebacondarwin]

Also we need to think about sorting an object whose properties are not objects. For instance, if you had the following object:

var obj = {
  a: 'A',
  b: 'B',
  c: 'C'
};

Then you can use the toArrayFilter but not with the $key property. So you can only convert it to:

expect(toArrayFilter(obj, false)).toEqual(['A', 'B', 'C']);

If you try to attach the $key property it will error, since we can't attach properties to strings, etc.

We can't solve this inside the toArrayFilter, since if we create new objects such as {$key: 'a', $value: 'A'} then our filter is no longer stable and we get an infinite digest error.

[lgalfaso]

The JavaScript spec does provide any warranties on the iteraration
order (and there are even some discrepancies between browsers
implementations).

I think we should not merge this

[caitp]

We need to refactor those tests so that they don't rely on any particular ordering. Perhaps so that we iterate over the object in the tests as well so that the ordering differences per browser are cancelled out?

The JavaScript spec does provide any warranties on the iteraration
order (and there are even some discrepancies between browsers
implementations).

The approach taken is actually entirely safe, despite not being spec'd --- It gets complicated if you mix elements with properties, but for strictly properties set at creation time, there is no engine which will get this wrong.

Needless to say, the documentation could use an update, but someone else can worry about that

---

Edit:

Actually, enumeration is pretty normatively defined at this point --- surprisingly not in Annex B, but the behaviour of the enumeration order is essentially what browsers implement now (rather, the enumeration order isn't strictly defined, but enumeration order generally matches [[OwnPropertyKeys]])

[petebacondarwin]

Some more grist for the mill:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#Array_iteration_and_for...in

[petebacondarwin]

and https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/delete#Cross-browser_issues

[caitp]

I know all about this stuff. The tests are safe, so if you want to fix out, push the button

[petebacondarwin]

@caitp - I believe that what you are saying is that since we are simply assigning object properties we will not fall foul of browsers failing to order the properties as we expect.

We still need to update the docs for ngRepeat to explain what people are letting themselves in for when iterating over an object's properties. It is a bit of a cop out to say this is someone else's problem.

Moreover, we need to be clear in both the docs and tests exactly what the situation is with current browsers; why we can use the tests that we have but can't rely on order in general. In other words, that just about all browsers maintain the order in which the properties are added but that some browsers, such as IE, may not respect assignment ordering if you are re-assigning a property that was previously deleted, yes? Any other cases?

This is important to prevent people from complaining in the future if things don't work out as expected.

We should not merge this PR until these items are in place.

[caitp]

I think the docs update can be done seperately

[petebacondarwin]

@caitp - we cannot land a PR with a breaking change to the way a directive works without including a suitable update to the documentation. If you do not want to do this then I will update the appropriate sections at some point after New Year as part of merging this - unless someone else wants to offer a docs update PR to merge in with this change.

Also what do you think of splitting ng-repeat into two more specific directives: #6210 (comment)?

[caitp]

Also what do you think of splitting ng-repeat into two more specific directives

If someone shows me a use case which is widely needed, then I'm all for it

[frfancha]

@petebacondarwin I find your suggestion to split ng-repeat in ng-repeat for arrays and ng-object for objects an excellent one.
Exactly as you say, it will enable clean and clear code for both instead of complex testing.
Clean and clear code is ALWAYS better.

I also agree with @caitp ''but honestly it's not really helping you do anything, and it's a really weird thing to depend on.'' regarding the remove of the sort.
But... I don't think the sort is really better: who would need to rely on alphabetical order for putting elements in the DOM?? While it is true that, as there is no spec on order of object properties, nobody should develop a serious project relying on a specific order, as alphabetical is not really useful, removing the sort and document the order as being "browser dependent" is not so bad.

[petebacondarwin]

I added documentation and landed. Thanks to everyone who has had input in this issue.