Skip to content

Docs falsely suggest exec() for async/await #8110

Closed
Closed
Docs falsely suggest exec() for async/await#8110
Labels
docsThis issue is due to a mistake or omission in the mongoosejs.com documentation
Milestone
5.7.3

Description

@moeriki
moeriki
opened on Aug 29, 2019

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

Activity

  1. vkarpov15 commented on Sep 4, 2019

    @vkarpov15
    vkarpov15
    on Sep 4, 2019
    Collaborator

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

  2. vkarpov15
    added this to the 5.6.13 milestone on Sep 4, 2019
  3. vkarpov15
    added
    docsThis issue is due to a mistake or omission in the mongoosejs.com documentation
    on Sep 4, 2019
  4. vkarpov15
    modified the milestones: 5.6.13, 5.7.1 on Sep 4, 2019

  5. moeriki commented on Sep 5, 2019

    @moeriki
    moeriki
    on Sep 5, 2019
    Author

    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.

  6. jloveridge commented on Sep 6, 2019

    @jloveridge
    jloveridge
    on Sep 6, 2019

    @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.

  7. moeriki commented on Sep 6, 2019

    @moeriki
    moeriki
    on Sep 6, 2019
    Author

    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.

  8. vkarpov15 commented on Sep 10, 2019

    @vkarpov15
    vkarpov15
    on Sep 10, 2019
    Collaborator

    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?

  9. vkarpov15
    modified the milestones: 5.7.1, 5.7.3 on Sep 11, 2019

  10. Enado95 commented on Sep 14, 2019

    @Enado95
    Enado95
    on Sep 14, 2019

    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

  11. vkarpov15 commented on Sep 19, 2019

    @vkarpov15
    vkarpov15
    on Sep 19, 2019
    Collaborator

    @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.

  12. Enado95 commented on Sep 19, 2019

    @Enado95
    Enado95
    on Sep 19, 2019

    @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?

  13. vkarpov15 commented on Sep 23, 2019

    @vkarpov15
    vkarpov15
    on Sep 23, 2019
    Collaborator

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

  14. Enado95 commented on Sep 24, 2019

    @Enado95
    Enado95
    on Sep 24, 2019

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

  15. vkarpov15
    closed this as completedin c5b2355on Sep 27, 2019

  16. vkarpov15 commented on Sep 29, 2019

    @vkarpov15
    vkarpov15
    on Sep 29, 2019
    Collaborator

    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.

  17. Enado95 commented on Sep 29, 2019

    @Enado95
    Enado95
    on Sep 29, 2019 via email

  18. DevBrent commented on Sep 2, 2020

    @DevBrent
    DevBrent
    on Sep 2, 2020

    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/

  19. Automattic
    locked and limited conversation to collaborators on Sep 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    docsThis issue is due to a mistake or omission in the mongoosejs.com documentation

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions