-
Notifications
You must be signed in to change notification settings - Fork 192
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
ViewModel Refactor Concept - Expect/Actual #218
Conversation
I wonder if it would be to constricting to have the |
@mrf7 That's possible. However, having that would "enforce" how the user should expose the data. In this concept, the ViewModel should be open to be used as required and needed in each project scope. |
Good point, something like that would be easy enough for users to extend and add to suit the needs of their projects. I've seen similar patterns of custom generic api formats wrapped around Androids VM, so you can just do the same here |
Yep, that probably will work better. Have a "generic shared" view model library and other libraries to integrate with the platform code. Like is on Android ViewModel and LiveData. |
@@ -65,6 +62,7 @@ kotlin { | |||
implementation(libs.touchlab.stately) | |||
implementation(libs.multiplatformSettings.common) | |||
implementation(libs.kotlinx.dateTime) | |||
api(project(":viewmodel")) |
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.
Any reason to have this in a separate module? Are you thinking about the viewmodel stuff as its own library?
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.
The idea was just to keep the code separated. If we want to create a library, it's halfway through.
// | ||
// Solution: This class inherits from KoinComponent and I can inject the parameters directly here | ||
// | ||
// TODO: PoC of iOS ViewModelFactory (this way we can move dependencies to constructor) |
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 definitely like the idea of moving things to constructor dependencies (we should do this for BreedModel
too)
// All state that will be public to the platforms have to be protected | ||
// This way, each platform view model can expose as they can handle. | ||
// | ||
// If for some reason the way some platform expose the data changes, the common class | ||
// and logic stays the same. | ||
// | ||
// See child implementations for more details | ||
protected val _breedStateFlow: MutableStateFlow<DataState<ItemDataSummary>> = | ||
MutableStateFlow(DataState(loading = true)) |
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.
You might be able to simplify things a bit by going back to a public (non-mutable) StateFlow
in this class that Android uses directly, and an extension function on iOS to pass the callback and kick off observation.
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've just pushed a commit with this change.
It looks much more straightforward and removes the inheritance for each platform.
@@ -40,7 +40,8 @@ class ObservableBreedModel: ObservableObject { | |||
} | |||
|
|||
func deactivate() { | |||
viewModel?.onDestroy() | |||
// Manually call to dispose is not a good solution |
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.
To some extent, this is inescapable. This ultimately needs to get called from the right part of the iOS UI lifecycle and that can't happen from Kotlin.
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.
Agreed. But maybe we can find some solution on the iOS side to help us here.
actual abstract class PlatformViewModel : ViewModel() { | ||
@CallSuper | ||
protected actual override fun onCleared() { | ||
super.onCleared() | ||
} | ||
} |
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 hate that you can't do actual typealias PlatformViewModel = ViewModel
here
PR #238 is a merge of the view model concept PRs. Closing this one. |
This is another proposal of a shared ViewModel.
Common ViewModel concept
Using the expect/actual modifiers provided by Kotlin, we can create a common ViewModel declaration. It can be pretty straightforward, with just one method that will run when the view is being destroyed to finish any work in progress.
This proposal keeps the Android standard of using the
onCleared
to perform such action.To provide the correct coroutine scope in the shared code, we will have an extension function (using expect/actual), similar to what is done by Android Lifecycle Components.
This abstract class will be used to implement a shared ViewModel (like a
CommonBreedViewModel
). But it will not be visible for the apps. Each platform will have to implement a subclass from this ViewModel exposing the data so that the platform's native language can access.We can have an Android implementation that just exposes some StateFlow (or LiveData/Rx):
And on iOS, we can use constructor callbacks (or SwiftCoroutines/Hyperdrive):
Pain points
During the development of this PoC, I was able to experience some pain points that need to be discussed and stress the current (and other possible) solutions. All of them are placed as comments in the code, but here are a few of them:
Dependencies
The first problem I had with it was about having the dependencies in the common code. The problems are:
ViewModelProvider
(or similar) for iOS would helpinit
from the common class;The solution used here was to make the
CommonViewModel
aKoinComponent
, with that, we can callby inject()
directly in the shared code.Native Coroutine Scope
Right now, the
MainScope
class depends on Kermit to run. To avoid forcing the user to import unwanted libraries, I created a new class that provides the coroutine scope calledNativeCoroutineScope
. It now does not depend on any external library, but we lose the logging capability.Some solutions for that are:
Logger
class that we send as a parameter for the native ViewModel may be a solution.iOS Lifecycle
In this implementation, we need to manually call the
.destroy()
function on the iOS side to dispose of the ViewModel. A way to attach to a ViewController lifecycle would be helpful.Pros
Cons