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

Add await to probot and server load #1517

Merged
merged 1 commit into from Apr 27, 2021
Merged

Conversation

kammerjaeger
Copy link
Contributor

Some part already have an await some not. This should make it behave the same if the app has an async entry point.
Please let me know what you think.

One test fails for me (end-to-end-tests › hello-world app) but it does that even without my change.

@kammerjaeger kammerjaeger requested a review from a team as a code owner April 27, 2021 03:36
@welcome
Copy link

welcome bot commented Apr 27, 2021

Thanks for opening this pull request! A contributor should be by to give feedback soon. In the meantime, please check out the contributing guidelines and explore other ways you can get involved.

Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove the await from the load function? I'll have a look at this test

src/probot.ts Outdated
@@ -121,6 +121,6 @@ export class Probot {
return;
}

return appFn(this, {});
return await appFn(this, {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the load function is already async, this change makes no difference. If appFn is async it will return a promise, and await server.load will await that promise, no need to add an await here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean because a async function will only wraps a return value in a promise if it is not a promise itself?
As described here?: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#rewriting_a_promise_chain_with_an_async_function

I removed the await here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, pretty much 👍🏼

@gr2m
Copy link
Contributor

gr2m commented Apr 27, 2021

The end-to-end tests pass when you run npm run build once

@probot probot deleted a comment from ayvazjet Apr 27, 2021
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Danke David!

@gr2m gr2m merged commit 8cc1590 into probot:master Apr 27, 2021
@welcome
Copy link

welcome bot commented Apr 27, 2021

Thanks for your contribution to probot! 🎉
Congrats!

@github-actions
Copy link

🎉 This PR is included in version 11.2.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kammerjaeger kammerjaeger deleted the patch-1 branch April 27, 2021 19:58
@kammerjaeger
Copy link
Contributor Author

Thanks for the fast fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants