[Financial Connections] Adds Mavericks to handle state-based UIs. #4986
Conversation
carlosmuvi-stripe
force-pushed
the
poc-mavericks
branch
from
bcfd21c
to
1330e6d
Compare
|
Diffuse output: APKMANIFESTDEX |
# Conflicts: # dependencies.gradle # financial-connections/build.gradle
| /** | ||
| * Restores existing persisted fields into the current [FinancialConnectionsSheetState] | ||
| */ | ||
| internal fun from(savedStateHandle: SavedStateHandle): FinancialConnectionsSheetState { | ||
| return copy( | ||
| manifest = savedStateHandle.get(KEY_MANIFEST) ?: manifest, | ||
| authFlowActive = savedStateHandle.get(KEY_AUTHFLOW_ACTIVE) ?: authFlowActive, | ||
| ) | ||
| } |
There was a problem hiding this comment.
There's no need to handle savedStateHandle with the @PersistState annotation.
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
| import javax.inject.Named | ||
|
|
||
| @Suppress("LongParameterList", "TooManyFunctions") | ||
| internal class FinancialConnectionsSheetViewModel @Inject constructor( | ||
| @Named(APPLICATION_ID) private val applicationId: String, | ||
| private val starterArgs: FinancialConnectionsSheetActivityArgs, |
There was a problem hiding this comment.
We'll use the state at the only source of truth, and retrieve initial args from there.
| onFatal(it) | ||
| }.onSuccess { | ||
| openAuthFlow(it) | ||
| withState { state -> |
There was a problem hiding this comment.
state is read via withState. It ensures all the pending state changes are commited.
| // stores manifest in state for future references. | ||
| _state.updateAndPersist { | ||
| it.copy( | ||
| setState { |
There was a problem hiding this comment.
state is set using setState.
carlosmuvi-stripe
changed the title
| ) { | ||
| @PersistState val manifest: FinancialConnectionsSessionManifest? = null, | ||
| @PersistState val authFlowActive: Boolean = false, | ||
| val viewEffect: FinancialConnectionsSheetViewEffect? = null |
There was a problem hiding this comment.
Now viewEffects are part of the state, ensuring we commit to an Unidirectional Data Flow.
- Recommended reading about why effects should not live out of the state: https://developer.android.com/jetpack/guide/ui-layer/events#consuming-trigger-updates
| constructor(args: FinancialConnectionsSheetActivityArgs) : this( | ||
| initialArgs = args | ||
| ) |
There was a problem hiding this comment.
instead of having to handle fragment args manually, Mavericks calls this constructor passing down the args so state can be used as the only source of truth.
| fetchManifest() | ||
| } else { | ||
| onActivityRecreated() |
There was a problem hiding this comment.
unrelated to mavericks - moved this out of the activity and handled in viewModel.
carlosmuvi-stripe
requested review from
brnunes-stripe,
michelleb-stripe,
skyler-stripe,
ccen-stripe,
jameswoo-stripe and
awush-stripe
as code owners
| /** | ||
| * handle state changes here. | ||
| */ | ||
| override fun invalidate() { | ||
| withState(viewModel) { state -> | ||
| if (state.viewEffect != null) { | ||
| when (state.viewEffect) { | ||
| is OpenAuthFlowWithUrl -> state.viewEffect.launch() | ||
| is FinishWithResult -> finishWithResult(state.viewEffect.result) | ||
| } | ||
| viewModel.onViewEffectLaunched() | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When state changes, invalidate gets triggered. State updates in the UI should be handled here.
carlosmuvi-stripe
force-pushed
the
poc-mavericks
branch
from
cf0c027
to
8b531d2
Compare
| viewModel.onActivityResult() | ||
| } | ||
|
|
||
| private val viewModel: FinancialConnectionsSheetViewModel by activityViewModel() |
There was a problem hiding this comment.
would a fragmentViewModel or navGraphViewModel be applicable here?
There was a problem hiding this comment.
we need an activityViewModel to receive the onNewIntent callback, captured by the activity, on this ViewModel shared with the fragment.
| } | ||
| supportFragmentManager.beginTransaction() |
There was a problem hiding this comment.
since there's only one fragment, is it possible to declare it deirectly in xml?
xmlns:tools="http://schemas.android.com/tools"
android:id="@+id/nav_host_fragment"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical"
android:name="com.stripe.android.financialconnections.FinancialConnectionsSheetFragment"
tools:context=".FinancialConnectionsSheetActivity" />
There was a problem hiding this comment.
Also why do we change the Activity to Fragment? Is it required by Mavericks?
There was a problem hiding this comment.
It is supported, but they recommend using fragments, as "Putting UI directly inside of an Activity is no longer recommended in general for Android." (airbnb/mavericks#616 (comment))
However if Compose will be used to build the UI, the activity is still a recommended approach - https://github.com/airbnb/mavericks/blob/main/sample-compose/src/main/java/com/airbnb/mvrx/compose/sample/ComposeSampleActivity.kt so the fragment would be removed in that case.
There was a problem hiding this comment.
Actually, rethinking about it, the fragment doesn't bring any value on this case as there's no UI currently. Moved things back to the activity - aaa6b24
| @@ -21,15 +21,15 @@ import javax.inject.Singleton | |||
| internal interface FinancialConnectionsSheetComponent { | |||
| val viewModel: FinancialConnectionsSheetViewModel | |||
|
|
|||
| fun inject(factory: FinancialConnectionsSheetViewModel.Factory) | |||
| fun inject(factory: FinancialConnectionsSheetViewModel.Companion) | |||
There was a problem hiding this comment.
The reason we have the inject method here is to ensure the factory was injectable when a fallback component injection is created, it needs to be explicitly called(e.g here), since fallback is not yet implemented, we could remove this method
There was a problem hiding this comment.
Forgot to remove it e5e6fbc
|
|
||
| internal class FinancialConnectionsInitializer : ContentProvider() { | ||
| override fun onCreate(): Boolean { | ||
| Mavericks.initialize(context = requireNotNull(context)) |
There was a problem hiding this comment.
👍 I wonder since this is already here if we could start move daggers here too(not needed in this PR)
There was a problem hiding this comment.
Yeah we actually can!
| */ | ||
| override fun invalidate() { | ||
| withState(viewModel) { state -> | ||
| if (state.viewEffect != null) { |
There was a problem hiding this comment.
nit: use state.viewEffect?.let {} or add an else block to when(state.viewEffect) for the null case
| withState(viewModel) { state -> | ||
| if (state.viewEffect != null) { | ||
| when (state.viewEffect) { | ||
| is OpenAuthFlowWithUrl -> state.viewEffect.launch() |
There was a problem hiding this comment.
I think you can directly define the launch() function inside class OpenAuthFlowWithUrl instead of an extension, the when/is check will be able to tell the type and no downcast is needed here
There was a problem hiding this comment.
you mean by adding a launch abstract function to the ViewEffect sealed class?
each viewEffect uses different dependencies hosted by the fragment, so we can't have a unified launch. However I don't think we need that much level of function extraction, we can simplify here removing the launch entirely: ea7d683
|
Thanks for taking a look at this @ccen-stripe !
That's a good point that I still need to investigate. We might need to communicate that we're integrating Mavericks in our docs, probably mentioning how to configure I opened this issue in Mavericks asking for SDK integration concerns. Might followup with this! |
carlosmuvi-stripe
requested review from
ccen-stripe and
mshafrir-stripe
and removed request for
jameswoo-stripe
carlosmuvi-stripe
added
the
financial-connections
Summary
Motivation
Establish a consistent presentation architecture across the Financial Connections SDK where screens are modeled as a function of state. This is a useful concept because it's:
Also, we get additional features:
Testing