[FC] Moves Activities and some Panes out of Mavericks #8125
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Diffuse output: APKMANIFESTDEXARSC |
carlosmuvi-stripe
changed the title
Comment on lines
18
to
19
| onSuccess: (T) -> Unit = {}, | ||
| onFail: (Throwable) -> Unit = {} |
There was a problem hiding this comment.
Added some side effect callbacks that should replace the onAsync behavior (IMO code is easier to follow if side effects follow the operation execution)
carlosmuvi-stripe
changed the title
carlosmuvi-stripe
force-pushed
the
carlosmuvi/poc-remove-mavericks
branch
from
f5bc4d0 to
ce3643e
Compare
carlosmuvi-stripe
changed the base branch from
master
to
carlosmuvi/i/remove-mavericks
| initialState: S | ||
| ) : ViewModel() { | ||
|
|
||
| val stateFlow: MutableStateFlow<S> = MutableStateFlow(initialState) |
There was a problem hiding this comment.
Should keep this private and expose a non-mutable StateFlow.
.../src/main/java/com/stripe/android/financialconnections/core/FinancialConnectionsViewModel.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| protected fun setState(reducer: S.() -> S) { | ||
| stateFlow.update { state -> state.reducer() } |
There was a problem hiding this comment.
Another super nit!
Suggested change
| stateFlow.update { state -> state.reducer() } | |
| stateFlow.update(reducer) |
| } | ||
| } | ||
|
|
||
| internal sealed class Result<out T>( |
There was a problem hiding this comment.
It’s not technically a result. Should we call this AsyncState or something more along those lines?
There was a problem hiding this comment.
what about we just call it Async? Feels simple and short : )
| * retained component) to the factory to facilitate its creation via dependency injection. | ||
| */ | ||
| @Composable | ||
| internal inline fun <reified T : FinancialConnectionsViewModel<S>, S> paneViewModel( |
Comment on lines
+125
to
+127
| fun factory(parentComponent: FinancialConnectionsSheetNativeComponent): ViewModelProvider.Factory = | ||
| viewModelFactory { | ||
| initializer { |
| .map { it.viewEffect } | ||
| .distinctUntilChanged() | ||
| .collect { viewEffect -> | ||
| if (viewEffect == null) return@collect |
There was a problem hiding this comment.
How about a filterNotNull above?
| @@ -161,7 +182,7 @@ internal class FinancialConnectionsSheetNativeViewModel @Inject constructor( | |||
| */ | |||
| fun onResume() = viewModelScope.launch { | |||
There was a problem hiding this comment.
Do we still need the coroutine scope now?
There was a problem hiding this comment.
yep! still needed for the mutex
| .map { it.viewEffect } | ||
| .distinctUntilChanged() | ||
| .collect { viewEffect -> | ||
| if (viewEffect == null) return@collect |
There was a problem hiding this comment.
Same filterNotNull suggestion.
| @@ -281,7 +281,7 @@ class FinancialConnectionsSheetViewModelTest { | |||
| viewModel.handleOnNewIntent(errorIntent) | |||
|
|
|||
| // Then | |||
| withState(viewModel) { | |||
| viewModel.stateFlow.value.let { | |||
There was a problem hiding this comment.
Maybe we can add some FinancialConnectionsViewModel specific test extensions like Mavericks had. What do you think?
There was a problem hiding this comment.
good idea! noted for future PRs : )
…alconnections/core/FinancialConnectionsViewModel.kt Co-authored-by: Till Hellmund <tillh@stripe.com>
| import kotlinx.coroutines.flow.update | ||
| import kotlinx.coroutines.launch | ||
|
|
||
| internal abstract class FinancialConnectionsViewModel<S>( | ||
| initialState: S | ||
| ) : ViewModel() { | ||
|
|
||
| val stateFlow: MutableStateFlow<S> = MutableStateFlow(initialState) | ||
| private val _stateFlow: MutableStateFlow<S> = MutableStateFlow(initialState) | ||
| val stateFlow: StateFlow<S> = _stateFlow |
There was a problem hiding this comment.
Suggested change
| val stateFlow: StateFlow<S> = _stateFlow | |
| val stateFlow: StateFlow<S> = _stateFlow.asStateFlow() |
| @@ -5,7 +5,7 @@ import kotlin.test.Test | |||
| import kotlin.test.assertEquals | |||
| import kotlin.test.assertNull | |||
|
|
|||
| class GooglePayResultTest { | |||
| class GooglePayAsyncTest { | |||
carlosmuvi-stripe
requested review from
jaynewstrom-stripe
and removed request for
a team
carlosmuvi-stripe
merged commit 8b8043c
into
carlosmuvi/i/remove-mavericks
11 of 12 checks passed
carlosmuvi-stripe
added a commit
that referenced
this pull request
Mar 27, 2024
* [FC] Moves Activities and some Panes out of Mavericks (#8125) * Removes mackericks references on Consent screen. * Updates files. * Uses viewmodel factory builder. * Updates functions. * Updates execute. * Updates compose util. * Adds missing side effects. * Simplifies code. * Renames viewmodel. * Updates async. * Removes mavericks from institution picker. * Reverts rename. * Updates baseline. * Adds setState and persists state. * Renames result error to fail. * Migrates Initial activity out of mavericks. * Removes persist state. * Updates tests. * Regenerates API. * filterNotNull. * Nits. * Updates async. * use suspend block. * Update financial-connections/src/main/java/com/stripe/android/financialconnections/core/FinancialConnectionsViewModel.kt Co-authored-by: Till Hellmund <tillh@stripe.com> * Tries onAsync. * Moves activity to stripe ui core. * PR feedback. * Regenerates deps. --------- Co-authored-by: Till Hellmund <tillh@stripe.com> * [FC] Removes mavericks from repositories and more panes. (#8154) * Migrates more screens out of mavs. * Migrates partner auth. * Removes active auth session field. * Updates tests. * Updates attach payment viewmodel. * PR feedback. * Api dump. * [FC] Removes mavericks from all viewmodels and tests (#8155) * Migrates more screens out of mavs. * Migrates partner auth. * Removes active auth session field. * Updates tests. * Updates attach payment viewmodel. * PR feedback. * Api dump. * Migrates missing viewmodels. * Updates dependencies. * PR feedback. * Merge master. * [FC] Removes mavericks dependency (#8160) * Migrates more screens out of mavs. * Migrates partner auth. * Removes active auth session field. * Updates tests. * Updates attach payment viewmodel. * PR feedback. * Api dump. * Migrates missing viewmodels. * Updates dependencies. * Removes mavericks dependency. * Merge with integration. * Uses collect. * Updates dependencies * Updates Changelog. * Update CHANGELOG.md Co-authored-by: Till Hellmund <tillh@stripe.com> --------- Co-authored-by: Till Hellmund <tillh@stripe.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
First PR removing Mavericks on both parent activities, as well as Consent and Institution Picker (Composable-based panes).
Result(replacesAsync).SavedStateHandleto save and retrieve fields on process death (replacing @PersistState)MavericksViewModelFactoryhelps retrieving the parent activity with itsViewModelContextWe use that to retrieve the activity viewmodel, that exposes the parent Dagger Component to build screen SubComponents. Replaced by a helper function that retrieves the parent component directly.Motivation
Testing
@PersistState.