-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix(forEach): add the array/object as the 3rd param like the native forEach #7902
Conversation
@@ -617,6 +617,62 @@ describe('angular', function() { | |||
forEach(obj, function(value, key) { log.push(key + ':' + value); }); | |||
expect(log).toEqual(['length:2', 'foo:bar']); | |||
}); | |||
|
|||
describe('should follow spec with', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that you are using the describe to make the its less repetitive, but they are not really readable that way.
@Narretz I've updated the tests, let me know if that's not what you meant. I labelled this as a fix originally because it was inconsistent between arrays vs other object types (and IE8 vs other browsers). However 36625de made things much more consistent by not using |
I think it makes sense to get his in. it's a small change and having access to the original array can be useful if the iterator is not a closure that has access to the original collection. |
|
||
expect(expectedSize).toBe(0); | ||
} | ||
it('should follow spec with array', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you rename all of these too should follow the ES spec when called with array
?
9e35b50
to
bd96cf6
Compare
I've updated the test names. |
This makes all the different object types supported by
angular.forEach
more like the nativeArray.forEach
by adding the 3rd argument to the callback. Also adding tests to make sure all the different object types invoke the iterator with all 3 arguments and the context.