.then() fires before previous .then() has returned

Question

I'm pulling category information from our local point of sale database (3rd party software) and trying to write it into a WooCommerce store. I'm also storing the data to my own database to maintain relationships between the two systems. I'm using promises to sequence everything, but one of my .then() statements is firing off before the previous .then() has returned, so I'm sending an empty payload to WooCommerce.

router.post("/:action", (req, res) => {
  if(req.params.action === "sync" && req.body.action === "sync") {
    // Query the POS database
    mssql.query(query, (err, result) => {
      let postData = {
        create: [],
        update: []
      }
      // Make some promises to pull the POS categories and their children
      Promise.all(promises)
        .then(cats => {
          let catPromises = cats.map(cat => {
            return new Promise((resolve, reject) => {
              Category.findOne(
                // Check for existing entry in the linking DB...
              )
                .then(data => {
                  // ...and handle accordingly
                  resolve()
              })
                .then(() => {
                  let childPromises = cat.children.map(child => {
                    return new Promise((resolve, reject) => {
                      Category.findOne(
                        // Checking for existing entry in the linking DB...
                      )
                        .then(data => {
                          // ...and handle accordingly
                          resolve()
                      })
                    })
                  })
                  Promise.all(childPromises)
                    .then(resolved => {
                      resolve()
                  })
              })
            })
          })
          Promise.all(catPromises)
            .then(() => {
              return
          })
      })
        .then(() => {
          // This is the part that's firing early
          return axios.post(
            // data
          )
      })
...

EDIT: Newly refactored, still having problems.

Promise.all(promises).then(cats => {
  let catPromises = cats.map(cat => {
    Category.findOne(
      // Check for existing...
    ).then(data => {
      // ...and handle accordingly
    }).then(() => {
      let childPromises = cat.children.map(child => {
        Category.findOne(
          // Check for existing...
        ).then(data => {
          // ...and handle accordingly
        })
      })
      return Promise.all(childPromises)
    })
  })
  // Now this is where we're reaching early
  return Promise.all(catPromises)
}).then(() => {
  // API call
})

Looks like you're falling into the [explicit promise construction anti-pattern](https://stackoverflow.com/q/23803743/283366) which may also be your problem, ie `Category.findOne()` returns a promise so there's no reason to wrap it in `new Promise()` — Phil, Dec 17 '18 at 23:06
you are over wrapping with promises when you already have promises. And you have an hell of callbacks too — quirimmo, Dec 17 '18 at 23:07
Please provide a Minimal, Complete, and Verifiable example. It is tough, but emphasis in minimal. Also, please fix the indentation. Follow https://stackoverflow.com/help/mcve — Naor Tedgi, Dec 17 '18 at 23:08
Note that If a Promise returns a `thenable` (a Promise or some other object with a `then` method), the outer promise takes the value of the inner promise. This means that certain explicit `new Promise` methods in your posted code are redundant, and may be causing you trouble. I'd post this as an answer, but without a clearer understanding of which methods return promises (`postData.update.push`, `mssql.query`, etc) I'm not sure I'd be able to specifically identify where your code is failing. — Jeff Bowman, Dec 17 '18 at 23:11
your code will also attempt to call `resolve` twice on one promise (the first `new promise` — Jaromanda X, Dec 17 '18 at 23:38
here is your first code snippet with anti-patterns removed https://jsfiddle.net/f7extrb6/ - that may help you see what you're doing wrong — Jaromanda X, Dec 17 '18 at 23:42
Avoid the [`Promise` constructor antipattern](https://stackoverflow.com/q/23803743/1048572?What-is-the-promise-construction-antipattern-and-how-to-avoid-it)! — Bergi, Dec 18 '18 at 09:46
Thanks for the assistance. Definitely should've known better on the extra wrapping, and I do see the early resolve() now, but returning Promise.all() was the big thing I was missing...or so I thought. I've refactored everything, but I'm still getting an early return, now on the child promises. Updated code in the original post -- any other ideas? — Erin Toler, Dec 19 '18 at 15:13
Got it; I failed to return the Mongoose calls, so the promises were resolving immediately due to lack of return value. Thanks for the help. — Erin Toler, Dec 19 '18 at 15:29

score 0 · Answer 1 · answered Dec 19 '18 at 15:31

Final solution:

Promise.all(promises).then(cats => {
  let catPromises = cats.map(cat => {
    return Category.findOne(
      // Check for existing...
    ).then(data => {
      // ...and handle accordingly
    }).then(() => {
      let childPromises = cat.children.map(child => {
        return Category.findOne(
          // Check for existing...
        ).then(data => {
          // ...and handle accordingly
        })
      })
      return Promise.all(childPromises)
    })
  })
  // Now this is where we're reaching early
  return Promise.all(catPromises)
}).then(() => {
  // API call
})

.then() fires before previous .then() has returned

1 Answers1