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

Mapping over Objects #1177

Merged
merged 5 commits into from
Jun 5, 2016
Merged

Mapping over Objects #1177

merged 5 commits into from
Jun 5, 2016

Conversation

aearly
Copy link
Collaborator

@aearly aearly commented May 31, 2016

WIP. Closes #1157.

I might add mapKeys and family later, but I can't contrive an example use-case where you'd need to map the keys of an object asynchronously.

@aearly aearly added this to the 2.0 milestone May 31, 2016
async.mapValuesLimit(obj, 2, function (val, key, next) {
running++;
async.setImmediate(function () {
expect(running).to.equal(concurrency[key]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BTW, I'm experimenting with this way of verifying concurrency. It should be more reliable than asserting a concurrency after some timeout value. Could help with some of our flaky tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, let me know if you want testingbot keys to try running in selenium

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We'd have to refactor other tests to be like this style, tracking concurrency and verifying things finish in a certain order, but it might be worth it 😓

eachfn(arr, function (value, index, callback) {
iteratee(value, function (err, v) {
results[index] = v;
var idx = keys ? indexOf(keys, index, 0) : index;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this shouldn't be necessary. Just keep track of by creating and incrementing a numeric index variable (idx in the eachfn

var idx = 0;
eachfn(arr, function(...) {
   idx += 1;
   iteratee(...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. Fixed in 611a442

var assert = require('assert');

describe('mapValues', function () {
var obj = {a: 1, b: 2, c: 3};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth testing with an array as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What would the expected behavior be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

{0: <val>, 1: <val>, 2: <val>}

I would also be fine with leaving this behaviour implied

@megawac
Copy link
Collaborator

megawac commented Jun 3, 2016

LGTM /cc me when you feel done with this PR and I'll review again+merge

@aearly
Copy link
Collaborator Author

aearly commented Jun 5, 2016

It's ready to merge as-is. I could add mapKeys, but as I said, I'm struggling coming up with a real-world-ish example where you'd want to iterate and update the keys of an object asynchronously.

@megawac megawac merged commit 872cb91 into master Jun 5, 2016
@aearly aearly deleted the map-objects branch June 5, 2016 23:50
hargasinski pushed a commit to hargasinski/async that referenced this pull request Jun 29, 2016
@RobertWHurst
Copy link

RobertWHurst commented Oct 28, 2016

I thanks think of plenty. For one, an object containing urls.
I'm not sure why you decided to remove this.

async.map({
  resource1: 'http://resource1.url',
  resource2: 'http://resource1.url'
}, (url, cb) => fetchResource(url, cb), (err, results) => { ... });

Update: If anyone is curious about how to continue mapping objects, there is mapValues instead.

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.

None yet

3 participants