-
-
Notifications
You must be signed in to change notification settings - Fork 737
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(socketDelay): support options.timeout #1848
Conversation
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.
Hi, thanks so much for this! 🙌
The current implementation only supports `req.setTimeout`, but Node.js allows for `options.timeout` to accomplish the same. https://nodejs.org/docs/latest-v12.x/api/http.html#http_http_request_url_options_callback
This looks good to me! I'll leave it open a couple more days in case anyone else wants to comment. |
const onEnd = sinon.spy() | ||
|
||
const req = http.request('http://example.test', { timeout: 5000 }, res => { | ||
res.setEncoding('utf8') |
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.
Does this setEncoding
have an affect on the 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.
I don't know to be fully honest. I copied the test case from above and made the minimal number of changes to make it work for options.timeout
. I can take a look into it and adjust it for both tests cases if needed?
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.
@mastermatt any thoughts?
This is failing on Node 8 because of the lock file issues we're having. Once that's fixed up and master is merged in again, this should be good to go. |
🎉 This PR is included in version 11.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
The current implementation only supports
req.setTimeout
, but Node.jsallows for
options.timeout
to accomplish the same.https://nodejs.org/docs/latest-v12.x/api/http.html#http_http_request_url_options_callback