fix(forEach): match behaviour of Array.prototype.forEach (ignore missing properties)

[caitp]

Array.prototype.forEach will not invoke the callback function if the properety
is not present in the object. Because of this, we have the illusion of not
iterating over non-added properties in a sparse array.

From ECMAScript:

9. Repeat while k < len
    a. Let Pk be ToString(k).
    b. Let kPresent be HasProperty(O, Pk).
    c. ReturnIfAbrupt(kPresent).
    d. If kPresent is true, then
           i. Let kValue be Get(O, Pk)
           ... (steps for invoking the function and aborting if it throws)

Closes #8510 Closes #8522

[jessebeach]

So does this mean that in the following case, with this behavior in native code:

var a = [];
var a[2] = 'hello';
a.forEach(function (value) {
  console.log(value);
});
// output: 'hello'
// we don't get: undefined, undefined, 'hello'

This was the behavior of angular.forEach in 1.2.0. But as of 1.2.21, undefined/null values are no longer ignored. I came across this while trying to implement the Angular Strap datepicker, which works on the project site's example, but breaks in my implementation.

When I apply the fix in this PR, the Angular Strap datepicker functions normally again.

So +1!

[mgcrea]

👍 Indeed angular.forEach should strictly behave like Array.prototype.forEach (POLA). Regarding AngularStrap's regression, it occurred in the date-parser helper.

[petebacondarwin]

I guess the check for isArray() is for performance reasons, since isArrayLike() already returns true for arrays?

[caitp]

Okay, so the performance hit of testing if we have the profile has led to some discussion today, and which I've been thinking about internally:

• For a while, we were using native Array.prototype.forEach() in browsers supporting ES5, for Array collections. A few releases ago in 1.3, and on June 30th, Igor has altered the code so that this path is never used, essentially. So because of this, the behaviour of not skipping missing properties in the Array is essentially a regression from the POV of users.
• In angular core, which is the primary user of forEach(), we don't really depend on iterating over properties of functions, iterating over string literals, or skipping missing properties. So we're doing a lot of stuff that we don't need, and we do see perf benefits from not testing if an array has a property or not.
• This regression has caused some issues in AngularStrap, due to not following ES5 semantics. Manually testing if a key is in a collection within the iterator callback in angular strap would fix this bug, but this is quite a big performance hit, since we incur the cost of testing the property AND invoking the callback and returning abruptly, regardless. However, this is not something which happens in core, so it's probably not the end of the world to have a costly algorithm on some of the web. But it is still a regression.

From my POV, it wouldn't be that bad to use ES5 semantics in forEach() by default, and have a separate "fast-path" which test if the property is present or not, and use this within the compiler, within internal algorithms like extend(), etc.

Obviously, the cost of testing if a property is available isn't free, but if we're honest, the cost isn't all that heavy (we're setill climbing through am array with a length of 10k and only 5k indexed properties over 1000 times per second on chrome desktop, and 600 times/second on mobile --- this isn't so bad). And since this is a regression, it's probably worth fixing regardless.

We can have a fast compiler which doesn't care about sparse arrays while still supporting the expectations of angular users. This is doable --- but the decision needs to come from from someone else.

Balls in your court~

[shahata]

Just out of interest, what is the benefit users get from using angular.forEach instead of Array.prototype.forEach? Since IE8 is no longer supported I think users can use Array.prototype.forEach directly... Personally, I only use angular.forEach to iterate over objects and never use it for arrays.

[caitp]

the benefit is, we did it before, and people used it, and then we stopped doing it and broke peoples code

[jessebeach]

If I understand the method correctly, angular.forEach iterates over enumerable things: arrays and objects. Module authors need not concern themselves with the arrayness or objectness of the things they pass to it.

[IgorMinar]

it would be good to add a jsperf link to this code and show that this is really faster than alternatives (hasOwnProperty, Object.keys()).

otherwise it looks good to me.

[caitp]

Here's a perf (it uses a very large array, in both sparse and full variants) http://jsperf.com/angular-foreach-comparison

if index in array wins pretty thoroughly, but it's also the only one which is anywhere close to being semantically correct. I'll make another version which deals with more realistic array sizes too

[caitp]

http://jsperf.com/angular-foreach-comparison-small for smaller (~256) arrays, Object.keys() wins, but is quite semantically incorrect. To make it semantically correct, we'd need to perform a search within the array for the array index property that we're testing, otherwise we'll get properties that we wouldn't get normally. This search could be O(n) or O(log n) or similar, but it's still going to be bad enough to crash your browser, and would also make the code quite a bit more complex.

As far as the original semantics (and ECMAScript) semantics of the routine go, if (index in collection) wins.

[caitp]

http://jsfiddle.net/caitp/Lrz5m5xs/ and this would be an example of why the Object.keys() case behaves very differently and is semantically incorrect.

I can add each of these to the code in comments or something so that there's a note of it

[IgorMinar]

Yes. Please do.
On Aug 13, 2014 6:44 PM, "Caitlin Potter" notifications@github.com wrote:

http://jsfiddle.net/caitp/Lrz5m5xs/ and this would be an example of why
the Object.keys() case behaves very differently and is semantically
incorrect.

I can add each of these to the code in comments or something so that
there's a note of it

—
Reply to this email directly or view it on GitHub
#8525 (comment).

[caitp]

So @IgorMinar's recent perf fixes have removed a lot of calls to forEach() in particularly hot sections of angular --- maybe more of these removals will be needed, but hurting the performance of forEach() matters a bit less now, so I'm going to land this and fix the regression.

[caitp]

@IgorMinar it would be good to cherry-pick this in stable since the regression technically exists there too, but the perf fixes which removed calls to forEach() in the compiler/jqlite/etc didn't land in stable... Do you care if this gets cherrypicked without the removal of calls to forEach? Or should those end up in stable too?