-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(unstable): add more options to Deno.createHttpClient #17385
Conversation
still need to add tests; unsure exactly how to test some of these though |
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.
LGTM. Tests would indeed be good: There is this PR #16210 that has a use-case for pool_max_idle_per_host
at least. That may serve as some idea for testing.
# Conflicts: # cli/file_fetcher.rs # cli/http_util.rs # ext/fetch/lib.rs
@@ -1746,7 +1747,7 @@ mod tests { | |||
|
|||
fn create_test_client() -> HttpClient { | |||
HttpClient::from_client( | |||
create_http_client("test_client", None, vec![], None, None, None) | |||
create_http_client("test_client", CreateHttpClientOptions::default()) |
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.
Good cleanup
LGTM |
/** Set an optional timeout for idle sockets being kept-alive. | ||
* Set to false to disable the timeout. */ | ||
poolIdleTimeout?: number | false; | ||
/** Whether HTTP/1.1 is allowed or not. */ |
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.
Allowed -> enabled. Should either say it defaults to true (but that is confusing), so probably should be disableHttp1: boolean
. Wdyt?
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.
Should either say it defaults to true (but that is confusing)
How so?
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.
Because undefined casts to false, not true. http1: undefined should therefore cast to http1: false
Closes #11056
Closes #13636