Correct chained promise syntax

Question

I trying to figure out how to correct re-write my function using promises. The original working version is below:

this.accountsAPI.find(filter, function(err,result){
    if (err || 0 == result.length) {
      return res.status(400).json({error: "Can't find the account."});
    }
    var workRequest = req.query.workRequest;
    // result has some records and we assume that _id is unique, so it must have one entry in the array
    var newJob = { jobId: workRequest, acceptedById: result[0]._id, dateCreated: new Date() };
    this.jobsAPI.create( newJob, function(jobErr, jobResult) {
      if (jobErr) { return res.status(400).json({error: "Can't create a new job."}); }
      res.status(200).json({newJob});
    });
});

I have re-written this as:

return new Promise(function ( fulfill, reject) {
    this.accountsAPI.find(filter)
      .then(function (result) {
        if (0 == result.length) { return res.status(400).json({error: "Can't create a new job."}); }
        var workRequest = req.query.workRequest;
        // result has some records and we assume that _id is unique, so it must have one entry in the array
        var newJob = { workRequestId: workRequest, acceptedById: result[0]._id, dateCreated: new Date() };
        this.jobsAPI.create( newJob, function(jobErr, jobResult) {
          if (jobErr) { return res.status(400).json({error: "Can't create a new job."}); }
          res.status(200).json({newJob});
        })
      })
      .catch((err) => {
        return res.status(400).json({
        error: "Can't create a job.",
        errorDetail: err.message
      });
});

Not positive that I coded the promise version correctly. However, even if I did, there is still a chained asynchronous request, so my Promise version just makes things more complicated.

Should I use promises for such calls? Is there a way to rewrite my code elegantly?

No, find returns an array of accounts. It is asynchronous. Though, we are using Feathers.JS [so find is a database adapter method] and David Luecke mentioned that they are wrapping everything with Promises in the next version. Sorry for a dumb question - why is that important? — Moshe Shmukler, Dec 16 '15 at 09:28
Because if it doesn't return a `Promise` it won't have a `.then()` method and you'll need to call `fulfill()` and `reject()` yourelf inside the `jobsAPI.create` callback. If it *did* (and `jobsAPI.create` returned a Promise too), you would simply have been able to to `return this.accountsAPI.find` — CodingIntrigue, Dec 16 '15 at 09:30
I still do not understand. Just to clarify, I can call .then on my newly constructed promise? — Moshe Shmukler, Dec 16 '15 at 09:49

score 2 · Accepted Answer · edited May 23 '17 at 10:27

No, wrapping everything in a Promise constructor doesn't automatically make it work.

You should start by promisifying the asynchronous functions you are using on the lowest level - that is, in your case, accountsAPI.find and this.jobsAPI.create. Only for this you will need the Promise constructor:

function find(api, filter) {
    return new Promise(function(resolve, reject) {
        api.find(filter, function(err, result) {
            if (err) reject(err);
            else resolve(result);
        });
    });
}
function create(api, newJob) {
    return new Promise(function(resolve, reject) {
        api.create(newJob, function(err, result) {
            if (err) reject(err);
            else resolve(result);
        });
    });
}

As you see, this is a bit repetitive, you can write a helper function for this; but in case you are using a promise library that is more than an ES6 polyfill it will probably provide one already.

Now we've got two functions, find and create, that will return promises. And with those, you can rewrite your function to a simple

return find(this.accountsAPI, filter).then(function(result) {
    if (result.length == 0)
        throw new Error("Can't find the account.");
    return result;
}, function(err) {
    throw new Error("Can't find the account.");
}).then(function(result) {
    // result has some records and we assume that _id is unique, so it must have one entry in the array
    return create(this.jobsAPI, {
        jobId: req.query.workRequest,
        acceptedById: result[0]._id,
        dateCreated: new Date()
    }).catch(function(err) {
        throw new Error("Can't create a new job.");
    });
}.bind(this)).then(function(newJob) {
    res.status(200).json(newJob);
}, function(err) {
    res.status(400).json({error:err.message});
});

Thank you. I get an error, trying to compile this. My guess is probably a bracket is missing. I am sure that your example is OK fundamentally. I do fail to see how Promise version is cleaner, in this case. In fact, I am having a difficult time understanding it, whereas the async version is relatively easy to comprehend. — Moshe Shmukler, Dec 16 '15 at 10:43
@MosheShmukler: Yeah, in this case the promise version is not much better as the code has so much error handling in it - which doesn't just go away but is inherent complexity. I did flatten it though so that I'm calling `res.status(400)` only in one position — Bergi, Dec 16 '15 at 10:49
Looks good, except you have some redundant error handling here. There is no need to catch `"Can't find the account."` only to rethrow it. — jib, Dec 17 '15 at 20:56
@jib: One error is when the `result` is empty, the other is when `find` rejected. Not sure what you mean by "rethrow", how would you do that? I could think of `find(…).then(res => { if (!res.length) throw /*nothing*/; … }).catch(_ => { throw new Error(…); }).…` — Bergi, Dec 17 '15 at 21:01
I misread the code a bit, so you're not rethrowing the same error, but still catching errors from e.g. `find` and replacing them with another (presumably less specific) error, instead of passing out the original error. Exposition perhaps, but could look a lot simpler without it, is all. — jib, Dec 22 '15 at 20:22

Correct chained promise syntax

1 Answers1

Linked