Deprecate ambiguous IPv4 and IPv6 target APIs #117
Conversation
Motivation: The API for creating IPv4/IPv6 resolvable targets is a bit ambiguous: it's quite easy to assume they will only resolve IPv4 or IPv6 addresses. Instead they expect to be provided with resolved IPv4 or IPv6 addresses. Modifications: - Deprecate these APIs and replace them with more obvious spellings - Add a debug-only check to validate that the target is a valid IPv4/IPv6 address Result: - Harder to make mistakes - Easier to diagnose mistakes
| @@ -29,6 +30,23 @@ extension ResolvableTargets { | |||
| /// Create a new IPv6 target. | |||
| /// - Parameter addresses: The IPv6 addresses. | |||
| public init(addresses: [SocketAddress.IPv6]) { | |||
| debugOnly { | |||
There was a problem hiding this comment.
Wouldn't it also be useful to get some better error other than the current unavailable: "Channel isn't ready." in release builds, too?
I'm thinking if the address comes from some config and there are different values for debug and release, it's possible that debug builds won't fail but then you'd get some confusing error in release.
There was a problem hiding this comment.
It would be useful but at that layer you don't have enough information to tie it back to creating the wrong resolver. The "Channel isn't ready." message can happen for any number of reasons, including you could've correctly used a resolver but the server just isn't up yet.
There was a problem hiding this comment.
I guess my question was actually if we should always perform this check, not just in debug mode (throwing an error instead of failing an assertion), or if it's too expensive to afford it in this codepath.
There was a problem hiding this comment.
I don't think it's necessary tbh, the change in spelling is clear enough.
Motivation:
The API for creating IPv4/IPv6 resolvable targets is a bit ambiguous: it's quite easy to assume they will only resolve IPv4 or IPv6 addresses. Instead they expect to be provided with resolved IPv4 or IPv6 addresses.
Modifications:
Result: