-
-
Notifications
You must be signed in to change notification settings - Fork 227
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 timeout option not leaving early when on sleep
mode
#199
Fix timeout option not leaving early when on sleep
mode
#199
Conversation
Fix sindresorhus#157 Add regression test
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 looks good, except the tests are failing on CI.
Thanks a lot for the PR Guillaume! I've just reviewed it. Looking forward to merge this fix. |
As for the tests, they're working on local. I could remove the "leaving early" test. I thought that checking 2000ms over 500ms expected was safe enough. I'll try to extends the margin first, but I might have to remove this test (which remove the regression test). |
You can try to use |
This looks good to me now. Just waiting for @sindresorhus review before merging it :) Thanks! 👏 |
@@ -267,9 +267,25 @@ const execa = (command, args, options) => { | |||
}, parsed.options.timeout); | |||
} | |||
|
|||
const resolvable = (() => { |
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 is better known as a deferred, and is usually considered a bad practice: https://github.com/petkaantonov/bluebird/wiki/Promise-Anti-patterns#the-deferred-anti-pattern
I haven't looked closely into the changes here, but maybe there's a way to solve it without using a deferred? If not, that's fine too. Sometimes it's needed. But usually when someone grabs for a deferred, there's usually a better way.
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.
I think we fall into the "it's needed" case.
You might have to use a deferred object when wrapping a callback API that doesn't follow the standard convention. Like setTimeout.
The goal is to resolve as soon as the process complete or the setTimeout
ends. I could use some kind of cancellation, but I don't see any better solution right now.
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.
Yeah, I don't see a better way either. I was just trying my luck in case you could think of a better way.
This is looking good. Thanks for contribution, @GMartigny 👌 |
Fix #157
Add regression test