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

Light-weight async.parallel for fetcher's single use-case #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Light-weight async.parallel for fetcher's single use-case #68

wants to merge 1 commit into from

Conversation

jlogsdon
Copy link
Contributor

A solution for #36

Mucked around a bit with rendr-app-template and it seemed to work fine.

@spikebrehm
Copy link
Member

Cool, thanks. It would be great to pull this little snippet into its own NPM module, but until we can get Browserify working for better packaging (so each app doesn't have to explicitly list it as a dependency), this looks reasonable.


if (err) {
callback(err, results);
callback = function() { };
Copy link
Member

Choose a reason for hiding this comment

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

Why set this to empty?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is an alternative to breaking out of the loop?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to bail early and stop iterating through the callbacks, in case we're doing some extra work in the tasks that we should avoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see that. Would be nice if _.each had an easy way to break out of loops.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, breaking out won't really help. The asynchronous nature means we could already be through the loop before an error occurs. Checking for it would require tracking the state somehow, and seems like over engineering the problem.

I could see not clearing the callback and continuing to collect results so it fires off again at the end. That may be strange behavior, though, as the callback could get used more than once (even more than twice).

Copy link
Member

Choose a reason for hiding this comment

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

I'll merge this in for the next minor version release, because removing
the async dependency may break the build step of any apps that try to
bundle it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, rendr-app-template will need an update to the Gruntfile (although it is locked to the rendr release it works with, so that's good).

@jlogsdon
Copy link
Contributor Author

Cool, thanks. It would be great to pull this little snippet into its own NPM module, but until we can get Browserify working for better packaging (so each app doesn't have to explicitly list it as a dependency), this looks reasonable.

Sounds good.

A quick note on pretty much all of your comments (which I will address individually): the implementation I made is an exact drop-in for async.parallel, mimicking all of its behavior. If we kept this as a local-only replacement I'd be fine with making the behavior even more specific, but if we want to NPM-ize it later I think it makes sense to keep the functionality as close to async's as possible.

@c089
Copy link
Member

c089 commented Feb 12, 2014

@jlogsdon sorry this was open for so long - if you're still interested in working on this I promise we'll get to it this time :) Browserify is now in as well, so you could do the npm module spike suggested. Otherwise please let me know so I can close here (trying to manage the remaining open PRs).

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

Successfully merging this pull request may close these issues.

3 participants