Adding forEachOf to Iterate Objects #168

Merged
merged 2 commits into from May 19, 2015

Conversation

Projects
None yet

So, I needed the ability to iterate objects (and get the key passed into the iterator) and noticed it was not done because it would break compatibility with the Node.js standard libs. I am proposing this "convention" of adding the Of suffix on iterators that deal with objects. (instead of arrays)

If this entire concept is rejected outright, then I'll stop here. However, if this seems like a good idea/convention I can finish up adding the same concept to the other various collection iterators.

Usage:

async.forEachOf({ a: 1, b: 2 }, function (value, key, callback) {
    // value = 1, 2
    // key   = a, b
}, function (err) {
    // complete
});

tleach commented Jan 18, 2013

+1 for what you're trying to do here. It would be awesome if async's collection methods were like those in Underscore.js and supported both Objects and Arrays as their primary arguments.

My suggestion would be to rework this slightly to incorporate Object directly support into forEach itself (using a bit of type checking) rather than introducing another function.

Would also be cool to add Object support to map.

I was going back and forth between creating a new function and modifying the existing one. Mostly didn't want to mess with the existing functions to keep from breaking things. But I suppose that's what unit tests are for, so I'll go ahead and make that change.

I also figured I would hold off on the other functions like map until it was clear whether or not this would ever land in master. However, it's probably not too much extra work so I'll go ahead and do it. Who knows, it may be more likely to land when it's complete. :)

tleach commented Jan 19, 2013

Cool. Another idea I had was that you should be able to optionally accept the item key in your iterator.
i.e. Both if these should be possible:

var fruitColors = {"apples": "green", "bananas": "yellow"};

// Outputs:
// green
// yellow
async.forEach(fruitColors, function(value, cb) {
    console.log(value);  
});

// Outputs:
// apples are green
// bananas are yellow
async.forEach(fruitColors, function(value, key, cb) {
    console.log(key + " are " + value);  
});

It should be possible to determine whether or not to pass the key by checking the length of the iterator function.

Again this is very much inspired by how Underscore.js works. I might hack on your branch a little if I get time.

Owner

caolan commented Feb 5, 2013

I'd lean towards overloading async.forEach etc too. Although, underscore.js has it easier here since it doesn't need to append a callback to the list of arguments. So we're forced to include the 'key' argument in any iterator definition - though I assume that's going to be the norm when using objects anyway.

Contributor

brianmaissy commented Feb 6, 2013

we're forced to include the 'key' argument in any iterator definition

@caolan Why can't we rely on arity to know whether to call iterator(value, cb) or iterator(value, key, cb)? Although I agree that providing both the key and value is almost always going to be useful. Furthermore I'm not sure it's obvious that if I were to require only one, the value would be the one to provide, and not the key. Better to explicitly provide both all the time. And as a side note, I would recommend the ordering iterator(key, value, cb) over iterator(value, key, cb)

Would also be cool to add Object support to map.

I know Underscore does this, mapping objects to arrays, but it strikes me as strange, like some sort of a type error. I would rather explicitly request an array of keys and then map over that. Anyone else feel the same way or is it just me?

Owner

caolan commented Feb 6, 2013

@brianmaissy we could rely on arity but it's not backwards compatible. JavaScript functions do not have to define all arguments and you can call them with fewer arguments than they were defined with too. For example, fs.readFile can take two or three arguments depending on whether you specify encoding. In our case we'd risk passing the index or key as the encoding in a case like this, meaning we'd always have to wrap the function instead of using it directly. Thankfully, it's actual definition is fs.readFile = function (path, encoding_) {, giving it an arity of two... but that's more by luck than judgement. I'm not saying it's out of the question, but it does require careful consideration.

Owner

caolan commented Feb 6, 2013

@brianmaissy I also agree with your suggested order of arguments: iterator(key, value, cb)

Edit: I could actually go either way on this since I assume the proposed API is based on the JavaScript forEach method which passes: value, index, array

Contributor

brianmaissy commented Feb 6, 2013

@caolan Ah ok now I see the issue. That is messy. I think passing the key and value all the time is clearer anyway.

@dominicbarnes Are you still working on this?

I was just about to sit down and work on this, which direction should I go ahead and go? Sounds like the consensus is to overload async.forEach and friends, and possibly also implementing passing the index/key for both arrays and objects.

Since my original PR here took the alternate approach, perhaps I should create a new branch, and a new PR for the discussion to continue there?

Owner

caolan commented Feb 7, 2013

@dominicbarnes changing the arity of iterators for forEach etc. would be a real pain now. I think your original suggestion was good, let's implement new functions such as forEachOf since variable arity could also get confusing.

Owner

caolan commented Feb 7, 2013

@dominicbarnes and if forEachOf could support arrays too then it deals with people asking for the index to be passed to iterators as well :)

Roger that @caolan, I'll try and finish fleshing out these changes before the end of this coming weekend. :)

brianmaissy referenced this pull request Feb 7, 2013

Closed

pass index to forEach #144

Alright, so far I've implemented: forEachOf, forEachOfSeries, forEachOfLimit

@caolan Would you review what I have so far? Also, should I continue and implement the same behavior for map, filter, reject, etc? (I'm guessing not in order to keep this PR as simple as possible, adding all of those variants will make this a pretty hefty PR)

olalonde commented Jun 3, 2013

Oops, seems I have been working on the same thing. #321. I did not implement it as a different function though, I simply check the function arity.

@brianmaissy we could rely on arity but it's not backwards compatible.

IMO, it would be worth breaking backwards compatibility for the sake of elegance and following the Javascript forEach convention. Would it be such a big deal if this was part of a major release? In addition, I doubt that so many people use async.forEach with 3 arguments functions anyways.

Contributor

brianmaissy commented Jun 7, 2013

You're right, we could change it at a major release. But how and when that happens is caolan's call

Any update on this?

@caolan what is status of this?

lnwdr commented Feb 24, 2014

+1 This would be really helpful. What's the status?

+1

I've just started investigating this library and this particular issue is a showstopper. Shame. I'm trying to load a paginated list of items from a redis database and need to asynchronously load the items but preserve order. .eachSeries will have the performance of a blocking IO and all I need is the key/index in the .each (or equivalent).

Workaround:

async.each(Object.keys(res), function (key, cb) {
  var val = res[key];
  console.log(val);
  cb();
}, cb);

PS: Won't iterate properties on prototype.

Owner

caolan commented Mar 28, 2014

I realise this is now ancient history, but if someone wants to add docs and make sure the tests still pass I'll merge this. I don't like overloading the each/eachSeries to handle keys or indexes so this seems like the best approach. Personally, I just iterate over Object.keys().

allain commented Jun 7, 2014

forIn? like in lodash

+1 How is this feature progressing?

I would keep the js convention "iterator(key, value, callback)" and always pass the three arguments along. It makes sense to me to change the current implementation of the "each" family, without creating duplicates, although I understand it could break old code. This would happen only if someone updates the library though, and it could be made clear in the docs that it's a major realease with potentially breaking changes.

nylen commented Nov 4, 2014

+1, I might be able to help document and test this sometime this week.

aearly referenced this pull request Jan 20, 2015

Merged

Completing forEachOf #705

I know there is a workaround, and I know this is free software etc., but I humbly vote +1 for this to be merged...

+1

I made a PR here a while ago: #321 but modified each instead of making a new eachOf method. I could move the code into a standalone eachOf method if there is still interest and @caolan agrees to merge :).

Well, last year @caolan wrote:

I realise this is now ancient history, but if someone wants to add docs and make sure the tests still pass I'll merge this. I don't like overloading the each/eachSeries to handle keys or indexes so this seems like the best approach. Personally, I just iterate over Object.keys().

So, I am pretty sure he won't say "no" to a pull request that doesn't overload each!

Collaborator

aearly commented Mar 31, 2015

Waiting on #705, which complete this....

@aearly aearly merged commit 2a13d08 into caolan:master May 19, 2015

@aearly aearly added a commit that referenced this pull request May 19, 2015

@aearly aearly Merge pull request #705 from aearly/for-each-of
Completing forEachOf.  Also closes #168 and #321
57cf750

you guys need to change the docs, please, there is still a reference to async.forEachOf in the readme.md file

Collaborator

aearly commented Oct 7, 2015

@the1mills what do you mean? forEachOf is in later version of async.

i am on version "version": "0.9.2",

Collaborator

aearly commented Oct 7, 2015

0.9.2 does not have forEachOf.

Contributor

ORESoftware commented Oct 8, 2015

Yup, and forEachOf is in the docs or at least the readme.md file that shows up in this repo

Contributor

ORESoftware commented Oct 8, 2015

is there a newer version thant 0.9.2...

Collaborator

aearly commented Oct 8, 2015

Yes, latest is 1.4.2.

The docs for 0.9.2 can be found here: https://github.com/caolan/async/tree/0.9.2

Contributor

ORESoftware commented Oct 8, 2015

whoops thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment