-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Replace async for-of loops with promises / clean up CI #8
Conversation
We can address this in #4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure execution finishes (i.e. wait for promises to resolve), be mindful of properly returning promises and waiting for them
I don't think this was addressed. What guarantees that the async calls in the loop execute? AFAICT you'll need to collect promises and use Promise.all to wait for all of them to complete.
Also, return
has no effect within forEach
. Returning something from run
doesn't make sense either imo, afaict nothing is using that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks a lot cleaner than my original version and this is what I had in mind 👍 Leaving a few comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking a few things i didn't get. Let's discuss these on the phone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
Issue #5.
Changes: