-
Notifications
You must be signed in to change notification settings - Fork 617
refactor(core:data): Migrate to KMP #2364
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
base: kmp-impl
Are you sure you want to change the base?
refactor(core:data): Migrate to KMP #2364
Conversation
8c983ef
to
6e6c165
Compare
import org.koin.core.module.dsl.singleOf | ||
import org.koin.dsl.bind | ||
import org.koin.dsl.module | ||
|
||
val RepositoryModule = module { | ||
single<CoroutineDispatcher> { Dispatchers.IO } | ||
single<CoroutineDispatcher> { get() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will fail at runtime. previously we had discussion i said just look in the Dispatchers module. I had a look we have named CoroutineDispatcher.
This single<CoroutineDispatcher> { get() }
would work if we had CoroutineDispatcher
without a name qualifier.
try this:
single<CoroutineDispatcher> { get(named(MifosDispatchers.IO.name)) }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented you suggestion. Thanks
} | ||
} | ||
|
||
actual val platformModule: Module = AndroidDataModule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we have only one service which is NetworkMonitor
we can directly bind it, no need to create PlatformDependentDataModule.
- in android we can do sth like below:
actual val DataModule: Module = module {
single<NetworkMonitor> {
ConnectivityManagerNetworkMonitor(
context = androidContext(),
dispatcher = get(named(MifosDispatchers.IO.name)),
)
}
}
- in other module for example js:
class JsNetworkMonitor : NetworkMonitor {
override val isOnline: Flow<Boolean> = flowOf(true)
}
actual val DataModule: Module = module {
single<NetworkMonitor> { JsNetworkMonitor() }
}
@@ -61,13 +62,13 @@ interface ClientService { | |||
): Flow<Page<ClientEntity>> | |||
|
|||
@GET(APIEndPoint.CLIENTS + "/{clientId}") | |||
suspend fun getClient(@Path("clientId") clientId: Int): ClientEntity | |||
fun getClient(@Path("clientId") clientId: Int): Flow<ClientEntity> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
@@ -109,7 +110,7 @@ class DataManagerClient( | |||
* @param clientId for Query in database or REST API request. | |||
* @return The Client Details | |||
*/ | |||
suspend fun getClient(clientId: Int): ClientEntity { | |||
fun getClient(clientId: Int): Flow<ClientEntity> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
|
||
suspend fun deleteClientImage(clientId: Int) | ||
|
||
suspend fun getClientAccounts(clientId: Int): ClientAccounts | ||
fun getClientAccounts(clientId: Int): Flow<DataState<ClientAccounts>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
} | ||
|
||
override suspend fun getClient(clientId: Int): ClientEntity { | ||
override fun getClient(clientId: Int): Flow<DataState<ClientEntity>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
@biplab1 I see some places you changed suspend to flow which is correct but some places you should keep it suspend.
Problem in Current Code Currently, methods like repository.getClient(...) returning Flow<DataState>, but in GetClientDetailsUseCase, we're treating it as if they were suspend functions:
This leads to a mismatch. Flow emits over time, and you can't await() a Flow directly. While we could use .first() or .collect(), that adds unnecessary overhead when the API call is just a one-shot operation. Change it like this:
DataManagerClient:
Repository:
RepositoryImpl:
|
@HekmatullahAmin Thanks for your detailed review! I will look into all your suggestions and implement things accordingly. |
Fixes - Jira-#400
I would like to thank @itsPronay for the fruitful discussions and his invaluable help in completing this PR.
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew check
orci-prepush.sh
to make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.