Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(forEach): add the array/object as the 3rd param like the native forEach #7902

Closed
wants to merge 1 commit into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Jun 19, 2014

This makes all the different object types supported by angular.forEach more like the native Array.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.

@jbedard jbedard added cla: yes and removed cla: no labels Jun 19, 2014
@Narretz Narretz added this to the Ice Box milestone Jul 4, 2014
@@ -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() {
Copy link
Contributor

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.

@jbedard
Copy link
Contributor Author

jbedard commented Jul 9, 2014

@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 Array.prototype.forEach at all. I'd still say angular.forEach should try to be consistent with Array.prototype.forEach though, just to do things the standard-like way and allow for the use of Array.prototype.forEach again in the future if it becomes faster then plain loops.

@IgorMinar
Copy link
Contributor

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() {
Copy link
Contributor

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 ?

@IgorMinar IgorMinar self-assigned this Sep 8, 2014
@jbedard jbedard force-pushed the foreach-like-native branch from 9e35b50 to bd96cf6 Compare September 8, 2014 04:38
@jbedard
Copy link
Contributor Author

jbedard commented Sep 8, 2014

I've updated the test names.

@IgorMinar IgorMinar closed this in df9e60c Sep 8, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants