Why are these nested promises not working?

Question

I am writing a process to be run on a cron job in a node environment.

The process fetches two lists of users from two external services, writes to file, and will do some comparison.

One of the sources for users is a Discourse forum, and unfortunately, to get the full user list, we have to fetch multiple trust_level lists and concatenate them.

I structured this using various nested promises and Promise.all. However, the function below is calling its then callback too early, before forumList.json and databaseList.json even exist... What am I doing wrong here?

import superagent from 'superagent'
import { writeFileSync } from 'fs'

export default function fetchData() {

  const process = []

  const databaseFetch = new Promise((resolve, reject) => {

    superagent.get('https://our-api.com/api/1/databases/our-db/collections/users')
      .end((error, response) => {

        if (error) {
          reject(error)
        } else {
          writeFileSync('temp/databaseList.json', JSON.stringify(response.body))
          resolve()
        }

      })

  })

  const forumFetch = new Promise((resolve, reject) => {

    // For reference, see https://meta.discourse.org/t/how-do-i-get-a-list-of-all-users-from-the-api/24261/8
    // We have to do this because of the way the discourse API is built
    const discourseApiList = [
      'trust_level_0',
      'trust_level_1',
      'trust_level_2',
      'trust_level_3',
      'trust_level_4',
    ]

    let forumList = []

    const discoursePromises = discourseApiList.map((trustLevel) => {

      return new Promise((resolveInner, rejectInner) => {
        superagent.get(`https://our-website.com/forum/groups/${trustLevel}/members.json`)
          .end((error, response) => {

            if (error) {
              rejectInner(error)
              reject()
            } else {
              forumList = forumList.concat(response.body.members)
              resolveInner()
            }

          })
      })

    })

    Promise.all(discoursePromises).then(() => {
      writeFileSync('temp/forumList.json', JSON.stringify(forumList))
      resolve()
    })

  })

  process.push(databaseFetch)
  process.push(forumFetch)

  return Promise.all(process)

}

@JaromandaX Then `then` from `fetchData()` itself, being called in a separate file. I want to read the two json files in `fetchData`'s `then`, but it is being called before they even exist... — j_d, Apr 11 '17 at 10:15
then you MUST return the final `Promise.all` - it DOES make a difference — Jaromanda X, Apr 11 '17 at 10:16
You don't need that `reject()` after `rejectInner()` - in fact you shouldn't use the outer `new Promise` at all. Avoid the [`Promise` constructor antipattern](http://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Apr 11 '17 at 10:19
thinking about it, if you don't return that final `Promise.all` you would return `undefined` ... and that does not have a `.then` to be called too early or otherwise - I suspect there's more to the **actual** code than you are letting on — Jaromanda X, Apr 11 '17 at 10:30

score 1 · Accepted Answer · answered Apr 11 '17 at 11:32

Promise code looks fine to me, problem must be elsewhere.

function fetchData() {

  const process = []

  const databaseFetch = new Promise((resolve, reject) => {
    setTimeout(function() {
      console.log('resolving databaseFetch');
      resolve();
    }, Math.round(Math.random() * 10000));
  })

  const forumFetch = new Promise((resolve, reject) => {

    const discourseApiList = [
      'trust_level_0',
      'trust_level_1',
      'trust_level_2',
      'trust_level_3',
      'trust_level_4',
    ]

    let forumList = []

    const discoursePromises = discourseApiList.map((trustLevel) => {

      return new Promise((resolveInner, rejectInner) => {

        setTimeout(function() {
          console.log('resolving ' + trustLevel);
          resolveInner();
        }, Math.round(Math.random() * 10000));

      })

    })

    Promise.all(discoursePromises).then(() => {
      setTimeout(function() {
        console.log('resolving discoursePromises');
        resolve();
      }, Math.round(Math.random() * 1000));
    })

  })

  process.push(databaseFetch)
  process.push(forumFetch)

  return Promise.all(process)
}

fetchData().then(() => console.log('finished!'));

score 0 · Answer 2 · answered Apr 11 '17 at 10:13

0

You should never nest Promises, because the only purpose of them is to create linear code. I would suggest to act this way:

1- create discoursePromises and resolve them with Promise.all

2- then, create forumFetch and databaseFetch and resolve them with Promise.all

I think you might find interesting async which is a great library for flow control. In particular, look at parallel. Hope this helps.

answered Apr 11 '17 at 10:13

enrichz

60
12

No, `async.js` does not help at all in promise-based code. – Bergi Apr 11 '17 at 10:42
Of course, you can use Promise OR async, because they're an alternative to each other. This means that the code must be rewritten to use async if desired. – enrichz Apr 11 '17 at 12:09

score -1 · Answer 3 · answered Apr 14 '17 at 12:47

Don't nest promises , if you don't want to use the async library , nor promise.all , u can just write your promises and then chain them to control their flow :

Identifying promises :

const promise1Fun = () => {
 return new Promise((resolve, reject) => {
 //do stuff 
 })
} 


const promise2Fun = () => {
 return new Promise((resolve, reject) => {
 //do stuff 
 })
}

Chaining promises:

promise1Fun.then(promise2Fun).catch((err) => console.error(err))

Why are these nested promises not working?

3 Answers3