-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a trigger parameter to delay feature start. #89
Comments
We were having two different problems for this issue:
This one is easier to explain: We were unit testing the parts of the feuater: actor, reducer, etc. But then we saw a PR where we were just changing the initial state. And there was no need to change any tests. That was a code smell. We weren't testing that. We thought to test the feature as a blackbox. The actor and reducer are just an "implementation detail". We want to check that if we send the wish The problem is that we can't test correctly the initial state because the bootstraper runs as soon as we create the feature so we never receive that initial state. I know that there are ways to avoid this. I'm just showing why this is a problem.
If we have this feature: class Asdf : BaseFeature<Unit, Unit, Unit, Unit, Unit>(
initialState = Unit,
bootstrapper = { Observable.just(Unit) },
wishToAction = {},
actor = { _, _ ->
Observable.never<Unit>()
.doOnSubscribe { Log.d("MVICore", "doOnSubscribe") }
.doOnDispose { Log.d("MVICore", "doOnDispose") }
},
reducer = { _, _ -> Unit }) We know that as soon as I run In our code, we have a much complex feature and just for creating it we were leaking all the Activity. This was because, some times, we weren't subscribing to the Feature se we thought it was pointless to dispose it. My 2 cents: The feature should not launch any internal subscription when created. The creation of an object should not have any side effect. So, instead of adding the |
This again seems to stem from a confusion about what a |
Ok, I agree that probably the subscription should not be the started of the feature (or at least not the only one). But I'm still don't get why to start the feature as soon as it's created. With the @ShikaSD's proposal I could archive this but I'm not sure if start the subscription in the creation is the best "default behaviour". (I'm not sure if this seems kind of rude. It's not my intention. I just want to share another point of view) |
I would argue that we don't need to add support from the library to achieve any of this (following our internal discussions). // 1. Pass source to feature, delay in bootstrapper manually
class ExampleFeature(
val startSignal: ObservableSource<Unit>,
): BaseFeature(...) {
class BootstrapperImpl(val startSignal: ObservableSource<Unit>): Bootstrapper<Action> {
override fun invoke() =
Observable.wrap(startSignal)
.firstElement()
.flatMapObservable { /* do your thing */ }
}
}
// 2. Use wish to init subscriptions
class ExampleFeature: BaseFeature(...) {
sealed class Wish {
object Init: Wish()
}
class ActorImpl: Actor<State, Wish, Effect> {
override fun invoke(state: State, wish: Wish) =
when (wish) {
is Init -> /* do your thing */
}
}
}
Answering @BraisGabin question:
It is started because it is supposed to be a hot observable. Imagine it like glorified |
Current implementation of the feature launches internal subscriptions when created. Although it is conceptually correct, the limitations of the Android framework sometimes force us to wait until
onCreate
to have any meaningful data.This proposal suggests to add an optional
startWhen
parameter to the feature constructor, so its start can be done after creation.Suggested update to the feature constructor:
The text was updated successfully, but these errors were encountered: