fix(Angular.js): fix isArrayLike for extends

[jackcviers]

Add spec for isArrayLike
Change element detection to use Node.hasChildren
Add Arguments detection
Prepares a single boolean return instead of several if return boolean
statements

1. Convert if-return boolean statements to single boolean return.
2. Assign boolean conditionals to break up the long boolean return
   and avoid repeated calls.
3. Declare variables at the beginning of function to avoid hoisting errors.
   Break long lines into indented and aligned expressions.

#4751

[petebacondarwin]

What is the performance impact of doing all the calculations up front rather than short-circuiting quick cases?

[petebacondarwin]

+1 for the unit test.

[jackcviers]

@petebacondarwin None of the cases involved require traversal. The return and boolean statements are set up to short-circuit with ORs as much as possible. If you'd like me to create a performance spec I can do that.

Interestingly, IE8 on win 7 is returning a bunch of

TypeError: Object doesn't support this property or method 

errors in the Travis Build. I can download an IE8 vm and investigate, but if you know if the build was failing before this pr, that would be immensely helpful. I didn't get any errors using --browsers Chrome,Firefox. I dual boot Win 8.1, so I can test in IE10 on that side. If it is related to this change, I suspect it is a missing native forEach on the object in question.

[petebacondarwin]

You call all these functions before you begin to short-circuit:

isDefined(obj)
isArray(obj)
isString(obj)
isObject(obj)

Plus these may also get called:

obj.hasChildNodes()
obj.hasOwnProperty('length')
obj.hasOwnProperty('callee')

[petebacondarwin]

BTW IE8 doesn't always have hasOwnProperty - http://stackoverflow.com/a/8159434/287070

[jackcviers]

No problem, I can polyfill.
On Nov 9, 2013 9:54 AM, "Pete Bacon Darwin" notifications@github.com
wrote:

BTW IE8 doesn't always have hasOwnProperty -
http://stackoverflow.com/a/8159434/287070

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4855#issuecomment-28129990
.

[jackcviers]

Haven't worked in it in over two years.
On Nov 9, 2013 10:04 AM, "Jack Viers" jackcviers@gmail.com wrote:

No problem, I can polyfill.
On Nov 9, 2013 9:54 AM, "Pete Bacon Darwin" notifications@github.com
wrote:

BTW IE8 doesn't always have hasOwnProperty -
http://stackoverflow.com/a/8159434/287070

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4855#issuecomment-28129990
.

[jackcviers]

http://jsperf.com/angular-isarraylike-performance

The current implementation is faster. But it doesn't identify arguments objects correctly.

Want me to plug an early return into that setup as well?

[jackcviers]

Added the early exit impl test.
http://jsperf.com/angular-isarraylike-performance

I am seeing that the new impl is 59% slower than the current broken implementation, and the early exit version of the new impl is 44% slower than the current broken implementation.

I don't know that the project preferences are, but the removal of the if-return boolean doesn't seem necessary for a 15% increase in correct performance.

I am adding the hasOwnProperty polyfill, assuming there isn't one already in exsitence.

[caitp]

I feel like these long expressions are actually much harder to mentally parse/understand, and you also lose the locality of reference benefit (everything is evaluated at the head, but used a full page down). And these may be a big part of the performance degrading.

[jackcviers]

@caitp @petebacondarwin I have added a hasOwnProperty(obj, propertyName) that delegates to the native one if the object has the native method or calls the Object.prototype.hasOwnProperty in the scope of the object passed with the propertyName if it doesn't. Should fix the IE<9 issue.

I have also reworded the specifications and specification local variable names to make them clearer to understand.

[caitp]

@jackcviers: You can use Object.prototype.hasOwnProperty.call() instead of obj.hasOwnProperty() to work around issues with IE < 9 host objects. No polyfill is necessary

[jackcviers]

@caitp
| @jackcviers: You can use Object.prototype.hasOwnProperty.call() instead of obj.hasOwnProperty() to work around issues with IE < 9 host objects. No polyfill is necessary.

Yeah, I'm testing to see if obj.hasOwnProperty is defined, and if not calling Object.prototype.hasOwnProperty.apply(obj, [propertyName]). However, Travis indicates massive failures. @petebacondarwin what do I need to add to run my local thru IE10? I get missing plugin notifications when trying --browsers Chrome,IE_10 I have local ie vms as well and there is an available karma ie vms plugin, should I include that?

[caitp]

http://jsfiddle.net/tUNnY/ — Try it in IE10, Chrome, Safari, Firefox, etc.

• Example with IE8 http://i.imgur.com/42zzaai.png
• Updated example which uses an object without hasOwnProperty (on IE): http://jsfiddle.net/tUNnY/4/ (IE8: http://i.imgur.com/cAlz5fU.png)

Caitlin Potter
snowball@defpixel.com

On Nov 10, 2013, at 9:49 PM, Jack Viers notifications@github.com wrote:

@caitp
| @jackcviers: You can use Object.prototype.hasOwnProperty.call() instead of obj.hasOwnProperty() to work around issues with IE < 9 host objects. No polyfill is necessary.

Yeah, I'm testing to see if obj.hasOwnProperty is defined, and if not calling Object.prototype.hasOwnProperty.apply(obj, [propertyName]). However, Travis indicates massive failures. @petebacondarwin what do I need to add to run my local thru IE10? I get missing plugin notifications when trying --browsers Chrome,IE_10

—
Reply to this email directly or view it on GitHub.

[jackcviers]

@caitp Yeah, that still fails on Travis. Need to get a local hook into Karma for these tests for IE vms so that I can see what is going on. I have tried exporting IE_BIN. Local when I try with grunt test --browsers Chrome,IE (or IE_10) that it is missing a plugin. Anyone on the project that can point me in the correct direction, help is welcome.

[caitp]

@jackcviers you need the karma-ie-launcher module installed, it isn't an angular.js package dependency so you have to manually install it.

npm install karma-ie-launcher

[caitp]

My variation on your patch is passing CI: https://travis-ci.org/caitp/angular.js/builds/13790008 --- Feel free to review it.

It requires a change to angular.forEach because isArrayLike ends up returning false for Host objects where it otherwise wouldn't, and Host objects don't provide hasOwnProperty on IE8. Might be able to fix it so that it doesn't return true for non-POJO objects with length === 0 property.

diff

[jackcviers]

I think that's fine, @caitp. I will pull your change over tomorrow. Thanks
for your help.
On Nov 10, 2013 10:56 PM, "Caitlin Potter" notifications@github.com wrote:

My variation on your patch is passing CI:
https://travis-ci.org/caitp/angular.js/builds/13790008 --- Feel free to
review it.

It requires a change to angular.forEach because isArrayLike ends up
returning false for Host objects where it otherwise wouldn't, and Host
objects don't provide hasOwnProperty on IE8.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4855#issuecomment-28174075
.

[jackcviers]

@caitp @petebacondarwin This now passes on Travis. @caitp thanks for your help. The version you gave me didn't pass on my local for some reason. This one does. Please let me know if you have any questions.

[caitp]

@jackcviers any chance we could get this patch cleaned up a bit? I was a bit surprised to learn this morning that it hasn't been merged yet. Lets make it merge-able!

[jackcviers]

Sure, I am on it.
On Dec 4, 2013 9:13 AM, "Caitlin Potter" notifications@github.com wrote:

@jackcviers https://github.com/jackcviers any chance we could get this
patch cleaned up a bit? I was a bit surprised to learn this morning that it
hasn't been merged yet. Lets make it merge-able!

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4855#issuecomment-29812353
.

[caitp]

Actually in hindsight, this probably isn't what I was surprised to learn today, it seems that we are calling hasOwnProperty on an object which does not necessarily implement it somewhere in the core code, although it's difficult to know that for sure without seeing the code causing the error.

But just the same, if this does offer some improvements, it would be nice to squish this patch down as small as possible and try to get some decent performance out of it.