feat(rt): introduce opaque KMP default HTTP client engine #606
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deprecate KtorEngine as synonymous with OkHttp. The `http-client-engine-ktor` is now a utility wrapper for _any_ Ktor compliant engine. The OkHttp engine has been moved to `http-client-engine-default` as the default engine on JVM. BREAKING CHANGE: Use of explicit `httpClientEngine = KtorEngine()` will no longer compile. The default engine will be synonymous with OkHttp making it unnecessary to set this engine. Users should remove this explicit SDK client configuration.
ianbotsf
approved these changes
Mar 18, 2022
Comment on lines
8
to
18
| /** | ||
| * Factory function to create a new HTTP client engine using the default for the current KMP target | ||
| */ | ||
| fun SdkHttpEngine(config: HttpClientEngineConfig = HttpClientEngineConfig.Default): HttpClientEngine = | ||
| newDefaultHttpEngine(config) | ||
|
|
||
| fun SdkHttpEngine(block: HttpClientEngineConfig.Builder.() -> Unit): HttpClientEngine { | ||
| val builder = HttpClientEngineConfig.Builder().apply(block) | ||
| val config = HttpClientEngineConfig(builder) | ||
| return SdkHttpEngine(config) | ||
| } |
There was a problem hiding this comment.
Comment: The name SdkHttpEngine feels ambiguous and inaccurate to me. Technically the CRT engine is also an "SDK HTTP Engine". It'd be clearer to me if it were DefaultHttpEngine or something similar.
| "use KtorEngine to wrap it. This constructor will be removed in a future release before GA.", | ||
| level = DeprecationLevel.ERROR | ||
| ) | ||
| constructor(config: HttpClientEngineConfig = HttpClientEngineConfig.Default) : this(DeprecationEngine) |
There was a problem hiding this comment.
Comment: It seems cleaner to throw an error immediately upon invoking KtorEngine() rather than later:
companion object {
@Suppress("UNUSED_PARAMETER")
@Deprecated(message = "KtorEngine was previously...", level = DeprecationLevel.ERROR)
operator fun invoke(config: HttpClientEngineConfig = HttpClientEngineConfig.Default): KtorEngine =
error("Invoking KtorEngine without an engine is deprecated")
}There was a problem hiding this comment.
It's irrelevant, code doesn't compile if you target this constructor
|
Kudos, SonarCloud Quality Gate passed!
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #
closes #602
Description of changes
http-client-engine-defaultthat provides anHttpClientEngineimplementation that works for KMP. This allows us to not hard code against a specific engine (which would require that engine be KMP compatible).%252Finstead of just%2Fwhen/was present in a query value. The fix for this was to turn off any encoding by ktor and use the encoding used by the SDK.KtorEngineintocommonand make it a utility that wraps any ktor compatible engine rather than being synonymous with a particular engine. See breaking change notes belowBreaking Change
This PR marks the
KtorEngineconstructor as deprecated since it no longer provides a concrete engine. Instead it now wraps some ktor compatible engine.Users that are currently constructing service clients like the above snippet will no longer compile on update. The default engine is now the Ktor OKHttp engine and so the recommended fix would be to simply not override
httpClientEngine.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.