Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make _.each/_.forEach support ES6 Set/Map #2208

Closed
wants to merge 1 commit into from
Closed

Make _.each/_.forEach support ES6 Set/Map #2208

wants to merge 1 commit into from

Conversation

benjycui
Copy link

@benjycui benjycui commented Jun 6, 2015

IMHO, with the approaching of ES6, it is time to support new features. But I am not sure if you want it :-)
See also: #2147.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 96.31% when pulling 9cd7d8d on benjycui:enhance-each into d04b7e9 on jashkenas:master.

@jdalton
Copy link
Contributor

jdalton commented Jun 6, 2015

@megawac can you make the coveralls less chatty (there's an option in its config on the coveralls site).

@benjycui I don't think it's time to support new forms of iteration just yet. For example IE 11 doesn't support new Set([1, 2, 3]) for assigning values to a Set. I'm also not a fan of duck typing for iteration or adding more overloaded functionality to methods.

The easiest way to deal with this in your ES6 code is to convert your sets/maps to an array with Array.from or something like:

_.each([...set], function() {});

@benjycui
Copy link
Author

benjycui commented Jun 8, 2015

@jdalton yep, you are right. We can convert Set/Map with ....

But for Map, this solution is inconsistent with the default behaviour of _.each:

Each invocation of iteratee is called with three arguments: (element, index, list). If list is a JavaScript object, iteratee's arguments will be (value, key, list).

var map = new Map([[1, 'one'], [2, 'two'], [3, 'three']]);
_.each([...map], function (kv) {  // I think this arguments list should be: `(value, key, list)`
  console.log(kv[0], ': ', kv[1]);
});

Do you think that consistency is worthy of 2 lines of source code? :-)

@jdalton
Copy link
Contributor

jdalton commented Jun 8, 2015

But for Map, this solution is inconsistent with the default behaviour of _.each:

If you want the familiar arg sig then you can do
_.each(_.object([...map]), function() {})

Do you think that consistency is worthy of 2 lines of source code? :-)

It's not just 2 lines, you're essentially buying into the whole duck type thing which then requires modifying all other methods. Duck typing adds a bit of a wild card to the API too as you don't know how the duck typed method will really act. In this case you probably want a specialized method instead of ones designed for arrays and objects.

@jridgewell
Copy link
Collaborator

Do you think that consistency is worthy of 2 lines of source code? :-)

I agree with @jdalton here. Adding this means adding it to all collection-iteration functions. Additionally, this doesn't fix the arg signature for Set, since it passes (value, value, set) to the forEach callback.

@jdalton
Copy link
Contributor

jdalton commented Jun 8, 2015

Closing for now.

@jdalton jdalton closed this Jun 8, 2015
@benjycui
Copy link
Author

benjycui commented Jun 9, 2015

Thank you for your explanation :-)

@benjycui benjycui deleted the enhance-each branch June 9, 2015 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants