Refactor NIOHTTPClient to SwiftNIOHTTPClient in SmithySwiftNIO #1002
Conversation
jbelkins
left a comment
jbelkins
left a comment
There was a problem hiding this comment.
See recommended adjustments to module membership. Also let's try to see if we can make the SwiftNIO client not depend on CRT.
| .package(url: "https://github.com/apple/swift-log.git", from: "1.0.0"), | ||
| .package(url: "https://github.com/open-telemetry/opentelemetry-swift", from: "1.13.0"), | ||
| .package(url: "https://github.com/swift-server/async-http-client.git", from: "1.26.0"), | ||
| .package(url: "https://github.com/swift-server/async-http-client.git", exact: "1.22.0"), |
There was a problem hiding this comment.
just noting that this needs to be switched to from: for release...
IIRC this is exact for testing the minimum supported version?
There was a problem hiding this comment.
yup exactly, will switch it to from since we can see it passed all integration tests
| "Smithy", | ||
| "SmithyHTTPAPI", | ||
| "SmithyTelemetry", | ||
| .product(name: "AwsCommonRuntimeKit", package: "aws-crt-swift"), |
There was a problem hiding this comment.
This CRT dependency is not needed & should be removed.
(also, any "API" package shouldn't depend on an external implementation like CRT, per SRA)
| } | ||
| /// Typealias for backward compatibility. | ||
| /// The actual implementation is now in SmithyTelemetry. | ||
| public typealias TelemetryContext = SmithyTelemetry.TelemetryContext |
There was a problem hiding this comment.
Check what appears in IDE help when you put the cursor on ClientRuntime.TelemetryContext.
We might need to preserve the original doc comment here.
| import enum Smithy.URIScheme | ||
| import struct SmithyHTTPAPI.Headers | ||
|
|
||
| public class HttpClientConfiguration { |
There was a problem hiding this comment.
nit: HTTPClientConfiguration (note the caps)
As long as we're moving this to a new module, might as well make the naming consistent.
There was a problem hiding this comment.
That's a breaking change since it was a public class, so we'll have to bump the minimum
| // All Rights Reserved. | ||
| // | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // |
There was a problem hiding this comment.
Presuming these Sources/SmithyTelemetry files are all largely identical to the ones that were in ClientRuntime?
| // SPDX-License-Identifier: Apache-2.0 | ||
| // | ||
|
|
||
| import protocol Smithy.LogAgent |
There was a problem hiding this comment.
Can't really bring Smithy.LogAgent into this module without a breaking change, can we?
Package.swift
Outdated
| dependencies: [ | ||
| "Smithy", | ||
| "SmithyHTTPAPI", | ||
| "SmithyHTTPClient", |
There was a problem hiding this comment.
SmithyHTTPClient depends on CRT, which means that SmithySwiftNIO depends on CRT.
We need to find a way to break that dependency.
There was a problem hiding this comment.
We use some CRT interfaces within this package, that is the likely cause of this dependency.
We've already replaced many of those - in particular, for GA I got rid of all the CRT references in generated code - but there are still some between modules in this package & aws-sdk-swift.
| import struct Smithy.Attributes | ||
| import protocol Smithy.LogAgent | ||
| import struct Smithy.SwiftLogger | ||
| import SmithyTelemetry |
There was a problem hiding this comment.
Move DefaultTelemetry from ClientRuntime to SmithyTelemetry, then typealias it here to prevent the breaking change.
Even though it is technically an "implementation", it has no dependencies other than Smithy and SmithyTelemetry so SmithyTelemetry is the right place for it.
Also, consider renaming SmithyTelemetry to SmithyTelemetryAPI, which would follow the SRA naming pattern for packages that contain interfaces but not implementations (at least not implementations with dependencies.)
2 participants
Issue #
Description of changes
Scope
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.