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.

Answer 1

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});
  })