Avoiding callback hell in node

Question

I have this function

/* GET main page */
router.get('/', (req, res, next) => {
    Account.find({accType: 'Employer'}, (err, col) => {
        if (err) {
            console.log(err);
        } else {
            Banner.findOne((err, banner) => {
                if (err) {
                    console.log(err);
                } else {
                    // Determine whether there is at least one awaiting account.
                    Account.findOne({accType: 'Awaiting'}, (err, doc) => {
                            if (err) {
                                console.log(err);
                            } else {
                                if (doc != null) {
                                    var awaiting = true;
                                }
                                console.log(doc);
                                res.render('admin/', {
                                    title: 'Admin pannel',
                                    user: req.user,
                                    employers: col,
                                    areAwaiting: awaiting,
                                    banner: banner,
                                )
                            }
                        }
                    );
                }
            });
        }
    });
});

I tried using the async module like this:

async.parallel(calls, (err, col) => {
    if (err)
        console.log(err);
    else {
        console.log(col);
        res.render('admin/', {
            title: 'Admin pannel',
            user: req.user,
            employers: col[0],
            banner: col[1],
            areAwaiting: col[2],
        })
    }
});

Now. The error I'm getting is wrapAsync(...) is not a function. I tried putting my functions in anonymous functions like this:

() => {
    Account.find({accType: 'Employer'}, (err, col) => {
        if (err)
            console.log(err);
        else return col;
    })
}

but then the code just freezes. I also tried this:

(col) => {
    Banner.findOne((err, doc) => {
        if (err)
            console.log(err);
        else return doc;
    });
    return col
},

but with the same effect. Any idea what I'm doing wrong? The callback hell version works, but is just ugly and not maintainable.

Answer by Dave Meehan. Had to edit it slightly to make it work.

router.get('/', (req, res) => {
    return Promise.all([
        Account.find(),
        Banner.findOne(),
        Account.findOne({accType: 'Awaiting'}),
    ]).then(([employers, banner, awaiting]) => { // Right here
        if (awaiting != null)
            var areAwaiting = true;
        res.render('admin/', {
            title: 'Admin pannel',
            user: req.user,
            employers: employers,
            areAwaiting: areAwaiting,
            banner: banner,
        });
    }).catch(e => {
        console.error(e)
    });
});

I had to close the array in then into ()

If you are on the server side (and your classes support Promises), and have latest NodeJS LTS version available, you could use native async / await language features. See https://blog.risingstack.com/mastering-async-await-in-nodejs/ — bubblez, Apr 01 '18 at 22:54
Will it work with my querries though? All the async docs show API calls — Alex Ironside, Apr 01 '18 at 22:59
If ever there was a good example of callback hell and why to use promise chaining, this is one. Switch to promises, not to the async library. — jfriend00, Apr 01 '18 at 23:04
And, chances are your database already supports promises too (it looks like it may be mongodb which does support promises already). — jfriend00, Apr 01 '18 at 23:48
I'm having hard time wrapping my head around the promise chaining. I edited my question to include my trial — Alex Ironside, Apr 01 '18 at 23:54

rtn · Answer 1 · 2018-04-02T00:20:32.187

Callback hell can always be avoided by not writing it as callback hell. In this case though, lets start by rewriting some of your code.

First of all you can bail out early if you receive an error, so no need to use else statements in all your calls. This will remove a lot of scope for you.

So just by rewriting parts of your code, you could turn it into something like below:

router.get('/', (req, res, next) => {                       
  getData((err, data) => {                                  
    if (err) return console.error(err)                                        
    res.render('admin/', { /* .. */ })                      
  })                                                        
})                                                          

function getData (cb) {                                     
  Account.find({accType: 'Employer'}, (err, col) => {       
    if (err) return cb(err)                                 
    Banner.findOne((err, banner) => {                       
      if (err) return cb(err)                               
      Account.findOne({accType: 'Awaiting'}, (err, doc) => {
        if (err) return cb(err)                             
        cb(null, { col, banner, doc })                      
      })                                                    
    })                                                      
  })                                                        
}

Now, lets go back to your usage of async which is a good idea to use here, since there's really no idea to call Account.find(), Banner.findOne() and Account.findOne() in order.

Your problem here is that you're using async.parallel incorrectly, which assumes an array (or an object) of functions taking a callback.

This could look something as follows:

async.parallel({                                       
  col: cb => Account.find({accType: 'Employer'}, cb),  
  banner: cb => Banner.findOne(cb),                    
  doc: cb => Account.findOne({accType: 'Awaiting'}, cb)
}, (err, result) => {                                  
  // access result.col, result.banner and result.doc   
})

Now we can refactor getData above, so the end result could look like:

router.get('/', (req, res, next) => {                              
  getData((err, data) => {                                         
    if (err) return console.error(err)                                               
    res.render('admin/', { /* data.doc, data.banner, data.doc */ })
  })                                                               
})                                                                 

function getData (cb) {                                            
  async.parallel({                                                 
    col: cb => Account.find({accType: 'Employer'}, cb),            
    banner: cb => Banner.findOne(cb),                              
    doc: cb => Account.findOne({accType: 'Awaiting'}, cb)          
  }, cb)                                                           
}

This looks pretty clean and there's really no need to use Promises.

When trying to implement this I get `Error: Callback was already called.` Any idea what might be the reason? — Alex Ironside, Apr 02 '18 at 16:15

Dave Meehan · Accepted Answer · 2018-04-02T16:35:38.413

1

This is about as simple as it gets, and assumes:

You are using Express or similar as your route handler, and you can return a promise instead of using the callback
You are using Mongoose or similar as the DB, and can return a promise instead of using the callback.

Check your versions for Promise support.

You appear to be missing some query parameters for Banner at least, and you need to work out if any of these are dependent or, as illustrated, they can be run in parallel.

router.get('/', (req, res) => {
  return Promise.all([
    Account.find(),
    Banner.findOne(),
    Account.findOne({ accType: 'Awaiting' }),
  ]).then(([ col, banner, doc ]) => {
    res.render('admin/', {
      title: 'Admin pannel',
      user: req.user,
      employers: col,
      areAwaiting: awaiting,
      banner: banner,
    );
  }).catch(e => { console.error(e) });
});

edited Apr 02 '18 at 16:35

answered Apr 02 '18 at 00:23

Dave Meehan

3,133
1
17
24

When trying to implement this I get: `/home/iron/Documents/school/wps/Project/routes/admin.js:32 ]).then([ col, banner, doc ] => { ^ SyntaxError: Unexpected token ]` Any idea what might be the reason? – Alex Ironside Apr 02 '18 at 16:15
Made it work after a slight modification. Take a look at the post for the working version – Alex Ironside Apr 02 '18 at 16:19
Sorry, yes, I missed the parentheses on the arrow function. Now edited. – Dave Meehan Apr 02 '18 at 16:36

Avoiding callback hell in node

2 Answers2

Linked