FantasyPremierLeague icon indicating copy to clipboard operation
FantasyPremierLeague copied to clipboard

ViewModel lifecycle on SwiftUI against Compose

Open frankois944 opened this issue 1 year ago • 31 comments

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 avatar May 22 '24 11:05 frankois944

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

frankois944 avatar May 22 '24 14:05 frankois944

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).

ColtonIdle avatar May 22 '24 14:05 ColtonIdle

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.

frankois944 avatar May 22 '24 14:05 frankois944

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.

rickclephas avatar May 22 '24 15:05 rickclephas

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 avatar May 23 '24 07:05 frankois944

@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.

frankois944 avatar May 28 '24 09:05 frankois944

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 .

joreilly avatar May 28 '24 09:05 joreilly

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.

frankois944 avatar May 28 '24 09:05 frankois944

@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 avatar May 28 '24 10:05 joreilly

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

joreilly avatar May 28 '24 11:05 joreilly

@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 avatar May 28 '24 12:05 frankois944

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.

frankois944 avatar May 28 '24 12:05 frankois944

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

joreilly avatar May 28 '24 12:05 joreilly

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!

TadeasKriz avatar May 28 '24 13:05 TadeasKriz

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.

frankois944 avatar May 28 '24 13:05 frankois944

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.

TadeasKriz avatar May 28 '24 13:05 TadeasKriz

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.

frankois944 avatar May 28 '24 14:05 frankois944

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 avatar May 28 '24 14:05 joreilly

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

joreilly avatar May 28 '24 16:05 joreilly

That's been merged now

joreilly avatar May 28 '24 18:05 joreilly

@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 avatar May 29 '24 07:05 frankois944

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.

frankois944 avatar May 29 '24 07:05 frankois944

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.

joreilly avatar May 29 '24 08:05 joreilly

@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)

frankois944 avatar May 29 '24 12:05 frankois944

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

joreilly avatar May 29 '24 12:05 joreilly

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.

frankois944 avatar May 29 '24 14:05 frankois944

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.

TadeasKriz avatar May 29 '24 14:05 TadeasKriz

@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.

joreilly avatar May 29 '24 17:05 joreilly

@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 avatar May 30 '24 08:05 frankois944

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.

frankois944 avatar May 30 '24 09:05 frankois944