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

Add Request/Response History to all public Response types #817

Merged
merged 9 commits into from
Mar 3, 2025

Conversation

gregcotten
Copy link
Contributor

Work to close #790

The fact that HTTPClient.Request is not Sendable make me think we're going to need to store something else, such as a URL and HTTPRequestHead, instead?

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 24, 2025

I think HTTPClient.Request can be made Sendable. We haven't bothered yet because we're still working through swift-nio* and don't want to have to revisit older packages, but a targeted change will work.


/// The target URL (after redirects) of the response.
public var url: URL? {
self.history.last?.request.url
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we think nil is actually possible here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as mentioned above, but copying here: Not from any HTTPClient.Response vended by the library, but since there are already public initializers, it will be nil for any library users that have been creating their own HTTPClient.Response.

@gregcotten
Copy link
Contributor Author

I think HTTPClient.Request can be made Sendable. We haven't bothered yet because we're still working through swift-nio* and don't want to have to revisit older packages, but a targeted change will work.

It seems like HTTPClient.Body isn't Sendable. Would adding conformance to it be as simple as marking it Sendable or are its internals more complicated than that?

@Lukasa
Copy link
Collaborator

Lukasa commented Feb 25, 2025

I'd expect that to just work.

@gregcotten
Copy link
Contributor Author

I'd expect that to just work.

indeed it does!

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Feb 27, 2025
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice one, I think this looks great.

@Lukasa Lukasa enabled auto-merge (squash) February 27, 2025 13:32
@Lukasa
Copy link
Collaborator

Lukasa commented Feb 27, 2025

Looks like we need to guard the Foundation imports with #if compiler(>=6.0) and then use @preconcurrency to import them on older compilers.

auto-merge was automatically disabled February 27, 2025 21:11

Head branch was pushed to by a user without write access

@gregcotten
Copy link
Contributor Author

Looks like we need to guard the Foundation imports with #if compiler(>=6.0) and then use @preconcurrency to import them on older compilers.

I did it somewhat surgically - let me know if this works

import Foundation
#if compiler(<6.0)
@preconcurrency import struct Foundation.URL
#endif

@gregcotten
Copy link
Contributor Author

@Lukasa, I "fixed" the API breakage... I think we should be good to go now

@gregcotten
Copy link
Contributor Author

Looks like we need to guard the Foundation imports with #if compiler(>=6.0) and then use @preconcurrency to import them on older compilers.

I did it somewhat surgically - let me know if this works

import Foundation
#if compiler(<6.0)
@preconcurrency import struct Foundation.URL
#endif

Nevermind, I don't actually like that and changed it to

#if compiler(>=6.0)
import Foundation
#else
@preconcurrency import Foundation
#endif

@gregcotten
Copy link
Contributor Author

@Lukasa, ready to go!

Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Great, still happy here, thanks!

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 3, 2025

Looks like the format check has opinions again, I'm afraid.

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 3, 2025

Format check is still grouchy.

@gregcotten
Copy link
Contributor Author

Format check is still grouchy.

and fixed

@Lukasa Lukasa enabled auto-merge (squash) March 3, 2025 15:46
@Lukasa Lukasa merged commit 31122ea into swift-server:main Mar 3, 2025
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Obtain Response URL
2 participants