ViewModel refactor - common ViewModel by composition #231
Conversation
|
|
||
| val breedStateFlow: StateFlow<DataState<ItemDataSummary>> = _breedStateFlow | ||
| val breeds = commonViewModel.breeds |
There was a problem hiding this comment.
Maybe get() accessor vs keeping a state reference here? Seems like a simple passthrough.
| private val _breedStateFlow: MutableStateFlow<DataState<ItemDataSummary>> = MutableStateFlow( | ||
| DataState(loading = true) | ||
| ) | ||
| private val commonViewModel = BreedCommonViewModel(breedRepository, log, viewModelScope) |
There was a problem hiding this comment.
I'm wondering, do we need to hide the commonViewModel and duplicate it's signature here? Is there any reason not to have this classes responsibility be just creating the common one and making it accessible to callers? I think the main reason would be confusion at the call site breedViewModel.commonViewModel.refreshBreeds() is obviously pretty ugly. But with some improved naming, might be worth it to avoid maintaining the duplicated signatures?
There was a problem hiding this comment.
In KaMPKit we don't, which is a refactor on my (undocumented, oops) todo list before merging this. In general though, you might want this separation if your platforms are not perfectly in-sync because it gives you a place to put android-specific stuff. It's also a place you can convert to LiveData or RxJava or anything else platform-specific you might be using in existing Android code.
| private val clock: Clock | ||
| ) { | ||
|
|
||
| private val log = log.withTag("BreedModel") |
There was a problem hiding this comment.
Don't need this anymore as the tag is added here, right?
| @@ -63,6 +64,16 @@ private val coreModule = module { | |||
| // See https://github.com/touchlab/Kermit | |||
| val baseLogger = Logger(config = StaticConfig(logWriterList = listOf(platformLogWriter())), "KampKit") | |||
| factory { (tag: String?) -> if (tag != null) baseLogger.withTag(tag) else baseLogger } | |||
|
|
|||
| single { | |||
| BreedRepository( | |||
There was a problem hiding this comment.
I think it'd be great for readability to have named arguments, especially since this is a widely referenced sample project.
|
PR #238 is a merge of the view model concept PRs. Closing this one. |
Moves VM logic to
BreedCommonViewModel, which is wrapped in anandroidx.lifecycle.ViewModelon Android and a customCallbackViewModelclass on iOS. TheBreedCallbackViewModelis wired intoObservableBreedModelin Swift which uses some different internals but keeps its existing interface.