diff --git a/documentation/whatsnew.md b/documentation/whatsnew.md index 829866a5..20237362 100644 --- a/documentation/whatsnew.md +++ b/documentation/whatsnew.md @@ -2,6 +2,24 @@ ### Pending changes +No pending changes + +### 1.3.1 + +#### Bug fixes + +([#138](https://github.com/badoo/MVICore/issues/167)): +Fixed regression related to BaseFeature actor. + +The Actor subject was made serializable, and was also using a flatMap. Both of these changes caused a change in behaviour relating to the ordering of news (in features that have a PostProcessor which triggers extra actions). +This change was made as part of introducing the optional `FeatureScheduler` to `BaseFeature`. + +If you provide a `FeatureScheduler` and use a PostProcessor, please be aware that the ordering of your news could change. + +The previous news ordering behaviour is actually a bug in BaseFeature caused by recursion, and will hopefully be addressed (as an opt in change) in a future release. + +### 1.3.0 + #### Additions ([#147](https://github.com/badoo/MVICore/pull/147)): diff --git a/mvicore/src/main/java/com/badoo/mvicore/feature/BaseFeature.kt b/mvicore/src/main/java/com/badoo/mvicore/feature/BaseFeature.kt index 9ffb3fbe..cf793992 100644 --- a/mvicore/src/main/java/com/badoo/mvicore/feature/BaseFeature.kt +++ b/mvicore/src/main/java/com/badoo/mvicore/feature/BaseFeature.kt @@ -47,7 +47,7 @@ open class BaseFeature { private val threadVerifier = if (featureScheduler == null) SameThreadVerifier(javaClass) else null - private val actionSubject = PublishSubject.create().toSerialized() + private val actionSubject = PublishSubject.create().toSerialized(featureScheduler) private val stateSubject = BehaviorSubject.createDefault(initialState) private val newsSubject = PublishSubject.create() private val disposables = CompositeDisposable() @@ -287,14 +287,25 @@ open class BaseFeature Observable.observeOnFeatureScheduler( scheduler: FeatureScheduler? ): Observable = - flatMap { value -> - val upstream = Observable.just(value) - if (scheduler == null || scheduler.isOnFeatureThread) { - upstream - } else { - upstream - .subscribeOn(scheduler.scheduler) + if (scheduler != null) { + flatMap { value -> + val upstream = Observable.just(value) + if (scheduler.isOnFeatureThread) { + upstream + } else { + upstream + .subscribeOn(scheduler.scheduler) + } } + } else { + this + } + + private fun Subject.toSerialized(scheduler: FeatureScheduler?) = + if (scheduler != null) { + toSerialized() + } else { + this } } } diff --git a/mvicore/src/test/java/com/badoo/mvicore/feature/BaseFeaturePostProcessorTest.kt b/mvicore/src/test/java/com/badoo/mvicore/feature/BaseFeaturePostProcessorTest.kt new file mode 100644 index 00000000..d37c3a4d --- /dev/null +++ b/mvicore/src/test/java/com/badoo/mvicore/feature/BaseFeaturePostProcessorTest.kt @@ -0,0 +1,90 @@ +package com.badoo.mvicore.feature + +import com.badoo.mvicore.element.Actor +import com.badoo.mvicore.element.NewsPublisher +import com.badoo.mvicore.element.PostProcessor +import com.badoo.mvicore.element.Reducer +import com.badoo.mvicore.feature.PostProcessorTestFeature.* +import io.reactivex.Observable +import org.junit.Test + +class BaseFeaturePostProcessorTest { + @Test + fun `GIVEN feature scheduler provided AND InitialTrigger sent WHEN post processor sends PostProcessorTrigger THEN news is in wish order`() { + val feature = PostProcessorTestFeature(featureScheduler = FeatureSchedulers.TrampolineFeatureScheduler) + val newsTestObserver = Observable.wrap(feature.news).test() + feature.accept(Wish.InitialTrigger) + + newsTestObserver.assertValues(News.TriggerNews, News.PostProcessorNews) + } + + /** + * The post processor is recursively calling the actor, meaning the news is in reverse order in this scenario. + */ + @Test + fun `GIVEN feature scheduler not provided AND InitialTrigger sent WHEN post processor sends PostProcessorTrigger THEN news is in recursive order`() { + val feature = PostProcessorTestFeature(featureScheduler = null) + val newsTestObserver = Observable.wrap(feature.news).test() + feature.accept(Wish.InitialTrigger) + + newsTestObserver.assertValues(News.PostProcessorNews, News.TriggerNews) + } +} + +private class PostProcessorTestFeature(featureScheduler: FeatureScheduler?) : + BaseFeature( + actor = ActorImpl(), + initialState = State, + reducer = ReducerImpl(), + wishToAction = { it }, + newsPublisher = NewsPublisherImpl(), + postProcessor = PostProcessorImpl(), + featureScheduler = featureScheduler + ) { + + sealed class Wish { + object InitialTrigger : Wish() + object PostProcessorTrigger : Wish() + } + + sealed class Effect { + object TriggerEffect : Effect() + object PostProcessorEffect : Effect() + } + + object State + + sealed class News { + object TriggerNews : News() + object PostProcessorNews : News() + } + + class ActorImpl : Actor { + override fun invoke(state: State, wish: Wish): Observable = + when (wish) { + is Wish.InitialTrigger -> Observable.just(Effect.TriggerEffect) + is Wish.PostProcessorTrigger -> Observable.just(Effect.PostProcessorEffect) + } + } + + class ReducerImpl : Reducer { + override fun invoke(state: State, effect: Effect): State = state + } + + class NewsPublisherImpl : NewsPublisher { + override fun invoke(action: Wish, effect: Effect, state: State): News = + when (effect) { + is Effect.TriggerEffect -> News.TriggerNews + is Effect.PostProcessorEffect -> News.PostProcessorNews + } + } + + class PostProcessorImpl : PostProcessor { + override fun invoke(action: Wish, effect: Effect, state: State): Wish? = + if (action is Wish.InitialTrigger) { + Wish.PostProcessorTrigger + } else { + null + } + } +} \ No newline at end of file