search

pedroql / mvflow

Simple Android MVI architecture using Kotlin Flows
MIT License
124 stars 10 forks source link

Consider moving from SharedFlow to Channel #32

Closed jordond closed 2 years ago

jordond commented 3 years ago

MVFlow uses a SharedFlow to handle the Effects, then launchWhenX {} in the View to collect the effects.

However this raises some good points about not using SharedFlow and instead use a Channel(Channel.BUFFERED). Because some Effects can be lost due to configuration changes. And even using launchWhen(Started/Resumed) {} is not immune to this, and he proposes a simple observer class to handle it.

pedroql commented 3 years ago

Hi Jordan,

Thanks for raising this.

At first I thought this didn't happen because I have tested it in the sample app and also because I even wrote unit tests for this (I was wrong in the second part - maybe I write similar tests for the state but not effects).

The library works fine with regards to surviving a configuration change. In other words, if you start some long lasting operation while the phone is in portrait, then the user changes to landscape, and eventually the operation emits an effect, provided you handle configuration changes properly, you will get this effect. You can play with this in the sample app of this library.

But based on your description/by reading the article you linked, there is one case where this is problematic which is if the effect is emitted while there are no subscribers. It doesn't seem to work in this situation (the effect is dropped). I quickly changed the code to use a BroadcastChannel (the original way this was implemented in the library) and also a Channel as you described but the problem is still there.

I wrote a unit test that exhibits the problem. But it fails in all 3 modes I have tried. Sadly I don't have enough time to look at this (my priority is to move away from jcenter/bintray and manage my fulltime job).

If you are keen to help or just investigate, you can have a go at making this unit test pass: https://github.com/pedroql/mvflow/tree/fix_effects_dropped

Thanks