joreilly / FantasyPremierLeague

Fantasy Premier League Kotlin Multiplatform sample using Jetpack Compose, Compose for Desktop and SwiftUI (and Room for local persistence)
Apache License 2.0
428 stars 48 forks source link

ViewModel lifecycle on SwiftUI against Compose #231

Open frankois944 opened 1 month ago

frankois944 commented 1 month ago

As your sample, we want to share the viewmodel on SwiftUI and Compose.

But there is a big difference between them, it's how a viewmodel is initialised, used and destroyed.

On SwiftUI, for exemple, the struct containing the view definition is initialised even the view is not displayed, as intended on SwiftUI with navigationLink, a good exemple is a list containing for each item a navigation link to a view containing a viewmodel.

So we can't put heavy code in the viewmodel constructor because it can be useless or cause memory waste, like data preloading or listener...

It's not the case on Compose, the viewmodel is initialised when the view will be displayed.

Currently, viewmodel data are loaded when it's bound/subscribed to the view. Because of SharingStarted.Lazily,

val allPlayers = repository.getPlayers()
    .stateIn(viewModelScope, SharingStarted.Lazily, emptyList())

We can share logic between SwiftUI and Compose, but the lifecycle is clearly different; the constructor and the fun onCleared() don't have the same meaning.

I found some dirty solutions that could work, but no proper solution.

Let's talk about it

frankois944 commented 1 month ago

After some experiences, purely managing lifecycle from the viewmodel is so wrong with SwiftUI.

the viewModelScope is poorly managed, we need to do it manually πŸ’£

After some exploration : https://github.com/touchlab/SKIE/issues/80#issuecomment-2124895484

ColtonIdle commented 1 month ago

I'm not really sure as I'm still a KMP noob and such. But. For the case of initialization... I know it's pretty much an anti pattern/not recommended to do stuff as part of a ViewModels init method. I have typically always used init for this sort of initialization work but I got into trouble with this a few months ago and brought it up on kotlinlang. Basically I was modifying snapshot state before the VM was actually ready and touching snapshot state in a VM init was crashing at like a 5% rate in production... Only for a specific VM though. Very weird. But now I don't use init in VM anymore. Lol

Also. I think Ian Lake said that even though VM and lifecycle are now compiled for KMP, they aren't actually hooked into iOS lifecycle and someone (jet brains?) would still actually have to do that work. (I don't want to misrepresent what Ian said so take that with a grain of salt).

frankois944 commented 1 month ago

I'm not really sure as I'm still a KMP noob and such. But. For the case of initialization... I know it's pretty much an anti pattern/not recommended to do stuff as part of a ViewModels init method. I have typically always used init for this sort of initialization work but I got into trouble with this a few months ago and brought it up on kotlinlang. Basically I was modifying snapshot state before the VM was actually ready and touching snapshot state in a VM init was crashing at like a 5% rate in production... Only for a specific VM though. Very weird. But now I don't use init in VM anymore. Lol

Also. I think Ian Lake said that even though VM and lifecycle are now compiled for KMP, they aren't actually hooked into iOS lifecycle and someone (jet brains?) would still actually have to do that work. (I don't want to misrepresent what Ian said so take that with a grain of salt).

It's not about only the init of the viewmodel but the lifecycle. Official KMP ViewModel aren't made be compatible with SwiftUI, we need to do some glue to fix the missing part.

rickclephas commented 1 month ago

I think destroying the VM (clear/onCleared) maps nicely to deinit in Swift.

As for the initialization it depends on the navigation logic used in SwiftUI. Some of the "older" approaches indeed create VMs (or similar objects) before the actual views will be shown. Other approaches such as NavigationStack don't have this problem.

In terms of behaviour a "Swift approach" that doesn't perform (much) work in a VM constructor would also work for Compose/Kotlin.

IMO VMs created by SwiftUI before the view is ever required is a bug, and I have worked around that even in pure Swift projects.

frankois944 commented 1 month ago

I think destroying the VM (clear/onCleared) maps nicely to deinit in Swift.

If we could do that easly, it would be great. The clear method is internal and I guess Compose specific.

I thought, manually cancelling the viewModelScope should be enough for cleaning the viewmodel from SwiftUI.

As for the initialization it depends on the navigation logic used in SwiftUI. Some of the "older" approaches indeed create VMs (or similar objects) before the actual views will be shown.

I think, we should emulate the KMP ViewModel lifecycle from SwiftUI StateObject lifecycle by wrapping it inside a ObservableObject.

Something like this :


@StateObject var viewModel = KTViewModel<MyViewModel>(.init())

class KTViewModel<T : Lifecycle_viewmodelViewModel> : ObservableObject {

    let instance: T

    init(_ viewModel: T) {
        self.instance = viewModel
    }

    deinit {
        self.instance.onCleared()
    }
}
override fun onCleared() {
    super.onCleared()
    if (viewModelScope.isActive) {
        println("[onCleared] Cancelling viewModelScope")
        viewModelScope.cancel("Cancelling viewModelScope for iOS Target")
    }
    println("[onCleared] MainScreenViewModel")
}

Reproducing the expected lifecycle of SwiftUI fix a lot of things, as the ViewModel is now correctly initialized/deinit, and we have the same behavior as a swift ViewModel.

Other approaches such as NavigationStack don't have this problem.

NavigationStack if Apple could make it accessible to every SwiftUI version... never!

frankois944 commented 1 month ago

@joreilly The main issue here : on iOS, if you don't cancel the viewModelScope, the callback onCompletion of the flows using .stateIn won't be call and this is an issue. Also, the viewmodel won't be properly cleared.

The viewmodel behavior between iOS/Android is currently different, but it should be the same to avoid bugs.

On Compose, it's not an issue as the viewmodel lifecycle is properly handled, but on iOS it must be manually handled.

joreilly commented 1 month ago

Been a bit crazy here and haven't had chance to catch up with this yet (among other things :) ) . If you do have particular changes in mind then would definitely welcome a PR .

frankois944 commented 1 month ago

Been a bit crazy here and haven't had chance to catch up with this yet (among other things :) ) . If you do have particular changes in mind then would definitely welcome a PR .

Thanks for the reply,

I have explain an solution https://github.com/joreilly/FantasyPremierLeague/issues/231#issuecomment-2126440053

It's about wrapping the Kotlin ViewModel inside a SwiftUI ObservableObject.

I worked on my side to do it the cleanest way, I will do a PR.

joreilly commented 1 month ago

@frankois944 one question I have from above is related to the SKIE issue you mentioned and comment from @TadeasKriz about using SharingStarted.WhileSubscribed() instead....wondering if that would impact things here (e.g. how cancellation takes place). BTW I've just started using SKIE's new Observing functionality in the project (for one view model as initial test but will add to others)...again not sure if this has any implications as well re. lifecycle.

joreilly commented 1 month ago

I'm not sure tbh why I'm using Lazily here! Quite possible that WhileSubscribed() is better approach.

frankois944 commented 1 month ago

@frankois944 one question I have from above is related to the SKIE issue you mentioned and comment from @TadeasKriz about using SharingStarted.WhileSubscribed() instead....wondering if that would impact things here (e.g. how cancellation takes place). BTW I've just started using SKIE's new Observing functionality in the project (for one view model as initial test but will add to others)...again not sure if this has any implications as well re. lifecycle.

SharingStarted.WhileSubscribed() will work for sure

But, is it the onCompletion event of the flow is called when the viewmodel is destroyed?

As the stateflow lifecycle is bound to the viewModelScope with .stateIn, I guess it needs to be cancelled to properly cancel the flow like @TadeasKriz said.

Just add the onCompletion to the flow and observe if it's called or not on iOS and Android.

You can do some testing on the SettingsView.swift, it's the only one who is displayed by navigation.

See my PR: https://github.com/joreilly/FantasyPremierLeague/pull/237

frankois944 commented 1 month ago

I'm not sure tbh why I'm using Lazily here! Quite possible that WhileSubscribed() is better approach.

It was me, I did the update but I didn't understand the origin of the issue well.

joreilly commented 1 month ago

Ah, ok....good example of what a terrible memory I have :)

TadeasKriz commented 1 month ago

Lazily always starts collecting the backing Flow in the provided scope (viewModelScope in this case) with the first subscriber. WhileSubscribed does basically reference counting. So there's a semantic difference between the two, which needs to be taken into account.

@joreilly The new Observing doesn't really change that much, because it's still within the confines of KotlinX Coroutiens and doesn't interact with Android ViewModel.

But I've been prototyping a similar thing to the new Observing, but for handling Android ViewModel lifecycle. This could be added to SKIE as a preview in the near future. Stay tuned!

frankois944 commented 1 month ago

But I've been prototyping a similar thing to the new Observing, but for handling Android ViewModel lifecycle. This could be added to SKIE as a preview in the near future. Stay tuned!

It would be great if there are more compatibility between Swift and the Android ViewModel as it was not originally made for working with SwiftUI. Until our savior SKIE make the day πŸ˜„, we need to fill the hole ourselves.

TadeasKriz commented 1 month ago

Yeah, it's one of the reasons why I'm not enjoying Android ViewModel myself (iOS developer alert).

In my current prototyping I'm trying our Swift macros to see if that would be something we can leverage. There's always the .attach(viewModel: x) view modifier to fall back on, but that's easy to forget.

frankois944 commented 1 month ago

Yeah, it's one of the reasons why I'm not enjoying Android ViewModel myself (iOS developer alert).

Yes me too (and also SwiftUI ViewModel) πŸ˜„

For now, I will stick on my solution by wrapping the KMP/Android viewmodel and wait for a smoother way.

joreilly commented 1 month ago

Chatting to @marcellogalhardo and he had some suggestions around use of ViewModelStoreOwner that look interesting. I'll try and get chance later to try them out (also still based on use of ObservableObject as you have @frankois944 )

joreilly commented 1 month ago

Needs cleanup but this is example based on @marcellogalhardo's suggestion https://github.com/joreilly/FantasyPremierLeague/pull/239.

joreilly commented 1 month ago

That's been merged now

frankois944 commented 1 month ago

@joreilly Wow, long way to access at the internal clear method of the ViewModel, but at least everything can be done from Swift without adding code on the shared project.

I think the SharedViewModelStoreOwner could be improved, but I need to do some testing.

frankois944 commented 1 month ago

About this issue, it also applies on UIKit project, it's easier as the lifecycle of a ViewController is simpler than SwiftUI.

At least, we need a common entry point which is inside SharedViewModelStoreOwner.swift

But as the current project is in SwiftUI it's not the subject.

joreilly commented 1 month ago

I think the SharedViewModelStoreOwner could be improved

Absolutely. There's a few known issues right now around lifecycle/scope etc. @marcellogalhardo' I believe is also actively working on how best to structure this (in general) including for example figuring out how this would work with navigation setup.

frankois944 commented 1 month ago

@joreilly You have an error on iOS > Accessing StateObject's object without being installed on a View. This will create a new instance each time.

https://github.com/joreilly/FantasyPremierLeague/blob/0e9508d5b187f082c020af51688bcd0329a1fe5b/ios/FantasyPremierLeague/FantasyPremierLeague/Fixtures/FixtureListView.swift#L8-L9

You shouldn't put the ViewModel into a @State, it's already hold by the @StateObject. you need to access to the viewmodel directly from viewModelStoreOwner.instance

The usage of the Android Viewmodel is the same as a SwuiftUI ViewModel (at least of the instance)

joreilly commented 1 month ago

Thanks. This was hacked together pretty quickly and definitely needs a few updates. Tadeas had also mentioned @State issue to me as well....will update later today

frankois944 commented 1 month ago

In my current prototyping I'm trying our Swift macros to see if that would be something we can leverage. There's always the .attach(viewModel: x) view modifier to fall back on, but that's easy to forget.

@TadeasKriz I'm also personnaly going on macro for removing the the boilerplate on init the instance and data binding

@sharedViewModel(ofType: MainScreenViewModel.self,
                 publishing: (\.mainScreenUIState, MainScreenUIState.self), (\.userId, String?.self)
)
class MyMainScreenViewModel: ObservableObject {}

Using SKIE Combine API, i can generate the code for binding (@Publish) the viewmodel to the view like a SwiftUI ViewModel and respect the lifecycle, it's less painfull as a iOS developer to avoid declaring @State properties and manually binding them as currently made in the project.

TadeasKriz commented 1 month ago

You don't need to declare @State and manually bind them if you use the new Observing(..) view that's in SKIE (https://skie.touchlab.co/features/flows-in-swiftui).

One very important thing to keep in mind, ObservableObject sends "will change" notification, but collecting a StateFlow is always "did change". So the value is already changed when you trigger objectWillChange which usually leads to weirdness in behavior and animations in SwiftUI which are difficult to debug and close to impossible to fix.

joreilly commented 1 month ago

@frankois944 re.

you need to access to the viewmodel directly from viewModelStoreOwner.instance

I had done that in initial cut of the code but was concerned if issue in having to do that retrieval every time there's a recomposition of the view.

frankois944 commented 1 month ago

@frankois944 re.

you need to access to the viewmodel directly from viewModelStoreOwner.instance

I had done that in initial cut of the code but was concerned if issue in having to do that retrieval every time there's a recomposition of the view.

It just like SwiftUI viewmodel, @StateObject is made to manage the lifecycle of the ObservableObject.

You should explore the lifecycle of the ObservableObject by printing some logs on init/deinit of the @StateObject or @State, you will see a big difference of the behavior.

Edit : It's better when there are some navigation for triggering the deinit πŸ˜„

frankois944 commented 1 month ago

This is my playground, kind of dirty but I'm exploring https://github.com/frankois944/kmp-mvvm-exploration.

My main goal is to be close of the usage of the SwiftUI MVVM with the Android ViewModel.