Skip to content

Genericize ResponseProviding to an HTTPSRequester - #4

Merged
mattmassicotte merged 7 commits into
ATProtoKit:mainfrom
germ-network:cross-platform-urlRequest
Dec 10, 2025
Merged

Genericize ResponseProviding to an HTTPSRequester#4
mattmassicotte merged 7 commits into
ATProtoKit:mainfrom
germ-network:cross-platform-urlRequest

Conversation

@germ-mark

Copy link
Copy Markdown
Contributor

Motivated by #3, which needs a URL request with different HTTP headers, genericize the ResponseProviding protocol to parametrize on more configurations of HTTPS requests.

The test bundle pulls in AsyncHTTPClient to demonstrate the drop-in use of their HTTPClient in place of URLSession, but it's intentionally omitted from the library dependencies as it pulls in a large set of dependencies.

Comment thread .github/workflows/ci.yml
- "platform=iOS Simulator,name=iPhone 16"
- "platform=tvOS Simulator,name=Apple TV"
- "platform=watchOS Simulator,name=Apple Watch Series 10 (42mm)"
- "platform=visionOS Simulator,name=Apple Vision Pro"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Github actions throws an error about finding two targets for Vision Pro, so this unblocks the macOS tests from executing.

///Or another networking library of their choice

//from Dave Delong
public struct HTTPMethod: Hashable, Sendable {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is indeed very much like NIOHTTP1.HTTPRequest (an enum), but defined here to avoid adding library dependency on Swift NIO.

Source is
https://davedelong.com/blog/2020/06/27/http-in-swift-part-1/, where he argues the type is properly a struct not an enum.

@mattmassicotte

Copy link
Copy Markdown
Contributor

I think I'm understanding what's changed here. And I think the additions in Sources/ATResolve/HTTPRequester.swift make sense. However! The only reason I introduced the ResponseProviding at all was specifically to keep the implementation usable for clients that do not have access to URLSession. One AsyncHTTPClient is pulled in, it no longer really serves a purpose.

So I'd like to propose a compromise between this and the original implementation.

GenericHTTPSComponents -> Request

public protocol ResponseProviding {
	func data(for request: Request) async throws -> Data
}

Please correct me if I'm wrong! But I think this will leave enough of an abstraction in place to work dependency-free with URLSession while also making what you need to do possible via a out-of-library conformance for the request stuff. Does that sound about right?

@germ-mark

Copy link
Copy Markdown
Contributor Author

That sounds good, I can implement the proposed changes.

Are you concerned about AsyncHTTPClient as a test only dependency? I don't think, as configured, it impacts the ATResolve target. A little new to cross-platform, so acknowledge that it may not be desired in the test bundle either, or should be a separate bundle.

Can remove it as well, of course.

@mattmassicotte

mattmassicotte commented Dec 9, 2025

Copy link
Copy Markdown
Contributor

Unfortunately yes. Anything added to the package's dependencies array will be pulled into all clients, even if they are only used in tests. Hopefully one day SPM and/or Xcode get smarter about this.

I really hate sending things back period, and especially for this reason. But people are quite dependency-adverse, and this would be a pretty big impediment for many users.

@germ-mark

Copy link
Copy Markdown
Contributor Author

understood. I'm learning from this so appreciate the feedback

@mattmassicotte

Copy link
Copy Markdown
Contributor

Just wanted to clarify a pretty terrible typo on my end:

I really hate sending things back period

Back, not bad! I have a pretty strong bias towards just accepting submissions as long as they do not impact public API in a way I feel like I cannot support. I really appreciate your patience and work here! I might revisit the renaming as well potentially, to reduce churn. But regardless it was super cool of you to a) take this on and b) bare with me.

@mattmassicotte
mattmassicotte merged commit ad4ac7f into ATProtoKit:main Dec 10, 2025
7 checks passed
@germ-mark

Copy link
Copy Markdown
Contributor Author

Thank you! No worries as I've been treating the PR's as worked proposals, so no attachment to the implementation short of we want to solve the problem in the most sensible way and not maintain a fork :).

@germ-mark
germ-mark deleted the cross-platform-urlRequest branch December 11, 2025 21:23
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.

2 participants