What is a nice way to override the value a promise resolves to?

Question

I am writing a Node.js script to populate an SQL database with a test dataset.

In the promise chain illustrated in the code snippet below (the real code is a bit more hairy), the function insertData() requires the db object to be passed through from the previous stage. However, asynchronous calls inside dropAndCreateTables() on the previous stage use db object but do not return it. I came up with a solution that wraps promises inside dropAndCreateTables() into another promise object that resolves to a db object. However:

• I heard that using Promise() constructor in non-library code is an antipattern and may lead to subtle and hard-to-diagnose mistakes
• I heard that nesting then()-chains is also an antipattern
• It does not allow me to ignore errors from the promiseDrop (for example, I don't care if tables don't exist on drop)
• It is ugly

Questions:

• Is there a simpler, nicer and more socially accepted way to override the return value of a promise? (in this case, created with Promise.all())
• Is there a way to restructure my code in a way that this problem does not occur? (That is, I don't exclude the possibility of "XY problem" here)

Code:

const dropAndCreateTables = (db, startClean) => {
  if(startClean) {
    const sqlDrop = fs.readFileSync('drop.sql').toString()
    const promiseDrop = db.raw(sqlDrop)

    const sqlCreate = fs.readFileSync('create.sql').toString()
    const promiseCreate = db.raw(sqlCreate)

    /********* Problems here? ************************************/
    return new Promise((resolve, reject) => {      // Ew?
      Promise.all([promiseDrop, promiseCreate])
        .then(() => {
          resolve(db) // Override the returned value
        })
        .catch(reject)
    })

  }
  return Promise.resolve(db)
}

initDB({ debug: false })
  .then((db) => {
    return dropAndCreateTables(db, START_CLEAN) // Without my hack this does not return `db`
  })
  .then((db) => {
    return insertData(db, DO_UPSERT)  // This needs the `db` object
  })
  .then(() => {
    console.info(`\n${timestamp()} done`)
  })
  .catch(handleError)

Answer 1

(Some fairly important notes midway and later in the answer, please do read all the way to the end.)

Is there a simpler, nicer and more socially accepted way to override the return value of a promise? (in this case, created with Promise.all())

Yes, you simply return a value from the then handler, and return the promise then returns:

return Promise.all([promiseDrop, promiseCreate])
    .then(() => db);

then (and catch) create promise chains. Each link in the chain can transform the result. then and catch return a new promise that will be fulfilled or rejected based on what happens in their callback:

• If their callback throws, the promise rejects with the error thrown
• If their callback returns a non-thenable value (e.g., promise), the promise is fulfilled with that value
• If their callback returns a thenable value, the promise is resolved to that thenable — it waits for the other promise to settle, then settles the same way

(If the term "thenable" isn't familiar, or you're not clear on the distinction between "fulfill" and "resolve," I go into promise terminology in this post on my blog.)

I heard that using Promise() constructor in non-library code is an antipattern and may lead to subtle and hard-to-diagnose mistakes

The distinction isn't library code vs. non-library code, it's between code that doesn't already have a promise to work with and code that does. If you already have a promise to work with, you almost never want to use new Promise. More: What is the explicit promise construction antipattern and how do I avoid it?

I heard that nesting then()-chains is also an antipattern

You almost never need to nest then chains, because again each link in the chain already has the means of tranforming the result passing through it. So:

// Unnecessary nesting
doSomething()
    .then(a => {
        return doSomethingElse(a * 2)
            .then(b => b * 3);
    })
    .catch(e => { /*...handle error...*/ });

can be more idiomatically and simply written:

doSomething()
    .then(a => doSomethingElse(a * 2))
    .then(b => b * 3);
    .catch(e => { /*...handle error...*/ });

Is there a way to restructure my code in a way that this problem does not occur? (That is, I don't exclude the possibility of "XY problem" here)

Not an X/Y per se, but you have a problem in that code: There's no guarantee the drop will happen before the create! So instead of starting both and letting them run in parallel and watching for the results with Promise.all, ensure those operations happen in sequence:

// Fairly minimal changes
const dropAndCreateTables = (db, startClean) => {
  if(startClean) {
    const sqlDrop = fs.readFileSync('drop.sql').toString()
    return db.raw(sqlDrop)
      .then(() => {
        const sqlCreate = fs.readFileSync('create.sql').toString()
        return db.raw(sqlCreate);
      })
      .then(() => db);
  }
  return Promise.resolve(db)
}

But, I wouldn't use sync file I/O. Instead

const promisify = require("utils").promisify;
const readWithPromise = promisify(fs.readFile);

and then

const dropAndCreateTables = (db, startClean) => {
  if(startClean) {
    const getDrop = readWithPromise('drop.sql');       // Start this first
    const getCreate = readWithPromise('create.sql');   // Then start this
    return getDrop
      .then(dropSql => db.raw(dropSql))                // Got the drop SQL, run it
      .then(() => getCreate)                           // Make sure we have the create SQl
      .then(createSql => db.raw(createSql))            // Run it
      .then(() => db);
  }
  return Promise.resolve(db)
}

Note how we avoid ever busy-waiting on I/O, and we can overlap the DB's drop operation with reading the create SQL.

Answer 2

You don't need to call the Promise constructor when returning another promise, you can just write it like:

return Promise.all([promiseDrop, promiseCreate])
    .then(() => db)
    .catch(error => {
       // handle the error or rethrow it
    })

Answer 3

You might omit resolving db from dropAndCreateTables like this:

.then((db) => {
    return dropAndCreateTables(db, START_CLEAN).then(Promise.resolve(db));
})

Answer 4

You should not let dropAndCreateTables return a db promise, there is no real usecase for it. So:

return Promise.all([promiseDrop, promiseCreate]);

is enough. Now the chaining part:

initDB({ debug: false }).then(async (db) => {

  await dropAndCreateTables(db, START_CLEAN);
  await insertData(db, DO_UPSERT);

  console.info(`\n${timestamp()} done`)
}).catch(handleError)