Add CompositeStore

[alex2069]

Used in modularization PoC at https://github.com/Rakuten-MTSD-PAIS/mavenir-android-client-snapshot/pull/3059

• Allows a module to have its own store, state, and middleware.
• In turn allows other modules (which also have their own store, state, and middleware) to create a "composite" store of two or more stores from other modules (including composites of composites).
• The composite store implements and behaves like a normal Store
  • In the PoC above, this means that the rest of the code does not need to change because it still acts and behaves as a normal Store (only the declaration of the stores changes)

The behavior of the composite store ensures that all related stores are appropriately updated etc no matter which module triggers a store state change. The tests show the full relations/behavior between stores, but in summary;

• Any Effect dispatched to ModuleStore will trigger any listeners on their ModuleStore or any CompositeStore that includes ModuleStore
• And vice versa - an Effect dispatched to CompositeStore will dispatch to its included stores
• Any Action dispatched to the ModuleStore will trigger CompositeStore subscribers
• And vice versa - an Action dispatched to the CompositeStore is dispatched to all included stores (and triggers subscribers in other composite stores appropriately as states change etc.)

Because each module has its own store and state type, it can also use thunk middleware internally (not possible with child stores). So in the PoC above it allows having a "navigation" module that handles the router state. Its store can declare and use its own middleware to manipulate the navigation state without exposing its middleware or other internals to the rest of the app.

[codecov-io]

Codecov Report

Merging #3 (5553bed) into master (40bf350) will decrease coverage by 3.84%.
The diff coverage is 60.20%.

@@             Coverage Diff              @@
##             master       #3      +/-   ##
============================================
- Coverage     76.42%   72.57%   -3.85%     
- Complexity       49       63      +14     
============================================
  Files             9       11       +2     
  Lines           263      361      +98     
  Branches         37       47      +10     
============================================
+ Hits            201      262      +61     
- Misses           47       77      +30     
- Partials         15       22       +7     

Impacted Files	Coverage Δ	Complexity Δ
rekotlin/src/main/kotlin/org/rekotlin/Compose.kt	14.28% <14.28%> (ø)	0.00 <0.00> (?)
...lin/src/main/kotlin/org/rekotlin/CompositeStore.kt	78.57% <78.57%> (ø)	14.00 <14.00> (?)
rekotlin/src/main/kotlin/org/rekotlin/Redux.kt	90.00% <0.00%> (+10.00%)	0.00% <0.00%> (ø%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40bf350...5553bed. Read the comment docs.

[NemoOudeis]

I started review and so far this looks very promising. I'm not quite done yet, but I wanna share my thoughts so far, to get a conversation started:

Tests

Chewing through the tests I made a couple of adjustments, see alex2069#1

• some of the tests test multiples things and sequences of things. Let's try to keep tests smaller. (see the PR)
• Test names are inconsistent with the rest of the project. the naming convention so far is should <expectation> on <action> or should <expectation> when <action> [given <context>] (see the PR)

API surface & Documentation

I'd like to keep the exposed classes, objects, enums, functions and constants to a minimum. That'll allow us to freely change anything that's not public within a major version (except for behavior of course).

• Can we make CompositeStore internal?
• Can we inline Compose, Compose1, ..., Compose9?
• Compose.kt has 0 doc comments. Let's write usable KDoc comments on exposed APIs.

Design

On the middleware topic: what middlewares do we tyically use? Thunk and...?

I've come to the view that we should avoid middlewares altogether. Thunk is the only valid use case for middlewares that I'm aware off (outside of testing) and that one is actually not so valid after all. The store should not be the engine for asynchronous behavior, coroutines (or Rx or whatever) should be. And in reality they are! Thunks typically need a coroutine scope to do anything meaningful.

class SomeAsyncLogicInAThunk(
    private val scope: CoroutineScope,
    private val repository: SomeRepository,
    private val someId: UUID
) : () -> Thunk<AppState> {
    override fun invoke() = thunk<AppState> { dispatch, _ ->
        dispatch(StartLoadingAction)

        scope.launch { 
            try {
                val something = repository.loadSomethingById(someId)
                dispatch(SomethingLoadedAction(something))
            } catch (e: RepositoryException) {
                dispatch(FaledToLoadSomethingAction)
            }
        }
    }
}

// callsite

val someAsyncLogicInAThunk = // get via DI
store.dispatch(someAsyncLogicInAThunk)

Instead we should just program async behavior in some logic object, let's say a useCase (or whatever works in your client architecture) and only use Redux to initiate state changes

class UseCase(
    private val scope: CoroutineScope,
    private val repository: SomeRepository,
    private val dispatch: DispatchFunction
) {
    fun someAsyncLogic(someId: UUID) {
        dispatch(StartLoadingAction)

        scope.launch { 
            try {
                val something = repository.loadSomethingById(someId)
                dispatch(SomethingLoadedAction(something))
            } catch (e: RepositoryException) {
                dispatch(FaledToLoadSomethingAction)
            }
        }
    }
}

// callsite

val useCase = // get via DI
useCase.someAsyncLogic

I believe Redux has nothing to do with running business logic. All it does is manage application state. If that holds then we should get rid of middlewares. But that's a larger discussion and we don't need to resolve that for this PR.

However: if we only use the thunk middleware, we could just use it in all stores and don't need to add it to composite store, right? Because the composite store always delegates, so even if only one of the stores (say store1) has a thunk middleware, shouldn't that trigger the thunk, dispatch to store1, which in turn propagates to all stores within the composite store?

Also: If you have a valid usecase for middlewares - please let me know!

[alex2069]

So quick disclaimer to save a little face lol - I kind of forgot to finish cleaning this up as this was never originally intended to go into this (public) repo. Originally the plan was to write it up in this project (for easier testing/development) and just put it in the LinkApp repo.
So on that, the tests and design/implementation of it all was never really designed on a "pure public consumption" kind of level (hence the complete lack of documentation and inconsistencies with other tests). In the end though, since the Subscription constructor is internal it made that impossible without a lot of hacks and I completely forgot to update/change it all.

Anyway, all that aside, your changes to the test are fine with me (and TBH can be further refined if needed - again it was meant to be added to another repo so I tried not to reuse a lot of the pre-existing test architecture).

API surface & Documentation

Can we make CompositeStore internal?

Done

Can we inline Compose, Compose1, ..., Compose9?

We could - TBH I'm more inclined to just make them internal as it makes it easier to read the functions that use them (but if you really want I'm not against it either). Made internal for now

Compose.kt has 0 doc comments. Let's write usable KDoc comments on exposed APIs.

I've held off on documentation until it was confirmed we'll use this, so once the API is confirmed then I'll add some for sure.

Middleware

As for the middleware; I can't speak to the design aspect with authority as my experience is limited to only personal small web projects outside of LinkApp, but it's used in a couple of places at the moment which kind of make sense as middleware.

Navigation

It's hard to judge whether there are better ways to do it as I didn't write the initial implementation (Majdi would know), but the navigation architecture (the router) uses it for push/pop/history/etc stuff;

• Added here: https://github.com/Rakuten-MTSD-PAIS/mavenir-android-client-snapshot/blob/22b02bef4122fbff6137082a6baeb2025fd091b8/common/navigation/src/main/java/jp/co/rakuten/oneapp/common/navigation/NavigationModule.kt#L37
• Declared here: https://github.com/Rakuten-MTSD-PAIS/mavenir-android-client-snapshot/blob/0bf417f6082537d3bc65dc73c19ed6788a7f06d9/common/navigation/src/main/java/jp/co/rakuten/oneapp/features/common/NavigationRedux.kt#L33

RewardSDK

The RewardSDK declares a MissionCode effect (which is implemented as an enum). Other modules dispatch those "mission complete" effects when certain conditions are met. A lot of the time those conditions already exist in the code as some sort of update or click Action. The middleware intercepts those and dispatches an Effect that the RewardSDK listens to and then goes about doing its thing etc.
e.g. Discovery reward missions: https://github.com/Rakuten-MTSD-PAIS/mavenir-android-client-snapshot/blob/22b02bef4122fbff6137082a6baeb2025fd091b8/features/src/main/java/jp/co/rakuten/oneapp/features/discovery/Reward.kt#L12

A nice aspect of this - if we want to remove the RewardSDK, you just remove the middleware and that's it - so in that regard it is good for encapsulating the logic.

Analytics

Similarly to the RewardSDK, a lot of actions are also the analytics triggers. These are dispatch to Rakuten Analytics and/or Adjust Analytics etc.

Single thunk

However: if we only use the thunk middleware, we could just use it in all stores and don't need to add it to composite store, right? Because the composite store always delegates, so even if only one of the stores (say store1) has a thunk middleware, shouldn't that trigger the thunk, dispatch to store1, which in turn propagates to all stores within the composite store?

As to the single thunk - that's actually the main reason this CompositeStore came into being. Since ReKotlin doesn't really leverage much typing when dispatching, it means in the end a typed thunk can be sent to the wrong store. When you call getState, depending on which store did the dispatching will determine what state object you get back (and of course crash with a cast exception when wrong). I ran into this problem in the initial stages of design (and when trying to use the child store system, along with other problems though).

[NemoOudeis]

a couple of disconnected thoughts:

about middlewares:

• the rewards scenario is a really good one, a middleware is a perfect fit for that indeed. Navigation
• the navigation scenario: I have vague memories of that - that's in the thunk bucket imo. It works but we could just build that into the navigation lib and make away with those middlewares. That's less flexible of course, so maybe it's a little more legitimate than thunks.
• analytics: I'm more inclined to move analytics to a usecase layer and and not involve redux at all.

but all in all there are a few valid reasons to keep middlewares so.... let's keep middlewares (I guess) 🤷‍♀️

Regarding the thunks: can you create a unit test for parent/child store that fails with the type cast exception?

I think once we merge this we can discard parent/child store and publish a new major version.

another visibility concern: the Array<out Store<*>>.s1-1() are implementation details, like Compose1-9, (but less critical because it's over arrays of a class that we declare). Ideally that should be private

I'll revisit the code sometime later this week to get some time to think about the implications of this design.

[alex2069]

Ok a few updates;

• Updated the test names to follow project convention
• Removed some other public functions from old testing implementations
• Added thunk ClassCastException tests
• Reduced Array<out Store<*>>.s1-1() visibility

another visibility concern: the Array<out Store<*>>.s1-1() are implementation details, like Compose1-9, (but less critical because it's over arrays of a class that we declare). Ideally that should be private

Ah yes, not really ideal on public API heh. I've reduced their visibility using @PublishedApi internal (so they can be inlined but not called outside of library), and made them inline so they can be added/removed without needing to worry about backwards compatibility. I can also just replace them in all the composeStores functions with their code if you'd prefer. Again it was just something meant to make my life easier when it was going to be in the LinkApp repo, so can appreciate if you'd rather them completely removed at this point.