CODEWITHSUNDEEP
node.jsexpress
Emilio
Emilio

Reputation: 1364

Why is this code working? (NodeJS/Express asynchronous callbacks)

Is this code respecting the NodeJS async way of doing things? It's fully working, but I don't understand why the res.render is inside the Database.findOne callback and not inside the Database.aggregate callback, even if I'm using the results of both Database.findOne and Database.aggregate.

However, if I put the res.render outside both the Database.aggregate and the Database.findOne, but still inside the router.get callback, then the code won't work at all (edit: of course, in that case, I declare the variables outside the database queries). How come? What is the correct NodeJS way of doing things?

Thanks

var _         = require('lodash');
var express   = require('express');
var Database = require('../models/database');
var router    = express.Router();
router.get('/:XXXX', function(req, res, next) {
var XXXX = req.params.XXXX;

var aggregationResults;
Database.aggregate([
    // pipeline
], function(err, results) {
    aggregationResults = results;
    if (err) return next(err);
});

Database.findOne({XXXX: XXXX}, function(err, XXXXresult){
    if(err) return next(err);

    res.render('page', {XXXXresult: XXXXresult, aggregationObject : aggregationResults[0]});


});

});

Upvotes: 0

Views: 87

Answers (2)

Elliott Beach
Elliott Beach

Reputation: 11483

If you moved the res.render outside of the findOne callback, it will execute before the callbacks - you won't have any data. Those database operations are going to complete after the router (req, res, next) callback returns, after all.

In your current code, given that the aggregate and findOne functions are async, you have a race condition. If the findOne callback executes before the aggregate callback, you will get a TypeError as aggregationResults will be undefined.

The simplest way to fix this is to place findOne call inside the aggregate callback. But this is not actually a good idea because then the two functions will execute one after the other, which is slower than executing them simultaneously.

What you should really be doing is using Promises, and use something like Promise.all with 2 promises to send the response after both operations complete.

Upvotes: 1

Prakash Sharma
Prakash Sharma

Reputation: 16482

In your code you are calling two async database functions and passing a callback to each function. Here you first called Database.aggregate() and passing a callback which stores the result in a variable i.e. aggregationResults. Then you are calling Database.findOne() and passing a callback to it which sends the result of both the call.

Here it is working because your first database call (Database.aggregate()) is resolving before the second one (i.e.Database.findOne()). So in your second database's callback you are assuming that first database call's result is available.

But that may not be the case always. For example there are chances that first database might fail and second one passes. In that case there is no result of first database.

What is the correct NodeJS way of doing things?

Well earlier it used to be a nested callback but today you should use promise. Your code then will look something like this:

Database.aggregate([pipeline]).then(function(results) {
    aggregationResults = results;
    return Database.findOne({XXXX: XXXX});
}).then(function(XXXXresult){
    res.render('page', {XXXXresult: XXXXresult, aggregationObject : aggregationResults[0]});
});

You can use library like bluebird to promisify your database methods.

Upvotes: 2

Related Questions