Use ktor and kotlinx serialization #126
Conversation
rahulsainani
changed the base branch from
main
to
improvement/official-kotlin-code-style
rahulsainani
changed the base branch from
improvement/official-kotlin-code-style
to
main
rahulsainani
force-pushed
the
use-ktor-and-kotlinx-serialization
branch
3 times, most recently
from
a570505 to
10a10e6
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the repository to use ktor for network requests and kotlinx serialization for JSON parsing in support of Kotlin Multiplatform. Key changes include replacing Retrofit calls with ktor’s safeRequest utility, updating the MoviesRepository and MoviesService implementations accordingly, and reorganizing dependency injection modules.
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| MoviesRepositoryTest.kt | Updated tests using runTest and coEvery with new API response handling. |
| HttpClientXTest.kt | Added tests for the new safeRequest implementation and ApiResponse mapping. |
| Movies.kt | Moved to the new DI package structure. |
| MoviesService.kt | Replaced Retrofit with ktor client calls and safeRequest; updated URL building and content type settings. |
| MoviesRepository.kt | Switched repository implementation to use suspend functions and ApiResponse toEither mapping. |
| Remaining files | Adjusted DI modules, removed unused Retrofit/Retrofit-based APIs and updated package naming. |
Files not reviewed (1)
- app/build.gradle.kts: Language not supported
Comments suppressed due to low confidence (1)
app/src/main/kotlin/com/fernandocejas/sample/features/movies/data/MoviesService.kt:32
- Verify that using ContentType.Text.Plain matches the API's expected response format; if the endpoint returns JSON, consider using ContentType.Application.Json.
contentType(ContentType.Text.Plain)
app/src/main/kotlin/com/fernandocejas/sample/features/movies/data/MoviesService.kt
Outdated
Show resolved
Hide resolved
rahulsainani
force-pushed
the
use-ktor-and-kotlinx-serialization
branch
from
10a10e6 to
d3456ce
Compare
There was a problem hiding this comment.
Thanks for the changeset @rahulsainani. Overall seems great but before merging, would be great to discuss the following topics:
- What is a feature and what is not?
- The role of the
coremodule and the dependency rule. - DI should be at core level and each feature (functionality) brings up its own dependencies that are attached to the DI graph.
Let me know what you think ;) 🚀
| @@ -99,7 +100,6 @@ dependencies { | |||
| implementation(libs.ktor.client.content.negotiation) | |||
| implementation(libs.kotlinx.serialization.json) | |||
| implementation(libs.ktor.serialization.kotlinx.json) | |||
| implementation(libs.converter.gson) | |||
There was a problem hiding this comment.
Love this! For a long time wanted to get rid of gson :)
| import com.fernandocejas.sample.features.movies.di.moviesFeature | ||
|
|
||
| fun allFeatures() = listOf( | ||
| networkFeature(), |
There was a problem hiding this comment.
I would not make network and navigation a feature. Unless we have a good reason for it. Think of features as user features. Networking and Navigation belong to core, which is the module shared across features.
Happy to read your thoughts :)
| @@ -83,4 +87,10 @@ class Navigator(private val authenticator: Authenticator) { | |||
| class Extras(val transitionSharedElement: View) | |||
| } | |||
|
|
|||
|
|
|||
| // temporary solution to compile till Navigator is deleted | |||
| fun navigationFeature() = object : Feature { | |||
There was a problem hiding this comment.
As mentioned in the comment above :)
| import org.koin.dsl.module | ||
| import io.ktor.client.plugins.logging.Logger as KtorLogger | ||
|
|
||
| fun networkFeature() = object : Feature { |
There was a problem hiding this comment.
Since this might not become a feature, the DI part should be rethought then. If do not see any issues to continue having a DI core module that includes all the cross cutting dependencies.
What?
On top of #125
– Use ktor instead of retrofit and kotlinx serialization instead of gson for KMP support
– Update MovieRepository to use ktor
– Add ApiResponse to model different api responses to objects, this encourages mapping exceptions to objects at the edge of the system
– Add safeRequest utility that maps to ApiResponse
– Test updates