feat: split event listeners #26
Conversation
| @@ -52,8 +60,9 @@ class DefaultUnleash( | |||
| private val unleashConfig: UnleashConfig, | |||
| unleashContext: UnleashContext = UnleashContext(), | |||
| cacheImpl: ToggleCache = InMemoryToggleCache(), | |||
| eventListeners: List<UnleashEventListener> = emptyList(), | |||
| private val lifecycle: Lifecycle = getLifecycle(androidContext) | |||
| eventListeners: List<UnleashListener> = emptyList(), | |||
There was a problem hiding this comment.
Add the ability of sending multiple event listeners instead of just one
There was a problem hiding this comment.
Sorry, Both List<UnleashEventListener> and List<UnleashListener>, allow you to send more than one though? They accept a list of listeners. So the real change here is a rename of the Interface?
There was a problem hiding this comment.
Yes, previously it was just an event listener but the change from 1 to many was done in a previous PR, this comment is stale :D currently, only changing the interface
| private val fetcher: UnleashFetcher? | ||
| private val networkStatusHelper = NetworkStatusHelper(androidContext) | ||
| private val impressionEventsFlow = MutableSharedFlow<ImpressionEvent>( | ||
| replay = 1, | ||
| extraBufferCapacity = 1000, |
There was a problem hiding this comment.
Maybe this is too much, or maybe not. This is just to make sure we don't drop impression events, but we also protect the memory.
Replay turns out to be useful for predictability, because sometimes the listeners don't get to register before the first event happens (same as with isReady flow). Arguably, maybe a replay is not needed for impression events
| private val impressionEventsFlow = MutableSharedFlow<ImpressionEvent>( | ||
| replay = 1, | ||
| extraBufferCapacity = 1000, | ||
| onBufferOverflow = BufferOverflow.DROP_OLDEST |
There was a problem hiding this comment.
You may miss some updates if you're not fast enough
| eventListeners: List<UnleashListener> = emptyList(), | ||
| bootstrap: List<Toggle> = emptyList() |
There was a problem hiding this comment.
I decided to send these as part of the start method. In other proxy SDKs they're part of the configuration, but I feel there's no point in keeping the toggles in memory (as config live span is tied to the application life span). Eventually, when toggles are fetched, we no longer need to reference the original bootstrapped toggles and having them here instead than in the config object means that the GC will collect them
| companion object { | ||
| private const val TAG = "ObservableCache" | ||
| } | ||
|
|
||
| private var events = MutableSharedFlow<UnleashState>() | ||
| private var newStateEventFlow = MutableSharedFlow<UnleashState>( | ||
| replay = 1, |
There was a problem hiding this comment.
This makes the cache more predictable. During testing I noticed that the coroutine that was adding a listener to the cache: https://github.com/Unleash/UnleashAndroidSdk/blob/3da55190599b660be58797075410dcc6f69e017b/unleashandroidsdk/src/main/java/io/getunleash/android/DefaultUnleash.kt#L246-L251
was executing after the first new state arrived (this is particularly true when using bootstraped toggles as they are written immediately to the cache). This caused the ready listener to miss the ready event.
With replay = 1 in place, even in this situation, the listener that arrives later will be able to see the first UnleashState received, despite the execution order.
Particularly for state, I believe having the latest always available even for new subscribers is important.
We are also not having a queue of states, just keep the latest and drop the oldest as soon as new states arrive
gastonfournier
force-pushed
the
split-events
branch
from
4d2a475 to
b037910
Compare
gastonfournier
force-pushed
the
split-events
branch
from
b037910 to
5a374db
Compare
This allows us to create a coroutine only when there's an action defined. Previously, we were using an empty implementation but the coroutine was still executing on every event.