etiennelenhart / Eiffel

Redux-inspired Android architecture library leveraging Architecture Components and Kotlin Coroutines
MIT License
211 stars 14 forks source link

Add a 'Interceptor' builder class #65

Closed jordond closed 5 years ago

jordond commented 5 years ago

What's new

I have added a convenience builder class which will allow you to use kotlin DSL to create a list of Interceptors.

Reasoning

Since EiffelViewModel takes a List<Interceptor<S, A>> as a constructor parameter, I often found myself creating the interceptions in a different file as top level variables.

Some made up examples (shortened for brevity):

// SampleInterceptors.kt
private val logActions = pipe<SampleState, SampleAction> { state, action ->
    Timber.d("Action: $action -> State: $state")
}

// SharedPrefs is a helper wrapper around SharedPreferences
private fun setSharedPrefsValue(prefs: SharedPrefs) {
    return pipe<SampleState, SampleAction> { state, action ->
        if (action is SampleAction.Success)
            prefs.setSomeValue = state.myAwesomeValue
    }
}

fun createInterceptors(prefs: SharedPrefs) = listOf(logActions, setSharedPrefsValue(prefs))

// SampleViewModel.kt
class SampleViewModel @Inject constructor(prefs: SharedPrefs) : EiffelViewModel<SampleState, SampleAction>(
    initialValue = SampleState(),
    interceptors = createInterceptors(prefs),
    update = update { state, _ -> state }
)

I am using Dagger2 for dependency injection, and some of my interceptors require those injected dependencies.

As you can see, for each interceptor i need to create a function or a variable, and then specify the type each time. Which can get a bit repetitive. Perhaps if EiffelViewModel.interceptions was an open val then it could be overridden in the child removing the need for specifying the types.

Now of course I could get rid of the problem of typing the State and Action types each time by putting into a listOf() call.

ex:

fun createInterceptors(prefs: SharedPrefs) = listOf<Interception<SampleState, SampleAction>>(
    filter { state, action ->  },
    pipe { state, action -> prefs.value = state.someValue }
)

Which is fine, but I didn't like all the commas. So I created a builder class in the style of kotlin DSL.

So now I can do:

Using Dependency Injection

// SampleInterceptor.kt
class SampleInterceptors @Inject constructor(private val prefs: SharedPrefs) {

    val list = buildInterceptors<SampleState, SampleAction> {

       addFilter { state, action -> // do something }

       addPipe { state, action -> prefs.value = state.someValue }

       addPipeOn<SampleAction.Error> { state -> // do something }
    }
}

// SampleViewModel.kt
class SampleViewModel @Inject constructor(
   sampleInterceptors : SampleInterceptors
) : EiffelViewModel<SampleState, SampleAction>(
    initialValue = SampleState(),
    interceptors = sampleInterceptors.list,
    update = update { state, _ -> state }
)

Manually passing in the dependencies

// SampleInterceptor.kt

fun createInterceptors(prefs: SharedPrefs) = buildInterceptors<SampleState, SampleAction> {

  addFilter { state, action -> // do something }

  addPipe { state, action -> prefs.value = state.someValue }

  addPipeOn<SampleAction.Error> { state -> // do something }
}

// SampleViewModel.kt
class SampleViewModel @Inject constructor(
   prefs : SharedPrefs
) : EiffelViewModel<SampleState, SampleAction>(
    initialValue = SampleState(),
    interceptors = createInterceptors(prefs),
    update = update { state, _ -> state }
)

I think it is a nice approach. But of course I would love your feedback, and if you don't feel it is something that is in the scope of the library. Then I can just continue to use it myself.

Notes

I tried my best to match your commenting style, and I believe everything I added is documented.

jordond commented 5 years ago

I have updated this PR to add more interception types, and support the new Reaction.Forwarding command.

I was thinking of moving the builder into its own module, so that the developer could opt-in.

If you want to see an example of this in action, check out my WIP side project here.

etiennelenhart commented 5 years ago

One thing I had in mind with interceptions-based side effects was improved testability of the "plumbing" layer that previously was entirely contained in a StateViewModel.

Now the plumbing can be split up into individual parts which may be tested separately in isolation and the whole machinery is testable via EiffelViewModel.

When using your DSL approach, the individual interceptions are anonymous objects that can't be tested separately. Whether that's a problem depends on the amount of plumbing logic an interception performs, I guess.

I haven't gotten too much into testing interceptions yet so I'd be glad to hear how you'd approach this.

Nevertheless I completely agree that the verbose typing is annoying but haven't figured out a flexible way to solve this.

———

Off topic:

In your "WhaleSay" example (great name by the way 🙂) you're using a Pipe to call audioPlayerUseCase.stop(). Normally Pipe is intended to be used for more or less unrelated actions like logging or analytics. Of course you're free to use it however you like, just note that its lambda expressions are called synchronously and therefore block the dispatchActor.

And the logic in onFabClicked() seems like a perfect candidate for an Adapter. 😉

jordond commented 5 years ago

I understand your concerns with the testing aspect. So I have created a new repo here, for it to live for now. And also the place I put any other "addons" or out-of-scope features.

So I'll close this for now, and if some point in the future we think of a different solution, we can re-open.

Thanks for the tips above, for some reason I didn't even think of using an Adapter for the click handler.