Adding forEachOf to Iterate Objects

[dominicbarnes]

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]

+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.

[dominicbarnes]

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]

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.

[caolan]

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.

[brianmaissy]

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?

[caolan]

@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.

[caolan]

@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

[brianmaissy]

@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?

[dominicbarnes]

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?

[caolan]

@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.

[caolan]

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

[dominicbarnes]

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

[dominicbarnes]

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]

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.

[brianmaissy]

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

[silenceisgolden]

Any update on this?

[niftylettuce]

@caolan what is status of this?

[selfawaresoup]

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

[medatech]

+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).

[olalonde]

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.

[caolan]

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]

forIn? like in lodash

[pensierinmusica]

+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]

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

[mercmobily]

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

[mjhasbach]

+1

[olalonde]

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 :).

[mercmobily]

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!

[aearly]

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

[the1mills]

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

[aearly]

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

[the1mills]

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

[the1mills]

to prove it to you

https://www.dropbox.com/s/pk09vzl3elhgiuy/Screenshot%202015-10-07%2014.24.33.png?dl=0

[aearly]

0.9.2 does not have forEachOf.

[ORESoftware]

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

[ORESoftware]

is there a newer version thant 0.9.2...

[aearly]

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

[ORESoftware]

whoops thx