rickclephas / KMP-ObservableViewModel

Library to use AndroidX/Kotlin ViewModels with SwiftUI
MIT License
569 stars 29 forks source link

ViewModel is deallocated immediately when used with NavigationStack #49

Closed angelix closed 11 months ago

angelix commented 11 months ago

In the following example, the ViewModel is instantiated and passed to a NavigationPath first and later is set to a View. Unfortunately this throws the following error:

KMMViewModelCore/ObservableViewModel.swift:34: Fatal error: ObservableViewModel has been deallocated

Is this caused by the path being @Published? Is there a way around it?

var body: some Scene {
        WindowGroup {
            NavigationStack(path: $navController.path) {
                RootView()
                    .navigationDestination(for: KMMViewModel.self) { destination in 
                        ViewFactory.viewForDestination(destination)
                    }
            }
            .environmentObject(navController)
        }
    }
class ViewFactory {
    @ViewBuilder
    static func viewForDestination(_ destination: KMMViewModel) -> some View {
        if let vm = destination as? TestViewModel {
            TestView(viewModel: vm)
        } else {
            EmptyView()
        }
    }
}
class NavController: ObservableObject {
I var path = NavigationPath()

    func show<VM>(_ vm: VM) where VM: KMMViewModel {
        path.append(vm)
    }
    ...
}
navController.show(TestViewModel() as KMMViewModel)
rickclephas commented 11 months ago

The Swift helper functions wrap your KMMViewModel in an ObservableViewModel. Something in your Swift code needs to hold a strong reference to this object since deallocation will result in your KMMViewModel being cleared. So instead of storing the KMMViewModel in your NavigationPath, you'll need to store the ObservableViewModel. Does the following work for you?:

func show<VM>(_ vm: VM) where VM: KMMViewModel {
    path.append(observableViewModel(for: vm))
}

RootView()
  .navigationDestination(for: ObservableViewModel<Kmm_viewmodel_coreKMMViewModel>.self) { destination in 
      ViewFactory.viewForDestination(destination.viewModel)
  }
angelix commented 11 months ago

I wasn't able to make it work with a generic type navigationDestination(for: ObservableViewModel<Kmm_viewmodel_coreKMMViewModel>.self).

I had to add the type explicitly like this

.navigationDestination(for: ObservableViewModel<Kmm_viewmodel_coreKMMViewModel>.self) { destination in
   TestView(viewModel: destination.viewModel)
}

But there is another error when force casting object to WeakObservableViewModel<ViewModel>) in observableViewModel func.

Could not cast value of type 'KMMViewModelCore.(unknown context at $104abfda4).WeakObservableViewModel<__C.CommonTestViewModel>' (0x1188120b8) to 'KMMViewModelCore.(unknown context at $104abfda4).WeakObservableViewModel<__C.CommonTestViewModel>' (0x11903d110).
Could not cast value of type 'KMMViewModelCore.
rickclephas commented 11 months ago

Hmm alright, could you possibly share some reproduction code for that?

angelix commented 11 months ago

I was able to identify the issue.

In my app i use an abstract class to declare all available data, an implementation class and a dummy preview class.

eg.


abstract class DemoViewModelAbstract(val isPreview: Boolean = false): BaseViewModel(isPreview = isPreview) {
    @NativeCoroutinesState
    abstract val counter: StateFlow<Int>
}

class DemoViewModel : DemoViewModelAbstract() {
    @NativeCoroutinesState
    val counter = MutableStateFlow(viewModelScope, 0)
}

class DemoViewModelPreview: DemoViewModelAbstract(isPreview = true) {
    val counter = MutableStateFlow(viewModelScope, 0)
}

Of course the actual implementation class is more complex with lot of dependencies etc. I was afraid that using the actual implementation classes in Previews, would slow down the preview generation. Not sure if thats is the case as i haven't migrated more complex ViewModels yet.

The issue arises when i use the abstract class as the variable type.

 struct RecoveryIntroView: View {
    @StateViewModel var viewModel: DemoViewModelAbstract // Crashes
    @StateViewModel var viewModel: DemoViewModel // works
}

Reproduced here. https://github.com/angelix/KMM-ViewModel/tree/ave-49

The crash report for reference:

Could not cast value of type 'KMMViewModelCore.(unknown context at $10442ce04).WeakObservableViewModel<KMMViewModelSample.TimeTravelViewModel>' (0x10b8655f8) to 'KMMViewModelCore.(unknown context at $10442ce04).WeakObservableViewModel<KMMViewModelSample.TimeTravelViewModelParent>' (0x111867cc0).
2023-09-19 19:06:45.485604+0300 KMMViewModelSample[78336:40961577] Could not cast value of type 'KMMViewModelCore.(unknown context at $10442ce04).WeakObservableViewModel<KMMViewModelSample.TimeTravelViewModel>' (0x10b8655f8) to 'KMMViewModelCore.(unknown context at $10442ce04).WeakObservableViewModel<KMMViewModelSample.TimeTravelViewModelParent>' (0x111867cc0).
rickclephas commented 11 months ago

@angelix I have updated the reference logic to no longer use the generics. Could you possibly double check if your project is working as expected with 4bd5d745e5b16f048924deb8db02cea8443b7205?

angelix commented 11 months ago

@angelix I have updated the reference logic to no longer use the generics. Could you possibly double check if your project is working as expected with 4bd5d74?

Works like a charm.

Thank you for the quick response and fix. Great work!

Fixed in 4bd5d745e5b16f048924deb8db02cea8443b7205