fix(forEach): refined treatment of objects with numeric length property #1938
Conversation
|
we can't be iterating over the whole array/object to determine how to handle it. that's way too expensive. how about doing this instead: |
|
If only JQLite and jQuery objects are the "special" objects that must be treated like arrays, indeed this is way more faster. I've changed the test accordingly. However, I'm not sure how to check the jQuery condition. The unit test is only checking explicitly the JQLite one. |
|
I changed your test to use jqLite instead of JQLite - jqLite is either jQuery or JQLite depending on the environment it runs on.. this is how the test verifies that the change works for both. however there are some problems on safari and IE :-/ http://ci.angularjs.org/job/angular.js-igor/778/console this is my modified commit: b1ad7976e50d6e8de25e629d997135321a4cd6ef UPDATE: I fixed the opera failure by using value.innerHTML instead of just value updated commit is here: 1627088694fe5653d0f9e446b84a387a67ef3d46 can you debug the IE issue? |
|
PR Checklist (Minor Bugfix)
|
|
as expected only IEs are failing now: http://ci.angularjs.org/job/angular.js-igor/780/console |
|
sure, I'll look to the IE issue |
|
If logged calls to forEach in IE8 and this is what i've found: forEach is receiving objects of these types: StaticNodeList: http://msdn.microsoft.com/en-us/library/ie/hh826017(v=vs.85).aspx In IE8, none of these objects has hasOwnProperty and this produces the error. |
|
right and also HTMLCollection (in the failing $anchorScroll test). there seems to be quite a lot of these objects so whitelisting them is not a good option. now I understand why the original check was |
|
I returned to the original approach but in a less naive way. Instead of iterating over the properties, as the test takes the premise that these objects that are like full arrays, the code only checks that the last property is defined. There is, however, an edge case when length is 0. jqLite, HTMLCollection, StaticNodeList and every other object like these ones must continue to be treated like an array. The downside is that when a plain old object has a length property with a zero value its going to be treated like an array too. I've make an exception with the window object that has a zero length property. If looked to a few other frameworks, and many have have also this bug. I found that jQuery addressed it in this way. |
…like objects fixes angular#1840 when an object has a numeric length property: if an object has a length property with a number value of 0, it will be considered like an array like object if the value is positive and the object looks like a full dense array with a property named as length minus one, it will be considered like an array like object Signed-off-by: Gonzalo Ruiz de Villa <gonzaloruizdevilla@gmail.com>
|
thanks for poking at this. the last commit still doesn't cover many usecases. I was able to craft this patch which I think covers everything that we care about on all browsers: ec54712 |
fixes #1840
to iterate the object like an array, it must verify one of these rules:
length property