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

Adds support for es6 iterators #1074

Merged
merged 3 commits into from
Mar 23, 2016
Merged

Adds support for es6 iterators #1074

merged 3 commits into from
Mar 23, 2016

Conversation

ex1st
Copy link

@ex1st ex1st commented Mar 22, 2016

@ex1st ex1st mentioned this pull request Mar 22, 2016
if (item.done)
return null;
i++;
return map ? {value: item.value[1], key: item.value[0]} : {value: item.value, key: i};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is map the only time we'd like to do this switch? Also not sure if tracking indexs even makes sense for a *Map as they won't correspond to the actual index (can't do map[1] to get item.value

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a valid thing to do. I think it's odd to iterate over a map and get [key, val] as the value with an arbitrary numeric index.

@ex1st
Copy link
Author

ex1st commented Mar 22, 2016

@megawac if wan't use map's indexes it makes iterator easy - just replace to lodash.prototype.next()
What do you think?

@megawac
Copy link
Collaborator

megawac commented Mar 22, 2016

We are not going to import lodash chaining

@ex1st
Copy link
Author

ex1st commented Mar 22, 2016

@megawac corrected.

@aearly
Copy link
Collaborator

aearly commented Mar 22, 2016

This is really cool. 👍

@aearly
Copy link
Collaborator

aearly commented Mar 23, 2016

Damn, I was looking for some precedent in how to handle Maps, but niether Lodash nor Ramda have real support for ES6's new collection types yet.

It does seem weird to have a special case just for Map iterators, but on the other hand, it is a built-in data type for JS now. I also think using the Map's keys as separate arguments to iteratees will better match user expectations. It is a special case, but I think we will have just this single special case. I see it as analogous to iterating over an Object vs. an Array.

@megawac
Copy link
Collaborator

megawac commented Mar 23, 2016

I also think using the Map's keys as separate arguments to iteratees will better match user expectations.

I disagree, if they want keys they should pass map.entries() iterator instead of the map itself

var item = iterate.next();
if (item.done)
return null;
i++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like tracking this index. People should pass .entries if they want access to the key. i may not correspond to the real index. Just leave key undefined

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some sort of index is needed for map(Series|Limit) to work.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point fine with this then

@aearly aearly added this to the 2.0 milestone Mar 23, 2016
@aearly aearly merged commit 92df0ca into caolan:master Mar 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants