Node.js Promise Chain with API and database query

Question

I am pretty new to the whole promises business and am trying out my project with them.
No I need to query an external API, go several items in the response, do other checks on my own database with them and when all of them are finished, need to resolve the whole promise in itself.
I also need even another call to my database at the marked point with another promise. These are independent from each other but the "parent" promise should only resolve when both are resolved.
I am at a wall here and probably need some general explanation about chaining several promises with multiple items.
Maybe I just understand something generally wrong here... Here is my code so far (shortened at ...):

'use strict';

import rp from 'request-promise';
import _ from 'lodash';

var Import = {
  init: function(user, options) {
    return new Promise((resolve, reject) => {
      rp.get("...") // call to external API
        .then((res) => {
          ...
          resolve();
        });
      ...
    });
  },
  run: function() {
    return new Promise((resolve, reject) => {
      ...

      rp.get(...) // call to external API
        .then((res) => {
          var events = JSON.parse(res).data;
          var promises = [];

          for (var i = 0; i < events.length; i++) {

            promises.push(new Promise((resolve, reject) => {
              ...

              Location.findOneAndUpdateAsync(...)
                .then((loc) => {
                  events[i].location = loc._id;
                  resolve();
                })
                .catch((err) => {
                  console.error(err);
                  reject();
                });

              // I need even another call to my database here later with another promise
            }));
          }
          return Promise.all(promises)
            .then(() => {
              console.log("all promises resolved");
              resolve(events);
            });
        })
        .catch((err) => {
          console.error(err);
          reject(err);
        });
    });
  }
};

it doesn't help to hide your question away inside the comments to the code... You're also using the typical promise "anti-pattern" of creating a new promise in your loop around a function that already creates a promise. — Alnitak, Feb 15 '16 at 15:38
These was just supposed to be an addition but I'm gonna change that — Kageetai, Feb 15 '16 at 15:41
First of all, avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572). Your functions already return promises. — Bergi, Feb 15 '16 at 15:41
Sounds like you're looking for `Promise.all([onePromise, otherIndependentPromise]).then(whenBothAreDone)` — Bergi, Feb 15 '16 at 15:43
@Kageetai is that two calls you're expecting to make _inside_ that loop? — Alnitak, Feb 15 '16 at 15:46
@Alnitak yes, two independent calls, they do not need to be after one another, but booth need to be resolved before the chain resolves — Kageetai, Feb 15 '16 at 15:47

score 3 · Accepted Answer · edited May 23 '17 at 12:23

3

You can dramatically simplify your code by not falling foul of the Promise constructor anti-pattern - the functions you're calling already return promises so you should take advantage of those.

Then, eliminate the for / push loop with Array.prototype.map:

var promises = events.map(function(event) {
    return Location.findOneAndUpdateAsync(...)
            .then((loc) => event.location = loc._id);
});

You said you have two sets of calls to make, but as they're independent, you might as well just use another .map call:

var promises2 = events.map(function(event) {
    return ...
});

and then you just need to wait for all of those and return your events object:

return Promise.all(promises.concat(promises2)).then(() => events);

There's no need for all those .catch blocks - you should let the error propagate upwards.

If (per your comments) the two inner calls have a dependency, you could try this:

var promises = events.map(function(event) {
    return Location.findOneAndUpdateAsync(...)
           .then((loc) => {
               event.location = loc._id;
               if (condition) {
                   return XXXOtherFunctionReturningPromise();
               }
           });
});

[and obviously then eliminate the .concat call from above]

edited May 23 '17 at 12:23

Community

1
1

answered Feb 15 '16 at 15:54

Alnitak

334,560
70
407
495

Hmm, would it be as easy to still the two calls depended, in the way, that I want to skip an event entirely if the first (which I still have to implement) call (to my own database) returns a specific answer? – Kageetai Feb 15 '16 at 16:00
probably easy, but not what you asked for! ;-) – Alnitak Feb 15 '16 at 16:01
Yeah, my own fault, just re-thought the whole process a bit... X-) – Kageetai Feb 15 '16 at 16:02
I've added something, but now on re-reading your comment I'm not sure what you're asking for. Was the first call the "first one in the loop", or the `rp.get()` call ? – Alnitak Feb 15 '16 at 16:06
I meant where I did `Location.findOneAndUpdateAsync()` was supposed be the first call to my own database and I still need another one afterwards to my database again – Kageetai Feb 15 '16 at 16:09
ok, so that's what I thought the first time - the updated code should achieve that - the `if()` branch can make that second call. – Alnitak Feb 15 '16 at 16:11
Thanks, I'm gonna try that out now. And still thanks alot for the explanation to just have two promise chains – Kageetai Feb 15 '16 at 16:11
NB: if that second function causes a `reject` it'll still propagate upwards correctly because it is being "returned". If you didn't have the `return` there it would fail silently, and the two chained promises would still appear to have been resolved. – Alnitak Feb 15 '16 at 16:12

Node.js Promise Chain with API and database query

1 Answers1