Add RxRedux sample #33
Conversation
|
Added a bit more about Redux in general |
| private val signInButton = AndroidButton(root.findViewById(R.id.sign_in_button), renderer) | ||
| private val resultTextView = AndroidTextView(root.findViewById(R.id.sign_result_text_view), renderer) | ||
|
|
||
| override val actions: Observable<SignInAction> = Observable |
There was a problem hiding this comment.
Isn’t it just a fancy .merge though?
There was a problem hiding this comment.
Ohh, definitely, I was like there is a way, but didn't want to spend too much time on it, will refactor, thanks
There was a problem hiding this comment.
foxed, thx
| data class ChangePassword(val password: CharSequence) : SignInAction() | ||
| object SignIn : SignInAction() | ||
| object ShowSigningInUi : SignInAction() | ||
| object ShowSignInSuccessfulUi : SignInAction() |
There was a problem hiding this comment.
Ah, *Ui* postfixes — good old days.
There was a problem hiding this comment.
Yeah, not happy about it either : (
Do you have other names on mind?
There was a problem hiding this comment.
Just SigningIn , SignInSuccessful, SignInFailure?
There was a problem hiding this comment.
hm, I thought Actions should be verbs, but ok
| object SignIn : SignInAction() | ||
| object ShowSigningInUi : SignInAction() | ||
| object ShowSignInSuccessfulUi : SignInAction() | ||
| data class ShowSignInFailureUi(val cause: Throwable) : SignInAction() |
There was a problem hiding this comment.
Never got a good reason why it is necessary to pass Throwable around.
There was a problem hiding this comment.
Can be logged or transformed to particular text (then it should be done not in View layer though)
There was a problem hiding this comment.
While passing around Throwable adds some value for logging and so on it also has some downsides: Usually you are not in control how the Throwable is created and hence testing via `assertEquals(expected = ShowSignInFailureUi(mockedException), actual) is hard.
There was a problem hiding this comment.
fair, I'll keep it for now because it's how it's done in other samples, created issue to track it #34
| @Test | ||
| fun `displays success ui`() { | ||
| inputActions.accept(SignInAction.ChangeEmail("test@email")) | ||
| inputActions.accept(SignInAction.ChangePassword("passw0rd")) |
There was a problem hiding this comment.
You can at least use JUnit 5 nesting.
There was a problem hiding this comment.
Honestly, I don't have time to deal with JUnit5 setup
|
If you simply pass the state it may change by the time you use it. StateAccessor is needed to access the current version of the state. |
|
Yeah I understand, that's why I said that I probably need more real life experience with long running async actions At the same time this kinda breaks the idea of functional pureness a bit, at least for me, maybe it's pragmatic tho |
| @@ -0,0 +1,10 @@ | |||
| package com.lyft.domic.samples.redux.rxredux.signin | |||
|
|
|||
| sealed class SignInAction { | |||
There was a problem hiding this comment.
You could move this action class in SignInStateMachine.kt file. The advantage is that you can make some Actions like ShowSigningInUi and ShowSignInSuccessfulUi that are actually just used internally for SideEffect private and not expose it to the public.
There was a problem hiding this comment.
Oh that's a nice idea
There was a problem hiding this comment.
Could get the actions private atm, but moving them to the StateMachine allowed to have better class name, so they're now just State and Action which looks much cleaner with all other advices you and Artur gave
There was a problem hiding this comment.
*Could not get
| data class ChangePassword(val password: CharSequence) : SignInAction() | ||
| object SignIn : SignInAction() | ||
| object ShowSigningInUi : SignInAction() | ||
| object ShowSignInSuccessfulUi : SignInAction() |
There was a problem hiding this comment.
Just SigningIn , SignInSuccessful, SignInFailure?
| object SignIn : SignInAction() | ||
| object ShowSigningInUi : SignInAction() | ||
| object ShowSignInSuccessfulUi : SignInAction() | ||
| data class ShowSignInFailureUi(val cause: Throwable) : SignInAction() |
There was a problem hiding this comment.
While passing around Throwable adds some value for logging and so on it also has some downsides: Usually you are not in control how the Throwable is created and hence testing via `assertEquals(expected = ShowSignInFailureUi(mockedException), actual) is hard.
| package com.lyft.domic.samples.redux.rxredux.signin | ||
|
|
||
| sealed class SignInState( | ||
| open val email: CharSequence, |
There was a problem hiding this comment.
open val email: CharSequence creates private fileds in SignInstate but you also create the same fields in subclasses like SignInState.Idle with override val email: CharSequence. I think the correct way would be:
sealed class SignInState {
abstract val email: CharSequence
abstract val password: CharSequence
abstract val signInButtonEnabled: Boolean
data class Idle(
override val email: CharSequence,
override val password: CharSequence,
override val signInButtonEnabled: Boolean
) : SignInState()
...
}
|
|
||
| inputActions.accept(SignInAction.SignIn) | ||
|
|
||
| service.signIn.accept(SignInResult.Success) |
There was a problem hiding this comment.
Nitpicking since it's not the aim of this repo to show best practices on testing but this feels super strange to me in combination with
class TestSignInService : SignInService()
...
override fun signIn(credentials: SignInService.Credentials): Observable<SignInService.SignInResult> {
signInCredentialsRelay.accept(credentials)
return signIn
}
}especially this part:
signInCredentialsRelay.accept(credentials)
return signInTheoretically, if you run your application with TestSignInService instead of RealSignInService, does this even work? I'm not sure if I like it but again, it's about domic not about testing :)
There was a problem hiding this comment.
Wait, what's wrong with it? How would you simulate an emission from the rx based business logic?
There was a problem hiding this comment.
Sorry, I just realiized that I commented on the wrong source file.
There is nothing wrong with service.signIn.accept(SignInResult.Success). Just TestSignInService implementation feels strange (not wrong, just strange 😄 )
| is SignInService.SignInResult.Error -> SignInAction.ShowSignInFailureUi(result.cause) | ||
| } | ||
| } | ||
| .startWith(SignInAction.ShowSigningInUi) |
There was a problem hiding this comment.
Micro optimization: Not sure if the startWith() is needed because Reducer could already produce the right state just by getting SignInAction.SignIn. See duplicated code in reducer:
is SignInAction.SignIn -> SignInState.SigningIn(
email = state.email,
password = state.password,
signInButtonEnabled = false
)
is SignInAction.ShowSigningInUi -> SignInState.SigningIn(
email = state.email,
password = state.password,
signInButtonEnabled = false
)There was a problem hiding this comment.
Good catch, I wrote this side-effect before the reducer and didn't notice that logic was duplicated after that
Interestingly, both distinctUntilChanged() on the reduxStore and Domic's diffing make stuff like this work without screen flickering and go unnoticed
|
Great job 👍 Some thoughts on your thoughts 😸
As I (usually) prefer integration test over unit tests, I still see some value in having functional in-memory tests. But yeah, you could just trigger actions directly, it just a matter of where (at which layer) your functional in-memory tests start. Same for "rendering state". Of course you could just test your state machine but if its easy and cheap to test rendering on UI too (which seems to be the case with domic's test), then why not include this layer too? I think its
Not sure what you mean? I mean, you do that in reducer in you code?!?
I feel your pain. I think this sample (it's essentially just doing a http request) is simple enough that it actually does not necessary need previous state to compute next state and therefore no state machine or redux store (or .scan() operator) at all. Hence it might feel a bit more boilerplate or verbose or even overkill compared to your mvvm implementation. Imho its because RxJava actually is already somehow a state machine with
Right, we weren't happy with this either and it caused me holding back open sourcing RxRedux for quite some time. We were looking for a better solution to that problem but eventually we didn't find a better way (if you know a better way I would be very happy to hear it). Eventually we decided to open source it with
🙏 |
|
I've addressed your feedback and actually like it much more now, especially due to Overall I like the integration, it definitely doesn't feel anywhere near as native as MVVM + Domic, but it's not bad at all. Especially after addressing your feedback. I must try the DSL-based approach as well though, to better understand the feel of React + Redux. A lot of people are (and were) super hyped about that exact combination for some reason, right. Thanks for review, @sockeqwe and @ming13, you know how much I appreciate that 😽 @vanniktech I'll keep it open for a day or two, it's not a functional update so there is no pressure in merging asap, you might have some comments I would assume :) |
|
Yeah wanted to wait until you address the feedback of others so we don't get too crazy here. |
| override fun render(signInState: Observable<State>): Disposable { | ||
| val state = signInState | ||
| .replayingShare() | ||
| .observeOn(Schedulers.computation()) |
There was a problem hiding this comment.
Doesn't this need to be the main thread? 4 lines below you do what looks like an operation that needs to run on the main thread.
There was a problem hiding this comment.
No, that's one of the major points of Domic, you're supposed to run your code off the main thread, Domic's rendering will take care of switching to main thread as efficient as we can
|
|
||
| override fun render(signInState: Observable<State>): Disposable { | ||
| val state = signInState | ||
| .replayingShare() |
There was a problem hiding this comment.
yeah, you need share or replayingShare to allow multiple streams down below in this method to observe same state
otherwise state Observable will run multiple times if it's Cold
| } | ||
| } | ||
|
|
||
|
|
| emailEditText | ||
| .observe | ||
| .textChanges | ||
| .map { Action.ChangeEmail(it) }, |
There was a problem hiding this comment.
emailEditText
.observe
.textChanges
.map { Action.ChangeEmail(it) },Duplicate?
| password = state.password, | ||
| signInButtonEnabled = false | ||
| ) | ||
| is Action.SigningIn -> State.SigningIn( |
There was a problem hiding this comment.
You can group is Action.SignIn and is Action.SigningIn together:
when(action) {
...
is Action.SignIn,
is Action.SigningIn -> State.SigningIn(
email = state.email,
password = state.password,
signInButtonEnabled = false
)
...
}There was a problem hiding this comment.
yeah, I was actually using multi is statement for ChangeEmail and ChangePassword, but quickly learned that Kotlin compiler doesn't narrow the type variance there and still requires me to check all the branches of is inside that block
gladly for this SignIn and SigningIn combination I don't need to look up into their fields
|
|
||
|
|
||
| val state: Observable<State> = inputActions | ||
| .observeOn(computationScheduler) |
There was a problem hiding this comment.
Is that necessary?
With that the signInButtonEnabled = false will only apply to at least the next frame. The user could tap it many times?
There was a problem hiding this comment.
that let's to move computations that don't need run on main thread from main thread thus making it more capable for handling other stuff like animations
observeOn(computationScheduler) on its own doesn't mean that rendering will happen on next frame, frame window is normally 16.6ms, computation scheduler can finish this particular code within 1ms
However, rendering in Domic (with standard renderer implementation) always happens on next frame anyway, but as efficient as we can otherwise.
Note that normally Handler.post() will give you similar behavior
This was discussed with Adam Powell (Android Framework UI lead and we agreed that one frame is nothing for non-game applications and is totally fine if stable 60 or more fps is more important)
There is also api for manual rendering asap: #19
There was a problem hiding this comment.
ok nice
observeOn(computationScheduler) on its own doesn't mean that rendering will happen on next frame, frame window is normally 16.6ms, computation scheduler can finish this particular code within 1ms
Until very recently, observeOn(AndroidSchedulers.mainThread()) would have been using Handler.post() behind the scenes so this would have been the cause for the application to the next frame.
There was a problem hiding this comment.
yep, I know, you can always use API provided in #19, if you care about rendering in current frame (which most utility apps shouldn't really care about)
|
@artem-zinnatullin I think you should just merge this to master and people will / can provide further improvement by sending a PR ... |
|
Sounds good 👍 |
PR adds sample of Domic integration with RxRedux made by @sockeqwe at Freelitics
Please note that while I seem to understand ideas of Redux and some pros/cons, it might be not very idiomatic.
General thoughts on Domic + Redux/MVI:
inputActionsproduced by View layer, however not much profit here compared to RxBindingtestmodule becomes useless unless you want to use it for functional in-memory tests for your Redux app on JVMGeneral thoughts on Redux:
copy()previous state to produce new oneChangeEmailandChangePasswordaction handling in state machineSome thoughts on RxRedux:
StateAccessorwas necessary, compared to just passing current state as parameter, I'll have to try in more complicated scenarios when something can happen in parallel to async action I guesscc @sockeqwe, code review from you would be reaaaaaly appreciated :)