Tlaster / PreCompose

Compose Multiplatform Navigation && State Management
https://tlaster.github.io/PreCompose/
MIT License
847 stars 49 forks source link

[BUG] koinViewModel in precompose-koin not consistent across platform #292

Closed reginaldlouis closed 3 months ago

reginaldlouis commented 6 months ago

In precompose-koin version 1.6, for jvm, just calling koinViewModel<BudgetViewModel>() work, but for wasmJs I get this error:

No value passed for parameter 'vmClass'

When digging into precompose-koin, I can see that koinViewModel has 2 different signatures.

In commonMain/kotlin/moe/tlaster/precompose/koin/Koin.kt the signature is:

fun <T : ViewModel> koinViewModel(
    vmClass: KClass<T>,
    qualifier: Qualifier? = null,
    stateHolder: StateHolder = checkNotNull(LocalStateHolder.current) {
        "No StateHolder was provided via LocalStateHolder"
    },
    key: String? = null,
    scope: Scope = LocalKoinScope.current,
    parameters: ParametersDefinition? = null,
): T

And in jvmAndroidMain/kotlin/moe/tlaster/precompose/koin/Koin.jvmAndroid.kt the signature is:

inline fun <reified T : ViewModel> koinViewModel(
    qualifier: Qualifier? = null,
    stateHolder: StateHolder = checkNotNull(LocalStateHolder.current) {
        "No StateHolder was provided via LocalStateHolder"
    },
    key: String? = null,
    scope: Scope = LocalKoinScope.current,
    noinline parameters: ParametersDefinition? = null,
): T

No need to pass a vmClass: KClass<T> parameter. Why not have a consitent api across platform?

Tlaster commented 6 months ago

Because there's a bug for Kotlin/Native with Compose: https://github.com/JetBrains/compose-multiplatform/issues/3147, the doc dose mention this in here.

reginaldlouis commented 6 months ago

Thx

/RL

On Thu, Mar 28, 2024 at 11:11 PM Tlaster @.***> wrote:

Because there's a bug for Kotlin/Native with Compose: JetBrains/compose-multiplatform#3147 https://github.com/JetBrains/compose-multiplatform/issues/3147, the doc dose mention this in here https://github.com/Tlaster/PreCompose/blob/master/docs/component/view_model.md#:~:text=NOTE%3A%20If%20you%27re%20using%20Kotlin/Native%20target%2C%20please%20use%20viewModel%20with%20modelClass%20parameter%20instead .

— Reply to this email directly, view it on GitHub https://github.com/Tlaster/PreCompose/issues/292#issuecomment-2026545174, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNEDW3G5XTZKC4Q7ETDXQ3Y2TLWLAVCNFSM6AAAAABFNVY2H2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRWGU2DKMJXGQ . You are receiving this because you authored the thread.Message ID: @.***>

ColtonIdle commented 5 months ago

Looks like the bug was fixed? https://github.com/JetBrains/compose-multiplatform/issues/3147

Tlaster commented 5 months ago

Looks like the bug was fixed? JetBrains/compose-multiplatform#3147

Seems like the fix is included in compose compiler 1.5.10.2, so we can get generic inline composable function now!

ColtonIdle commented 5 months ago

@Tlaster seems like androidx now supports kmp lifecycle, navigation and viewmodel (at least in the alphas). Plans to wind down this library?

Tlaster commented 5 months ago

@Tlaster seems like androidx now supports kmp lifecycle, navigation and viewmodel (at least in the alphas). Plans to wind down this library?

I haven't decided yet, but since Google and Jetbrains officially releasing ViewModel and Navigation for multiplatform, maybe PreCompose have done its job.

ColtonIdle commented 5 months ago

🫡

Sent from Proton Mail Android

-------- Original Message -------- On 5/4/24 1:30 PM, Tlaster wrote:

@.***(https://github.com/Tlaster) seems like androidx now supports kmp lifecycle, navigation and viewmodel (at least in the alphas). Plans to wind down this library?

I haven't decided yet, but since Google and Jetbrains officially releasing ViewModel and Navigation for multiplatform, maybe PreCompose have done its job.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>