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.

Async functions already return promises. Why would you do that? — SLaks, May 27 '18 at 16:49
You should not be wrapping existing promises in a new promise. That is considered an anti-pattern because it's completely unnecessary. — jfriend00, May 27 '18 at 16:51
Apart from the two concerns above, you should also handle the database connection differently..You're creating a new connection on every request. It would be more efficient to init the connection on start-up and reuse the connection. — eol, May 27 '18 at 16:52
Thank you for your replies. I think I was over thinking the problem. [Refactored Code](https://github.com/BrianMichaelRomano/nodejs_mongodb_project/blob/master/index.js). As far as the mongodb connection, your saying that it is ok to connect once when the application is spun up and leave open the whole time? Will it eventually close the connection on its own, needing to check upon future request to see if connection is still alive or need to be reconnected? I aws just going by the mongoDB nodejs driver Docs which shows opening and closing on each database interation. Again thanks for the help. — Firecore, May 27 '18 at 17:12
[Yes, it is absolutely an antipattern!](https://stackoverflow.com/a/43083793/1048572) — Bergi, May 27 '18 at 17:59

jfriend00 · Accepted Answer · 2018-05-27T18:40:03.697

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.

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.
If you get an error after opening the DB, you don't close the DB
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:

The database connection is closed, even in error conditions
The function returns a promise which is resolved with the result of .insertOne() (if there is a meaningful result there)
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).

Awesome great informative answer. I am also stuggling to do as other recommended and open the mongo connection on app spinning up and keep using same connection. Every approach I have come up with has failed. I can wrap my whole app in a mongoClient.connect but this seems not right. It seems I am having trouble passing the response from the `MongoClient.connect('mongodb://localhost:27017', { useNewUrlParser: true })` — Firecore, May 27 '18 at 18:45
@Firecore - It sounds like that should be the subject of another question where you can show what you tried and explain why it didn't work for you. — jfriend00, May 27 '18 at 18:48
OK thanks I will refactor my promises then open another question. — Firecore, May 27 '18 at 18:54

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

1 Answers1