Skip to content

Create the default RequestOptions from the client instead of using a required init#97

Merged
FranzBusch merged 5 commits intoapple:mainfrom
guoye-zhang:default-options
Mar 6, 2026
Merged

Create the default RequestOptions from the client instead of using a required init#97
FranzBusch merged 5 commits intoapple:mainfrom
guoye-zhang:default-options

Conversation

@guoye-zhang
Copy link
Contributor

Related discussion in #71

public var allowsExpensiveNetworkAccess: Bool = true
public var allowsConstrainedNetworkAccess: Bool = true

public init() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove the initializer on the concrete client's options or keep it? It might be confusing whether there is a difference between HTTPRequestOptions() and DefaultHTTPClient.shared.defaultRequestOptions, but it's way less discoverable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on whether you anticipate ever needing to inspect the client's state before constructing a new options value. If not, you could either keep the init, or use something a bit more self-documenting, like:

public static var `default`: Self { .init() }

request: HTTPRequest,
body: consuming HTTPClientRequestBody<RequestWriter>?,
options: RequestOptions,
options: RequestOptions?,
Copy link
Contributor

Choose a reason for hiding this comment

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

Filed this to continue the nil vs non-nil discussion: #98

@guoye-zhang
Copy link
Contributor Author

I added the empty initializer back. We now have 3 ways to express the same thing:

  1. options: HTTPRequestOptions()
  2. options: DefaultHTTPClient.shared.defaultRequestOptions
  3. options: nil

I would like to discuss if it's actually a good idea for defaultRequestOptions to return differently per client instance, or should RequestOptions just have nil option properties which mean default. E.g. would "minimal" be a configuration passed to the client, then defaultRequestOptions would automatically be minimal?

@FranzBusch
Copy link
Member

I added the empty initializer back. We now have 3 ways to express the same thing:

  1. options: HTTPRequestOptions()
  2. options: DefaultHTTPClient.shared.defaultRequestOptions
  3. options: nil

I would like to discuss if it's actually a good idea for defaultRequestOptions to return differently per client instance, or should RequestOptions just have nil option properties which mean default. E.g. would "minimal" be a configuration passed to the client, then defaultRequestOptions would automatically be minimal?

I think we need to think about the developer experience when making the decision here:

// This only works with 2.
func foo(client: some HTTPClient) async throws {
  var options = client.defaultRequestOptions
  options.someSetting = true
}

// .init forces this
func foo<Client: HTTPClient>(client: Client) async throws {
  var options = Client.RequestOptions()
  options.someSetting = true
}

Just for that reason I think we should go with 2.. Now the other question is if we should make the requestOptions parameter on the protocol requirement optional or non-optional. If it is optional we need to define what clients should do. I personally believe it is best to make it non-optional on the protocol and the conveniences can make it optional and state that they call defaultRequestOptions.

@guoye-zhang guoye-zhang enabled auto-merge (squash) March 6, 2026 06:32
@FranzBusch FranzBusch disabled auto-merge March 6, 2026 07:59
@FranzBusch FranzBusch merged commit 010e498 into apple:main Mar 6, 2026
17 of 21 checks passed
@guoye-zhang guoye-zhang deleted the default-options branch March 7, 2026 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⚠️ semver/major Breaks existing public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants