You are initiating a bunch of asynchronous operations inside a for loop and expecting all the asynchronous operations to be done when the for loop is done. But, in reality, none of them are yet done so your doc.comments array is not yet populated. You were trying to use it before it had been populate so that's why you see it as defined where you were trying to use it.
The best way to solve this is to learn how to use Promises and then fire multiple requests with something like Blubird's Promise.map() or ES6 Promise.all() and then let the promise engine tell you when all the requests are done.
Short of converting your database calls over to use Promises, you can hand-code to know when everything is done as follows:
Hand-coded Callback Implementation
router.get('/:id', function (req, res, next) {
List.findOne({listurl: req.params.id}, function (err, doc) {
var doneCnt = 0;
if (!err && doc != null) {
for (var i = 0; i < doc.comments.length; i++) {
(function(index) {
User.findOne({Name: doc.comments[i].commenter}, function (err, data) {
++doneCnt;
if (err) {
// need some form of error handling here
doc.comments[index].gvUrl = "";
} else {
if (data) {
doc.comments[index].gvUrl = gravatar.url(data.Email, {s: '200', r: 'pg', d: '404'});
} else {
doc.comments[index].gvUrl = 'noGravs';
}
}
// if all requests are done now
if (doneCnt === doc.documents.length) {
res.render('list', {
appTitle: doc.Title,
list: doc
});
}
});
})(i);
}
}
else {
res.status(404).render('404', {appTitle: "Book not found"});
}
});
}
This code recognizes the following things about your async operations:
- You are firing multiple
User.findOne() async operations in a loop.
- These async operations can finish in any order.
- These async operations will not be complete when the loop is done. All the loop has done is initiate the operations. They will all complete some indeterminate time later.
- To know when all of the async operations are done, it keeps a counter to count how many have completed and renders the page when the count reaches the number of total requests that were started. That's the "manual" way to how you know when all of them are done.
Bluebird Promise Implementation
Here's how it could work using the Bluebird Promises library and converting your database operations to support Promises:
var Promise = require('bluebird');
// promisify the methods of the List and User objects
var List = Promise.promisifyAll(List);
var User = Promise.promisifyAll(User);
router.get('/:id', function (req, res, next) {
List.findOneAsync({listurl: req.params.id}).then(function(doc) {
if (!doc) {
throw "Empty Document";
}
return Promise.map(doc.comments, function(item, index, length) {
return User.findOneAsync({Name: item.commenter}).then(function(data) {
if (data) {
item.gvUrl = gravatar.url(data.Email, {s: '200', r: 'pg', d: '404'});
} else {
item.gvUrl = 'noGravs';
}
});
}).then(function() {
return doc;
});
}).then(function(doc) {
res.render('list', {
appTitle: doc.Title,
list: doc
});
}).catch(function(err) {
res.status(404).render('404', {appTitle: "Book not found"});
});
}
Here's how this works:
- Load the Bluebird promise library
- Add promisified methods to the
User and List objects so it adds a new version of each method that returns a promise.
- Call
List.findOneAsync(). The "Async" suffix on the method name represents the new method that the .promisifyAll() added.
- If no
doc, then throw which will reject the promise will then be handled in the .catch() at the end. Promises are async throw-safe which is very handy for async error handling.
- Call
Promise.map() on the doc.comments. This will automatically iterate the doc.comments array and call an iterator on each item in the array (similar to Array.prototype.map() except here it collects all the promises returned by the iterator and returns a new promise that is resolved when all the underlying promises are resolved. In this way, it allows all the iterators to run in parallel and tells you when all the iterators are done.
- The iterator calls
User.findOneAsync() and sets the doc.comments[index].gvUrl value with the result.
- There's an extra
.then() handler on the Promise.map() just to change the resolved value of that promise to be the doc object so we can get at that from the outer promise handlers.
- For success from the outer promise, render.
- For error from the outer promise, show the 404 page. Keep in mind that any rejected promises anywhere in this whole scheme will propagate up and be a rejection at the top level. This auto-propagation of async errors in promises is hugely useful.
ES6 Promise Implementation
This could be done with straight ES6 promises without the Bluebird promise library, but you'd have to do a few more things by hand:
- You'd have to promisify the
List.findOne() operation.
- You'd have to promisify the
User.findOne() operation.
- You'd have to user a regular
doc.comments.map() iteration and collect each individual promise into an array and then use Promise.all() on that array instead of letting Promise.map() do all of that for you.
Here's the code:
// manually promisify findOne
List.findOneAsync = function(queryObj) {
return new Promise(function(resolve, reject) {
List.findOne(queryObj, function(err, data) {
if (err) return reject(err);
resolve(data);
});
}
}
User.findOneAsync = function(queryObj) {
return new Promise(function(resolve, reject) {
User.findOne(queryObj, function(err, data) {
if (err) return reject(err);
resolve(data);
});
}
}
router.get('/:id', function (req, res, next) {
List.findOneAsync({listurl: req.params.id}).then(function(doc) {
if (!doc) {
throw "Empty Document";
}
var promises = doc.comments.map(function(item, index) {
return User.findOneAsync({Name: item.commenter}).then(function(data) {
if (data) {
item.gvUrl = gravatar.url(data.Email, {s: '200', r: 'pg', d: '404'});
} else {
item.gvUrl = 'noGravs';
}
});
});
return Promise.all(promises).then(function() {
return doc;
});
}).then(function(doc) {
res.render('list', {
appTitle: doc.Title,
list: doc
});
}).catch(function(err) {
res.status(404).render('404', {appTitle: "Book not found"});
});
}
