Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async/await guide #7506

Closed
JulioJu opened this issue Feb 9, 2019 · 14 comments
Closed

Async/await guide #7506

JulioJu opened this issue Feb 9, 2019 · 14 comments
Labels
discussion If you have any thoughts or comments on this issue, please share them! docs This issue is due to a mistake or omission in the mongoosejs.com documentation

Comments

@JulioJu
Copy link
Contributor

JulioJu commented Feb 9, 2019

Hi @libook @mhombach @vkarpov15

Following the issue #7461 I would propose to improve API documentation.

For instance, for https://mongoosejs.com/docs/api.html#model_Model.findOneAndUpdate

[Note: I've deleted a proposal here, because it was so bad]

Or instead as A query is not a fully-fledged promise (see https://mongoosejs.com/docs/promises.html) we could say instead:

Returns
    Query (a thenable object)

Like that a user that know what is a Promise and a thenable and what is a callback could know easy how to use this function. When I've started with Mongoose, I thought falsely « dash, mongoose API doesn't return Promise contrary to the mongodb native driver ».

It should be done for all CRUD operations, into lib/model.js and lib/query.js.

Furthermore https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mongoose/index.d.ts should be updated accordingly. At least for this TypeScript definition file, we could say that: « findOneAndUpdate and others CRUD operations are Promises ».

What do you think about that?

@JulioJu JulioJu changed the title [Docs] CRUD API, promise and callback. [Docs] CRUD API, promise/thenable and callback. Feb 9, 2019
@JulioJu
Copy link
Contributor Author

JulioJu commented Feb 10, 2019

Also, the example could be updated to:

A.findOneAndUpdate(conditions, update, options, callback) // returns Query and executes the request
A.findOneAndUpdate(conditions, update, options)  // returns Query (execute the request like for a Promise)
A.findOneAndUpdate(conditions, update, callback) // returns Query and executes the request
A.findOneAndUpdate(conditions, update)           // returns Query (execute the request like for a Promise)
A.findOneAndUpdate()                             // returns Query (execute the request like for a Promise)

JulioJu pushed a commit to JulioJu/DefinitelyTyped that referenced this issue Feb 10, 2019
JulioJu pushed a commit to JulioJu/DefinitelyTyped that referenced this issue Feb 10, 2019
@JulioJu
Copy link
Contributor Author

JulioJu commented Feb 10, 2019

Please check DefinitelyTyped/DefinitelyTyped#32874

In this sample, for DefinitelyTyped/DefinitelyTyped@4b0c6cd , I have marked as promise only when { RawResult: true }

But for the others, maybe DocumentQuery could inherit from PromiseConstructor ? : see https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mongoose/index.d.ts#L1715

As it we could use async / await in TypeScript, like @mhombach @vkarpov15 you described at #7461 .

@vkarpov15
Copy link
Collaborator

There is some nuance that prevents us from having queries inherit from promises. See https://mongoosejs.com/docs/queries.html#queries-are-not-promises for more details. With #7398 we should be able to make queries extend from promises.

I'm not sure I like adding parentheses after queries return value. Maybe we add examples of using async/await with queries to the API docs instead?

@vkarpov15 vkarpov15 added docs This issue is due to a mistake or omission in the mongoosejs.com documentation discussion If you have any thoughts or comments on this issue, please share them! labels Feb 12, 2019
@JulioJu
Copy link
Contributor Author

JulioJu commented Feb 13, 2019

Ok I see,

But for DefinitelyTyped/DefinitelyTyped#32874 the problem is that if Query doesn't inherit from PromiseConstructor, tslint gives errors await-promise and no-invalid-await .

If you say that it's not a good idea to say in the TypeScript definition file that Query inherits from Promises, maybe in the doc we could add a small note to say that tslint errors await-promise and no-invalid-await are false positives?

@libook
Copy link

libook commented Feb 15, 2019

Hi @JulioJu, thanks for your concern for my issue.

As we know, 'Queries' are not Promises but 'thenable' objects.
And the await command is not only for Promises. Actually, await command works with thenable objects.

//This will return a thenable object.
function foo(x) {
  return {
    then: function(resolve, reject) {
      setTimeout(resolve, 3000, 'The x is: ' + x);
      return this;
    }
  };
}

(async () => await foo(2))().then(console.log);//Use await to execute the thenable object.

I guess this is how Queries support await command.

So, what if this is a issue of tslint? Checking whether the object has a then method, instead of checking whether the object is a Promise.

I don't use TypeScript and tslint. So there may be something I don't know. I'll be happy if you point them out.

@JulioJu
Copy link
Contributor Author

JulioJu commented Feb 15, 2019

@libook tanks.

Maybe a better example is the following (Query is in Mongoose library, myMongooseService could be in a Service file, and the last line in a route file provider, for instance).

function Query(x) {
    return {
        then: function(resolve, reject) {
            setTimeout(resolve, 3000, 'The x is: ' + x);
            return this;
        }
    };
}

myMongooseService = async () => {
    const x = await Query(2);
    console.log('coucou', x);
    return x;
}


myMongooseService().then(m => { console.log('main', m); });

Preceding could be polyfill in es6 with the following (and it demonstrates why your affirmation is true and how it works, even if it's the worst solution):

myMongooseService = () => {
    return Promise.resolve(Query(2))
    .then(r => {
            console.log('coucou', r);
        });
};

myMongooseService().then(m => { console.log('main', m); });

Preceding could be polyfill in es5 / es3 (this sample show that Promise.resolve syntax presented above is not useful in this case, even if await syntax sugar could stay cool).

myMongooseService = () => {
    return Query(2)
    .then(r => {
            console.log('coucou', r);
        });
};

myMongooseService().then(m => { console.log('main', m); });

Note: your sample could be polyfill in es6 too: (and it demonstrates why your affirmation is true and how it works)

(() => {
    return Promise.resolve(foo(2));
})().then(console.log);

or simply

Promise.resolve(foo(2)).then(console.log);

or more simply (this sample show that Promise is not useful in this case, even if await syntax could stay cool)

foo(2).then(console.log);

The preceding samples work, because each then handler of the Promise is omitted, then handler apply to the foo(2) function, see also https://javascript.info/promise-chaining )

@JulioJu
Copy link
Contributor Author

JulioJu commented Feb 15, 2019

With further tests, tslint check check whether the object has a then method, sorry.

JulioJu pushed a commit to JulioJu/DefinitelyTyped that referenced this issue Feb 15, 2019
@JulioJu
Copy link
Contributor Author

JulioJu commented Feb 15, 2019

Therefore, for DefinitelyTyped/DefinitelyTyped#32874 all it's ok, I've tested ;-).

For the doc https://mongoosejs.com/docs/api.html I propose to comment like following: 

Returns
    Query, a thenable object

Example
A.findOneAndUpdate(conditions, update, options, callback) // returns Query and executes the request
A.findOneAndUpdate(conditions, update, options)  // returns Query
A.findOneAndUpdate(conditions, update, callback) // returns Query and executes the request
A.findOneAndUpdate(conditions, update)           // returns Query
A.findOneAndUpdate()                             // returns Query

Maybe somewhere in the doc, something like #7506 (comment) could be added to explain how and why Mongoose work with await / async. Indeed, it's difficult to understand for a newbie like me with for instance https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/await or other existing famous tutorials. In fact, about await they say only “ The await expression causes async function execution to pause until a Promise is resolved ”, they speak never about then.

@vkarpov15 vkarpov15 changed the title [Docs] CRUD API, promise/thenable and callback. Async/await guide Feb 19, 2019
@vkarpov15
Copy link
Collaborator

@JulioJu adding an async/await guide is a good idea. There's a lot of nuance with using async/await, that's why I wrote a book about it. We'll also add a note that queries are thenable to the API docs

@Rossh87
Copy link
Contributor

Rossh87 commented Jun 24, 2019

I'd be happy to write this if it's still an issue of interest. Would you like me to fork and create a new .pug file, or edit it into an existing file?

@vkarpov15
Copy link
Collaborator

@Rossh87 sure, new .pug file would be great. The docs are a bit tricky to work with, but running node ./static.js to start a static server and make gendocs to compile the docs will get you most of the way there.

@Rossh87
Copy link
Contributor

Rossh87 commented Jul 2, 2019

@vkarpov15 I've got a PR ready to go that includes the new docs/async-await.pug, as well as changes to docs/layout.pug and source/index.js that are needed to get the new file added to site navigation. However, said PR also includes the newly-generated docs from running gendocs. From the contributing guidelines it looks like that may not be what you prefer; if not, let me know and I can get a commit set up with just the relevant file changes and no new documentation. Otherwise I'll submit what I've got.

@vkarpov15
Copy link
Collaborator

Feel free to submit what you have. We generally don't commit newly generated docs from running make gendocs, but that isn't a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion If you have any thoughts or comments on this issue, please share them! docs This issue is due to a mistake or omission in the mongoosejs.com documentation
Projects
None yet
Development

No branches or pull requests

5 participants
@vkarpov15 @libook @JulioJu @Rossh87 and others