Is creating a new promise with a async function call bad practice?

Question

Snippets are from a node.js and mongoDB CRUD application.Github repo for full code. The code is working fine but unsure if my structure and use of promises and async await are bad practice.

handlers._newbies = {};

handlers._newbies.post = (parsedReq, res) => {

    const newbie = JSON.parse(parsedReq.payload);
    databaseCalls.create(newbie)
        .then((result) => {
            res.writeHead(200,{'Content-Type' : 'application/json'});
            const resultToString = JSON.stringify(result.ops[0]);
            res.write(resultToString);
            res.end();
        })
        .catch(err => console.log(err));
};


const databaseCalls = {};

databaseCalls.create = (newbie) => {
    return new Promise(async (resolve, reject) => {
        try {
            const client = await MongoClient.connect('mongodb://localhost:27017', { useNewUrlParser: true });
            console.log("Connected correctly to server");
            const db = client.db('Noob-List');
            const result = await db.collection('newbies').insertOne(newbie);
            client.close();
            resolve(result);
        } catch(err) {
            console.log(err);
        }
    });
};

When the node server gets a POST request with the JSON payload, it calls the handlers._newbies.post handler which takes the payload and passed it to the

const newbie = JSON.parse(parsedReq.payload);
databaseCalls.create(newbie)

call. I want this database call to return a promise that holds the result of the db.collection('newbies').insertOne(newbie); call. I was having trouble doing this with just returning the promise returned by the insertOne because after returning I cant call client.close();.

Again maybe what I have done here is fine but I haven't found anything online about creating promises with promises in them. Thank you for your time let me know what is unclear with my question.

Answer 1

It is considered an anti-pattern to be wrapping an existing promise in a manually created promise because there's just no reason to do so and it creates many an opportunities for error, particular in error handling.

And, in your case, you have several error handling issues.

1. If you get an error anywhere in your database code, you never resolve or reject the promise you are creating. This is a classic problem with the anti-pattern.
2. If you get an error after opening the DB, you don't close the DB
3. You don't communicate back an error to the caller.

Here's how you can do your .create() function without the anti-pattern and without the above problems:

databaseCalls.create = async function(newbie) {
    let client;
    try {
        client = await MongoClient.connect('mongodb://localhost:27017', { useNewUrlParser: true });
        console.log("Connected correctly to server");
        const db = client.db('Noob-List');
        return db.collection('newbies').insertOne(newbie);
    } catch(err) {
        // log error, but still reject the promise
        console.log(err);
        throw err;
    } finally {
        // clean up any open database
        if (client) {
            client.close();
        }
    }
}

Then, you would use this like:

databaseCalls.create(something).then(result => {
    console.log("succeeded");'
}).catch(err => {
    console.log(err);
});

FYI, I also modified some other things:

1. The database connection is closed, even in error conditions
2. The function returns a promise which is resolved with the result of .insertOne() (if there is a meaningful result there)
3. If there's an error, the returned promise is rejected with that error

---

Not particularly relevant to your issue with promises, but you will generally not want to open and close the DB connection on every operation. You can either use one lasting connection or create a pool of connections where you can fetch one from the pool and then put it back in the pool when done (most DBs have that type of feature for server-side work).