Best Practice for chaining promises in Nodejs/Express

Question

I know that best way to chain promises in Nodejs/Express is like:

doSomeThing()
.then()
.then()
.catch();

But recently had to use the async and q module to iterate over a list/array and run an async function. I wanted to know that is there better way of doing/writing this -

var deferred = Q.defer();
var deferred2 = Q.defer();          
models.Local.findOne({
        where: {
            id: parseInt(req.body.localid)
        }
    })
    .then(function(resultLocal){
        if(!resultLocal){
            return res.status(404).json(
                {
                    "status" : "error",
                    'error': "Local Not Found"
                });
        }
        return models.Documents.create(req.body.document);
    })
    .then(function(docCreated){
            var attributes = req.body.document.Attributes;
            async.each(attributes, function(item, callback) {
                models.Doc_Tags.create({
                    value: item.value,
                    attribute_id: item.id,
                    document_id: docCreated.id
                })
                .then(function(attributeCreated){
                    var upObj = {};
                    upObj[item.col_name] = item.value;

                    models[item.table_name].update(upObj,{
                        where:{
                            id: req.body.document.local_id
                        }
                    })
                    .then(function(primaryUpdated){
                        deferred2.resolve();
                    })
                    .catch(function(error){
                        return res.status(400).json({status: 'error', error:error.message});
                    });

                    deferred2.promise
                    .then(function(){
                        callback();
                    })
                    .catch(function(error){
                        return res.status(400).json({status: "error", error: error.message});
                    });

                })
                .catch(function(error){
                    return res.status(400).json({status: 'error', error:error.message});
                });
            }, function(err,r){
                if( err ) {
                    return res.status(400).json({status: 'error', error:err.message});
                } else {
                    console.log('All attributes Associated');
                    deferred.resolve(docCreated);
                }
            });
            deferred.promise.then(function(result, attributes){
                var obj = req.body.Local;
                models.Local.update(obj, {
                    where: {
                        id: result.local_id
                    }
                })
                .then(function(resultUpdate){
                    return res.status(201).json({status: "success", document: result});
                })
                .catch(function(error){
                    return res.status(400).json({status: "error", error: error.message});
                });
            })
            .catch(function(error){
                return res.status(400).json({status: "error", error: error.message});
            });
        })
    .catch(function(error){
        return res.status(400).json({status: "error", error: error.message});
    });

Please correct me if I am doing something wrong. Functionality wise the code is running properly but I think I can refactor it somehow to look and read better.

Thanks.

Not using `async.js` together with promises is the most important good practice here. — Bergi, Jul 07 '16 at 06:17
@AlongkornChetasumon You can't just add the [tag:Bluebird] tag to the question when the OP is not using that library! — Bergi, Jul 07 '16 at 06:19
Have a look at the [deferred antipattern](http://stackoverflow.com/q/23803743/1048572) and how to avoid it. — Bergi, Jul 07 '16 at 06:21

Alongkorn · Answer 1 · 2016-07-07T04:21:16.790

your code can be cleaner and shorter.

basic ideas are

turn callback to promise, e.g., promisify() of bluebird.js can do that
async.each part can be refactor to Promise.all to call promise parallelly
re-arrange .then chain
javascript es6 is cleaner than older version

sample refactored version

const Promise = require('bluebird')

// CustomError should be separated to another node module
class CustomError {
  constructor(message, code) {
    this.code = code
    this.message = message
  }
}

let docCreated = undefined

function getPromiseParams(item) {
  return Promise.try(() => {
    return models.Doc_Tags.create({
        value: item.value,
        attribute_id: item.id,
        document_id: docCreated.id
    })
  }).then(attributeCreated => {
    const upObj = {};
    upObj[item.col_name] = item.value;
    return models[item.table_name].update(upObj, { where:{ id: req.body.document.local_id } })
  }).then(primaryUpdated => {
    return docCreated
  }).catch(error => {
    throw new CustomError(error.message, 400)
  })
}

Promise.try(() => {
  return models.Local.findOne({ where: { id: parseInt(req.body.localid) } })
  }).then(resultLocal => {
    if(!resultLocal) throw new CustomError('Local Not Found', 404)

    return models.Documents.create(req.body.document)
  }).then(_docCreated => {
    docCreated = _docCreated // assign value to docCreated

    const attributes = req.body.document.Attributes
    const promiseParams = attributes.map(item => getPromiseParams(item))
    return Promise.all(promiseParams)
  }).then(() => {
    const obj = req.body.Local
    return models.Local.update(obj, { where: { id: result.local_id }})
  }).then(() => {
    return res.status(201).json({status: "success", document: docCreated})
  }).catch(error => {
    return res.status(error.code || 400).json({status: "error", error: error.message});
  })

If you could please post the refactored version. It would give me more clarity on how to proceed and greater understanding. You may skip the nitty gritty and just the skeleton of the refactored code will do. Thanks. — Siddharth Srivastva, Jul 06 '16 at 19:36
@SiddharthSrivastva, see the example above, i have updated my post — Alongkorn, Jul 07 '16 at 04:21

Best Practice for chaining promises in Nodejs/Express

1 Answers1