-
Notifications
You must be signed in to change notification settings - Fork 1
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
fix(PM): close with timeout #133
Conversation
4aa404f
to
c9db7db
Compare
@@ -203,8 +203,6 @@ export class Controller { | |||
this.stateActions.removeFromQueue(slug); | |||
|
|||
this.updateProgressInfo(slug); | |||
|
|||
this.triggerNextPromo(); |
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.
This change dont break/fix any test. Can we add test for this?
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.
No, this change breaks 8 tests for callback validation controller.subscribe(callback)
. They are corrected by emitChange calls here. triggerNextPromo
is also called in this function
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.
Maybe we should make closePromoWithTimeout async and await result?
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.
We still need to call addPromoToFinished
and removeFromQueue
before await result. It's unnecessary here
0b0276d
to
3e4e4de
Compare
No description provided.