Promises seem to not be called in correct order?

Question

I am trying to rewrite a module I wrote that seeds a MongoDB database. It was originally working fine with callbacks, but I want to move to Promises. However, the execution and results don't seem to make any sense.

There are three general functions in a Seeder object:

// functions will be renamed
Seeder.prototype.connectPromise = function (url, opts) {
    return new Promise((resolve,reject) => {
        try {
            mongoose.connect(url, opts).then(() => {
                const connected = mongoose.connection.readyState == 1
                this.connected = connected
                resolve(connected)
            })
        } catch (error) {
            reject(error)
        }
    })
}
[...]
Seeder.prototype.seedDataPromise = function (data) {
    return new Promise((resolve,reject) => {
        if (!this.connected) reject(new Error('Not connected to MongoDB'))
        // Stores all promises to be resolved
        var promises = []
        // Fetch the model via its name string from mongoose
        const Model = mongoose.model(data.model)
        // For each object in the 'documents' field of the main object
        data.documents.forEach((item) => {
            // generates a Promise for a single item insertion.
            promises.push(promise(Model, item))
        })
        // Fulfil each Promise in parallel
        Promise.all(promises).then(resolve(true)).catch((e)=>{
            reject(e)
        })
    })
}
[...]
Seeder.prototype.disconnect = function () {
    mongoose.disconnect()
    this.connected = false
    this.listeners.forEach((l) => {
        if (l.cause == 'onDisconnect') l.effect()
   })
}

There is no issue with the main logic of the code. I can get it to seed the data correctly. However, when using Promises, the database is disconnected before anything else is every done, despite the disconnect function being called .finally().

I am running these functions like this:

Seeder.addListener('onConnect', function onConnect () { console.log('Connected') })
Seeder.addListener('onDisconnect', function onDisconnect () {console.log('Disconnected')})

Seeder.connectPromise(mongoURI, options).then(
    Seeder.seedDataPromise(data)
    ).catch((error) => {  <-- I am catching the error, why is it saying its unhandled?
        console.error(error)
}).finally(Seeder.disconnect())

The output is this:

Disconnected
(node:14688) UnhandledPromiseRejectionWarning: Error: Not connected to MongoDB
    at Promise (C:\Users\johnn\Documents\Code\node projects\mongoose-seeder\seed.js:83:37)

which frankly doesn't make sense to me, as on the line pointed out in the stack trace I call reject(). And this rejection is handled, because I have a catch statement as shown above. Further, I can't understand why the database never even has a chance to connect, given the finally() block should be called last.

The solution was to return the Promise.all call, in addition to other suggestions.

Answer 1

You are passing the wrong argument to then and finally. First here:

Seeder.connectPromise(mongoURI, options).then(
    Seeder.seedDataPromise(data)
)

Instead of passing a callback function to then, you actually execute the function on the spot (so without waiting for the promise to resolve and trigger the then callback -- which is not a callback).

You should do:

Seeder.connectPromise(mongoURI, options).then(
    () => Seeder.seedDataPromise(data)
)

A similar error is made here:

finally(Seeder.disconnect())

It should be:

finally(() => Seeder.disconnect())

Promise Constructor Anti-Pattern

Not related to your question, but you are implementing an antipattern, by creating new promises with new Promise, when in fact you already get promises from using the mongodb API.

For instance, you do this here:

Seeder.prototype.connectPromise = function (url, opts) {
    return new Promise((resolve,reject) => {
        try {
            mongoose.connect(url, opts).then(() => {
                const connected = mongoose.connection.readyState == 1
                this.connected = connected
                resolve(connected)
            })
        } catch (error) {
            reject(error)
        }
    })
}

But the wrapping promise, created with new is just a wrapper that adds nothing useful. Just write:

Seeder.prototype.connectPromise = function (url, opts) {
    return mongoose.connect(url, opts).then(() => {
        const connected = mongoose.connection.readyState == 1
        this.connected = connected
        return connected;
    });
}

The same happens in your next prototype function. I'll leave it to you to apply a similar simplification there, so avoiding the promise constructor antipattern.

---

In the later edit to your question, you included this change, but you did not return a promise in that function. Add return here:

  return Promise.all(promises).then(() => {
//^^^^^^
      return true
  }).catch(() => {
      console.log(`Connected:\t${this.connected}`)
  })