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

Add async.tryEach #1365

Merged
merged 1 commit into from
Apr 7, 2017
Merged

Add async.tryEach #1365

merged 1 commit into from
Apr 7, 2017

Conversation

faridnsh
Copy link
Contributor

@faridnsh faridnsh commented Feb 7, 2017

This adds async.tryEach which fixes what was requested in #687. I needed something with similar requirements. My use case was that I needed to get some data, which I could get it from 3 websites(with slightly different code for each) but sometimes they go down randomly.

The name might be reconsidered. We can't use try because it's a reserved word. I couldn't think of anything better.

You guys might want to also review the documentation, English is not my first language.

As you can see I wrote tests covering most cases, and the coverage is 100% for the file.

@megawac
Copy link
Collaborator

megawac commented Feb 8, 2017

Could this not also be accomplished with find/detect? This looks extremely similar (especially if we consider committing to the proposal in #1248/#591)

@faridnsh
Copy link
Contributor Author

faridnsh commented Feb 9, 2017

It doesn't say in the documentation what happens when there's an error in detect. I believe right now it short circuits on error.
Also note that there's no data to iterate on with my use case. There's just a series of functions. That's why it's in the control flow category. It'll be slightly more code for the user to achieve this use case with detect.

lib/tryEach.js Outdated
}
error = err;
result = args;
callback(args);
Copy link
Collaborator

@hargasinski hargasinski Feb 9, 2017

Choose a reason for hiding this comment

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

Related to the discussion in #1248. Should this be callback(!err)? If the task returns an error and a partial result (e.g. {}), the function should probably move onto the next task as opposed to returning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. It should be callback(!err). I'll change it today.

@hargasinski
Copy link
Collaborator

I would be in favour of resolving #1248 before moving forward with this PR.

lib/tryEach.js Outdated
* @memberOf module:ControlFlow
* @method
* @category Control Flow
* @name series
Copy link
Collaborator

@hargasinski hargasinski Feb 9, 2017

Choose a reason for hiding this comment

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

Just an fyi, some of the JSDoc tags were duplicated here.

@faridnsh
Copy link
Contributor Author

faridnsh commented Mar 2, 2017

@hargasinski I have fixed the issues you mentioned :)

@aearly
Copy link
Collaborator

aearly commented Apr 1, 2017

I think this is really cumbersome to expect people to implement with find/some, mainly because those iterate a collection of values, rather than a collection of functions. Here's my stab at it:

function nihTryEach(fns, done) {
  var lastResult;
  var lastError;
  async.some(fns, function (fn, cb) {
    fn(function(err, result) {
      lastErr = err;
      lastResult = result;
      cb(null, !err);      
    });
  }, function (err, anySucceeded) {
    if (!anySucceeded) return done(lastError);
    done(null, lastResult);
  }
}

It's slightly more complicated than @alFReD-NSH 's implementation. I also think the concerns in #1248 wouldn't affect this implementation, we can deal with those separately.

I'd say this is good to merge as-is now. 👍

@aearly aearly merged commit 7aa90f3 into caolan:master Apr 7, 2017
@aearly
Copy link
Collaborator

aearly commented Apr 29, 2017

Released in v2.4.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants