Skip to content
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

Merged
merged 19 commits into from
May 21, 2023

Conversation

crowlKats
Copy link
Member

Closes #11056
Closes #13636

@crowlKats
Copy link
Member Author

still need to add tests; unsure exactly how to test some of these though

@bartlomieju bartlomieju added this to the 1.31 milestone Jan 28, 2023
Copy link
Collaborator

@aapoalas aapoalas left a 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.

cli/tsc/dts/lib.deno.unstable.d.ts Outdated Show resolved Hide resolved
ext/fetch/lib.rs Outdated Show resolved Hide resolved
@bartlomieju bartlomieju removed this from the 1.31 milestone Feb 23, 2023
aapoalas and others added 3 commits March 14, 2023 22:58
ext/fetch/lib.rs Outdated Show resolved Hide resolved
cli/tsc/dts/lib.deno.unstable.d.ts Outdated Show resolved Hide resolved
cli/tsc/dts/lib.deno.unstable.d.ts Outdated Show resolved Hide resolved
@crowlKats crowlKats changed the title feat: add more options to Deno.createHttpClient feat(unstable): add more options to Deno.createHttpClient May 20, 2023
ext/fetch/lib.rs Outdated Show resolved Hide resolved
test_util/src/lib.rs Outdated Show resolved Hide resolved
test_util/src/lib.rs Outdated Show resolved Hide resolved
@@ -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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup

@bartlomieju
Copy link
Member

LGTM

@crowlKats crowlKats merged commit 3e03865 into denoland:main May 21, 2023
@crowlKats crowlKats deleted the httpclient_more_options branch May 21, 2023 01:43
/** 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. */
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

APIs to facilitate connection pools [extention/fetch] Allow reqwest HTTPClient configuration
4 participants