freeletics / RxRedux

Redux implementation based on RxJava
https://freeletics.engineering/2018/08/16/rxredux.html
Apache License 2.0
566 stars 34 forks source link

RxRedux + Coordinator pattern opinions and questions #24

Open jhowens89 opened 5 years ago

jhowens89 commented 5 years ago

Hello @sockeqwe ! Another year and another time I find myself on github thanking you for such a great library (and the rest of the freeletics team!) and hoping to seek your advice / pick your brain. I was refreshing myself on your coordinator posts when I skimmed over this guy; I couldn't help but think how much cleaner (IMO) that implementation looks with the introduction of the RxRedux library. That's not to say the original was ugly, but to say how great of an idea I think it was to elevate side effects to the status of a first class citizen. Kinda one of those things where you don't realize how much you needed it until you have it :-)

I've luckily got a great gig where I have the opportunity of creating a new app from scratch. I've spent a good amount of time learning about the coordinator pattern and gradually fine tuning my approach. As you know, literature on this for Android is scarce so I've had the opportunity to grow my previously very limited iOS knowledge and learn from their community. I eventually settled on an approach very similar to this: https://medium.com/blacklane-engineering/coordinators-essential-tutorial-part-i-376c836e9ba7. After rereading your posts, I realize your approach is very similar so I apologize for being such a copycat -- this time it wasn't on purpose lol.

Anyways, I'll get down to the nuts and bolts. To me, being able to pull up navigation into the business logic layer is huge. If someone asked me to give a fair argument for and against MVI, for all the positive things I would say, relaying these one time events or command-ish things through the view/fragment layer is a bit of a hairy situation. Sure at this point I have been working with MVI 1.5 years in production and I know a lot of the tricks, but number one on that list is finding a way to cleverly rework the problem where I'm not doing that. Asking someone newer to the pattern to have the foresight to do the same is asking a lot. So again, not having to worry about that on every screen is the bees knees.

But...with this new found superpower comes concern. Now that I no longer have to venture through my live data state holder, my mosby behavior relay, etc -- in your opinion, what do you think is the best way to I make sure that I'm still respecting the lifecycle when I do my fragment transactions? In your example, you simply grant the activity reference in onCreate and nullify it onDestroy and I'm not sure there's an obvious upgrade to that. Even in Zhuinden's simple-stack's samples (who I would probably consider about as much of a lifecycle expert as anyone), he does something extremely similar with a BackStackHolder class. Could the screen not be rotated during this process (where in the best case I drop the event instead of crash) and would this not force me to maintain the paradigm that sideeffects that trigger navigation never leave the UI thread? Have you toyed with anything similar to how we enqueue states with LiveData but with navigation events? By the way, my concern is only with regards to a single activity architecture.

Bonus question: In your article you mention nullifying the reference to your lambda/coordinator in your ViewModel::onCleared. I've been doing the same because I was emulating the iOS implementations. I started questioning whether it was necessary. Can you describe the resulting memory leak?

Also happy to discuss my experience with the RxRedux and Coordinator combination rather than drill you with questions :-). It was a challenge to implement due the android lifecycle and challenging iOSish requirements from design but IMO it's turning out very sleek. Coordinators not only do navigation for me but while they navigate they create and subscope existing dagger components so that I have great DI graph that I had a lot of trouble cleanly architecting without the Coordinator pattern.

Edit: After reading this, I'm starting to wonder/suspect that my original question and my bonus question are related...

lgtout commented 5 years ago

@jhowens89 I'm very interested in learning more about this - "Coordinators not only do navigation for me but while they navigate they create and subscope existing dagger components". How did you achieve this?

jhowens89 commented 5 years ago

Sure @lgtout! Would you first mind describing your level of familiarity with Dagger terminology (like subcomponents, scopes other than singleton) so that I can avoid under or over explaining the current setup?

sockeqwe commented 5 years ago

Thanks @jhowens89 for your kind words and to bring this up interesting discussion.

I'm sorry guys that I can't contribute much to this discussion right now because Im super busy to finish stuff before Christmas. I hope to find some time over the holidays to join this discussion.

lgtout commented 5 years ago

@jhowens89 - I'm pretty familiar with Dagger concepts. If you introduce anything that's new to me, I can research that separately. Thanks!

Zhuinden commented 5 years ago

Even in Zhuinden's simple-stack's samples (who I would probably consider about as much of a lifecycle expert as anyone), he does something extremely similar with a BackStackHolder class.

@jhowens89 Two important tidbits:

1.) in retrospect I made a mistake with the BackstackHolder, because it can only be @Singleton global if the single-activity launchMode is singleTask. I'm not sure how I managed to forget that. :disappointed: on the bright side I am using singleTask in production and it seems to work well for my purposes :smile:

2.) Backstack cannot leak because the BackstackDelegate removes the state changer ref in onPause() and sets it back in onResume() without triggering a state change. However, Fragments are still a bitch tricky in regards that if you mash HOME button + a fragment transaction, you can get the "async transaction" to run just after HOME made app go beyond onStop, so fragmentTransaction.commit() is called while there is a StateChanger, when it actually executes the app has already done onSaveInstanceState(). Clearly the person who designed transaction.commit() didn't think this through, so I use commitAllowingStateLoss() as this is a very rare scenario (you actively have to work for it).

Coordinators not only do navigation for me but while they navigate they create and subscope existing dagger components so that I have great DI graph that I had a lot of trouble cleanly architecting without the Coordinator pattern.

Cool!