Mongoose: Docs falsely suggest `exec()` for async/await

[Moeriki]

Do you want to request a feature or report a bug?

Docs.

What is the current docs?

https://mongoosejs.com/docs/promises.html#built-in-promises

This means that you can do things like MyModel.findOne({}).then() and await MyModel.findOne({}).exec() if you're using async/await.

What is the expected docs?

await Model.findOne() works just fine.

This means that you can do things like MyModel.findOne({}).then() and await MyModel.findOne({}) if you're using async/await.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

Mongoose 5.5.14
Node.js 10.16.2

[vkarpov15]

Both approaches work. We'll add a note to the docs.

[Moeriki]

I'm not sure why you would reference both, since the .exec() is just more code with no benefit. Or am I missing something?

I found out by means of a colleague who unknowingly wrote this since the docs suggested it. As far as I can tell .exec() is only useful for invoking a callback-style response.

I could PR docs.

[jloveridge]

@Moeriki The difference is that the return type of Model.findOne is a Query. They just happen to have added a then to it so it is "thennable" but it is not a true promise. The return from exec is actually a promise if you don't supply a callback to the exec. While you might not care about this distinction some of us do.

[Moeriki]

I see. Thanks for the feedback.

I take back my words. Since users may have certain expectations from Promises I'd keep the .exec() as the most prominent documentation. We could still mention the "thennable" object in smaller print.

[vkarpov15]

You're right that queries are not promises: https://mongoosejs.com/docs/queries.html#queries-are-not-promises . The most meaningful difference is that if you await on the same query object twice, you will execute the query twice, whereas await on the same promise twice only executes the query once. We did also run into an issue where node's async_hooks don't work properly if you await on a custom thennable, but that's a node issue that mongoose can't work around.

TLDR; there are reasons to prefer using exec(), but for most apps it doesn't matter which one.

@jloveridge I'm curious to hear why you prefer a true promise over a thennable. Is it one of the reasons I listed or something else?

[Enado95]

So @vkarpov15 i use this approach :

async(companyID) => { await User.find({company:companyID}).then(data =>{ return data }).catch(err => { return err.message }) }

It works but is it faulty? I do the same for updating, saving and deleting documents
Or is it that i should do mongoose.Promose = require('bluebird') to make this "work" as intended

[vkarpov15]

@Enado95 I'm not sure what you mean by 'work'. The code as written definitely needs some work - its clumsy and suppresses errors. Why do you do .then(data => data)? That does nothing besides create an unnecessary promise. Also, why the .catch()? That means that you ignore all errors.

[Enado95]

@Enado95 I'm not sure what you mean by 'work'. The code as written definitely needs some work - its clumsy and suppresses errors. Why do you do .then(data => data)? That does nothing besides create an unnecessary promise. Also, why the .catch()? That means that you ignore all errors.

@vkarpov15 When say it works. I mean it returns a response and if there's mongo error it returns the a 500 response as well; here's my exact implementation below for deleting a record. I do the same for updating and save():

exports.deleteCompany = async (req, res) => {
  const company = await Company.findByIdAndDelete(
    mongoose.Types.ObjectId(req.params.id)
  )
    .then(data => {
      if (!data)
        return res.status(404).send('Company with the given id was not found')

      res.send('Company successfully deleted')
    })
    .catch(err => {
      return res.status(500).send({
        message: err.message
      })
    })
}

Since the .then(data => data) does nothing, how would truly make this method async and catch errors?

[vkarpov15]

This implementation for deleting a company should work fine. Not sure what you mean by "truly make this method async".

[Enado95]

So the same implementation for find or findOneAndUpdate would work similarly?
Also, since its an async function, is the .exec() required?

[vkarpov15]

find() and findOneAndUpdate() both return mongoose queries, so they should be fine as far as await is concerned. The exec() isn't necessary.

However, it is typically better to use exec() because of better stack trace support.

[Enado95]

Okay noted.

On Sun, Sep 29, 2019 at 6:53 PM Valeri Karpov [e-mail]
wrote:

find() and findOneAndUpdate() both return mongoose queries, so they should
be fine as far as await is concerned. The exec() isn't necessary.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/Automattic/mongoose/issues/8110?email_source=notifications&email_token=AGE5KHRAVXA4RD5QBJSNAZ3QME5WHA5CNFSM4IR7P3PKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD74BVYI#issuecomment-536353505,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGE5KHUKXKAOX3NTH2YK67DQME5WHANCNFSM4IR7P3PA
.

[DevBrent]

So @vkarpov15 i use this approach :

async(companyID) => { await User.find({company:companyID}).then(data =>{ return data }).catch(err => { return err.message }) }

It works but is it faulty? I do the same for updating, saving and deleting documents
Or is it that i should do mongoose.Promose = require('bluebird') to make this "work" as intended

A string is not an error. I would avoid throwing strings by any means necessary. Just remove the entire catch. Let your calls to async(companyID) handle the error how they see fit. For instance, they may prefer to use the error.code 11000 or duplicate key error to change the error response to the user.

https://web.archive.org/web/20111226063046/http://www.devthought.com/2011/12/22/a-string-is-not-an-error/