-
Notifications
You must be signed in to change notification settings - Fork 49
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
Discussion #1
Comments
I forgot to mention: I would love to help out. |
Yeah, I've only recently started using async/await in a few things, so my familiarity with them is slowly increasing. Making the switch wouldn't require too much refactoring. Thanks for the suggestion. |
While using async/await was already possible with this package, it now uses them internally to handle requests. There's now one main function that returns a promise, while the others internal functions use that and async await to have a cleaner flow. The tests have been updated to reflect the ability to use async/await. The readme has also been updated to show how the async/await functionality can be used. Thanks to @rontav for issue #1 bringing up the idea.
I've pushed up my changes where internally uses some I don't think all the examples in the readme need to be changed. While using async functions is nice, it's a little cumbersome in my opinion. The examples in the readme could be run exactly as they are. For example, if someone slapped this into a file to run, const res = await moviedb.searchMovie({ query: 'alien' })
console.log(res) it would fail because it's not inside of an async function. Also, it's foundational to understand Promises before understanding async/await. I've included an example of how to use async/await, but don't think it's necessary to change all the examples. |
Firstly, I would like to congratulate you on taking initiative for this project. 👍
I like the switch to Promises, but I'm wondering why you didn't go all the way and use async/await.
Also, I think that the examples in the readme should be using async/await so that they are simpler.
The text was updated successfully, but these errors were encountered: