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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
"underscore": "1.4.4",
"backbone": "1.0.0",
"handlebars": "git://github.com/spikebrehm/handlebars.js.git#0687c7016c62122ab160a8683817a931b03354ad",
"async":"0.1.22",
"qs": "0.5.1",
"express": "~3.0.6",
"validator": "0.4.21",
Expand Down
1 change: 0 additions & 1 deletion scripts/postinstall.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ var root = __dirname + '/..',
var dependencies = [
'underscore',
'backbone',
'async',
'handlebars'
];

Expand Down
33 changes: 30 additions & 3 deletions shared/fetcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,10 @@ and returns an identifying object:
}
*/

var Backbone, CollectionStore, ModelStore, async, modelUtils, _;
var Backbone, CollectionStore, ModelStore, modelUtils, _;

_ = require('underscore');
Backbone = require('backbone');
async = require('async');

modelUtils = require('./modelUtils');
ModelStore = require('./store/model_store');
Expand All @@ -56,6 +55,34 @@ function Fetcher(options) {
});
}

function parallel(tasks, callback) {
var results = {},
completed = 0,
length;

if (_.isEmpty(tasks)) {
return callback(null, results);
}

length = _.keys(tasks).length;
_.each(tasks, function(task, key) {
task(function(err) {
var args = Array.prototype.slice.call(arguments, 1);
if (args.length <= 1) {
args = args[0];
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this? Is this trying to replicate behavior in async? When would there ever be more than one argument (besides err)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was simply to replicate async exactly.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, all good then.

}
results[key] = args;

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

} else if (++completed >= length) {
callback(null, results);
}
});
});
};

/**
* Returns an instance of Model or Collection.
*/
Expand Down Expand Up @@ -185,7 +212,7 @@ Fetcher.prototype._retrieve = function(fetchSpecs, options, callback) {
}
}.bind(this);
}, this);
async.parallel(batchedRequests, callback);
parallel(batchedRequests, callback);
};

Fetcher.prototype.needsFetch = function(modelData, spec) {
Expand Down