Implement caching Toggle.Store and Toggle@enabled() flow API #7019
Conversation
| * - When a collector starts, the current toggle value is emitted immediately. | ||
| * - Subsequent emissions occur whenever the underlying [store] writes a new [State]. | ||
| * - The flow is cold: a listener is only registered while it is being collected. | ||
| * - When collection is cancelled or completed, the registered listener is automatically. |
There was a problem hiding this comment.
nit: I think a word is missing here 😅
| object : Listener { | ||
| override fun onToggleStored(newValue: State) { | ||
| // emit value just stored | ||
| trySend(isEnabled()) |
There was a problem hiding this comment.
Why not just newValue?
There was a problem hiding this comment.
Because newValue is the state value however isEnable() is a computed value that has to check not only the toggle state value but also targets, min version etc
|
|
||
| override fun enabled(): Flow<Boolean> = callbackFlow { | ||
| // emit current value when someone starts collecting | ||
| trySend(isEnabled()) |
There was a problem hiding this comment.
If a coroutine context isn't specified, we'll use the same one that the consumer has specified (which in practice, might be Main most of he time. Shoud we do callbackFlow {...}.flowOn(Dispatchers.IO)? In practice, if the value is cached (and it should) it might not make much of a difference, but I think it still doesn't hurt to take some load off the main thread regardless
There was a problem hiding this comment.
No strong opinions, that also has the issue that -- other than API documentation -- it's hard to make the caller aware of it, it can happen the caller is not and then it tries to refresh the UI in IO, which will crash.
That said, I can add it
There was a problem hiding this comment.
If the caller doesn't specify its own context when calling the coroutine and they use appCoroutineScope instead of lifecycleScope then yes, it will crash. That being said, I'd rather have the app crashing in that case (which will be very hard to miss during implementation and/or automated testing) than have the app silently run non-UI work in the main thread
The context we set in flowOn will only apply to that first trySend block, though, as the second one is within a callback, so if we want to also set the context there, we'd need to launch a coroutine
aitorvs
force-pushed
the
feature/aitor/CachedToggleStore
branch
from
fab9cea to
2a39c6e
Compare
3 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1198194956794324/task/1211774033967841?focus=true
Description
Steps to test this PR
Feature 1