etiennelenhart / Eiffel

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

Add 'Interceptions' class with Builder #105

Closed etiennelenhart closed 5 years ago

etiennelenhart commented 5 years ago

Resolves #104.

@jordond Could you maybe take a look and check if this implementation would fit your needs? I tried to match some of your proposals while also allowing easy building with testable Interceptions and integrating it with EiffelViewModel. It now allows composing the interceptions chain in three different ways.

Providing a list of Interception instances:

class TestableAdapter : Adapter<SampleState, SampleAction>() {
    override fun adapt(state: SampleState, action: SampleAction): SampleAction {
        return if (state.sample && action is SampleAction.DoSomething) {
            SampleAction.Adapted
        } else {
            action
        }
    }
}

class TestableFilter : Filter<SampleState, SampleAction>() {
    override fun predicate(state: SampleState, action: SampleAction): Boolean {
        return action !is SampleAction.IgnoreMe
    }
}

class TestablePipe : Pipe<SampleState, SampleAction>() {
    override fun after(state: SampleState, action: SampleAction?) {
        Log.d("Sample", "Chain result: $action")
    }
}

fun interceptions() = Interceptions(TestableAdapter(), TestableFilter(), TestablePipe())

Using the Interceptions.Builder class:

fun interceptions() = Interceptions.Builder<SampleState, SampleAction>()
    .add { TestableInterception() }
    .adapter { state, action ->
        if (state.sample && action is SampleAction.DoSomething) {
            SampleAction.Adapted
        } else {
            action
        }
    }
    .filter { _, action -> action !is SampleAction.IgnoreMe }
    .pipe { _, action -> Log.d("Sample", "Chain result: $action") }
    .build()

Using the DSL-like wrapper for the builder class:

fun interceptions() = interceptions<SampleState, SampleAction> {
    add { TestableInterception() }
    adapter { state, action ->
        if (state.sample && action is SampleAction.DoSomething) {
            SampleAction.Adapted
        } else {
            action
        }
    }
    filter { _, action -> action !is SampleAction.IgnoreMe }
    pipe { _, action -> Log.d("Sample", "Chain result: $action") }
}

Interceptions implements the List<E> interface by delegating to its chain so EiffelViewModel can still use it just as before.

I omitted your "on" style statements for now since I think it would make more sense to provide a full DSL for declaratively creating interceptions in the future. Something like this would be nice:

interceptions<SampleState, SampleAction> {
    adapter {
        on(SampleAction.DoSomething) {
            adaptTo { SampleAction.Adapted }
            whenState { sample == true }
        }
    }
    filter { 
        block { SampleAction.IgnoreMe }
    }
    pipe { 
        after { Log.d("Sample", "Chain result: $action") }
    }
}

But that will probably require a lot more work and consideration.

jordond commented 5 years ago

For the most part it looks good to me, I just had one question about your decision to move the Interception convenience functions (adapter, filter, etc) into the builder, instead of just invoking them. Is there no use-case for using the standalone convenience functions?

As for your note on the on variations. 99% of the Interception's that I use are the on variants.

Because I liked the looks of:

buildInterceptions<PasswordResetState, PasswordResetAction> {
    named("Block duplicate requests if one is 'Incomplete'")
    addFilterOn<ResendResetPassword> { state, _ ->
        state.resetEmailStatus !is Incomplete
    }

    named("Adapt 'Resend' to 'Reset' password")
    addAdapterOn<ResendResetPassword> { state, _ ->
        ResetPassword(state.email, true)
    }

    named("Block duplicate resets")
    addFilterOn<ResetPassword> { state, action ->
        !state.hasInitialized || action.isResend
    }

    named("Call server to reset password")
    addLiveCommandOn<ResetPassword> { _, action ->
        resetPasswordUseCase(this, action.email) { PasswordResetAction.UpdateResetStatus(it) }
    }
}

Over:

buildInterceptions<PasswordResetState, PasswordResetAction> {
    named("Block duplicate requests if one is 'Incomplete'")
    addFilter { state, action ->
        if (action is ResendResetPassword) state.resetEmailStatus !is Incomplete
        else true
    }

    named("Adapt 'Resend' to 'Reset' password")
    addAdapter { state, action ->
        if (action is ResendResetPassword) ResetPassword(state.email, true)
        else action
    }

    named("Block duplicate resets")
    addFilter { state, action ->
        if (action is ResetPassword) !state.hasInitialized || action.isResend
        else true
    }

    named("Call server to reset password")
    addLiveCommand { action ->
        when (action) {
            is ResetPassword -> forwarding {
                resetPasswordUseCase(this, action.email) {result -> 
                    PasswordResetAction.UpdateResetStatus(result) 
                }
            }
            else -> ignoring()
        }
    }
}

It makes it less verbose, which I think pairs well with the DSL mentality. (Ignore the named thats just an experimental feature I added to add a debugname to the interception, it looks nice in the logcat, but I'm not sure how much I like the implementation)

I was also toying with the idea of having something like:

buildInterceptions<PasswordResetState, PasswordResetAction> {
    on<ResendResetPassword> {
        named("Block duplicate requests if one is 'Incomplete'")
        addFilter { state, _ -> state.resetEmailStatus !is Incomplete }

        named("Adapt 'Resend' to 'Reset' password")
        addAdapter { state, _ -> ResetPassword(state.email, true) }
    }

    on<ResetPassword> {
        named("Block duplicate resets")
        addFilterOn<ResetPassword> { state, action -> !state.hasInitialized || action.isResend }

        named("Call server to reset password")
        addLiveCommandOn<ResetPassword> { _, action ->
            resetPasswordUseCase(this, action.email) { UpdateResetStatus(it) }
        }
    }
}

As I frequently seem to have multiple Interceptions that target a specific Action.

So with my current use-case, I can get by with most of your builder, but the targeted Action's.

etiennelenhart commented 5 years ago

You make some very good points. 🙂 I'll think about it and will make some refinements.

etiennelenhart commented 5 years ago

@jordond I made some adjustments.

I changed all interceptions to final classes that expect constructor parameters instead of overriding functions. I think that'll make the library easier to understand and work with:

val sampleAdapter = Adapter<SampleState, SampleAction> { state, action ->
    if (state.that && action is SampleAction.DoSomething) SampleAction.DoThat else action
}

I also added Interceptions.Builder.Targeted for interceptions targeting a specific action which powers your proposed on statement:

interceptions<SampleState, SampleAction> {
    pipe { _, action -> Log.d("Sample", "Chain result: $action") }
    on<SampleAction.DoSomething> {
        adapter { state, action -> if (state.that) SampleAction.DoThat else action }
        filter { state, _ -> !state.doingSomething }
    }
}

The debugName can still be passed as an optional first parameter:

interceptions<SampleState, SampleAction> {
    pipe("Log chain result") { _, action -> Log.d("Sample", "Chain result: $action") }
    ...
}

I also toyed with some alternatives like your named style or putting it into a builder for each interception but I think that just makes it more complicated.

Let me know what you think.  🙂

jordond commented 5 years ago

Nice! It's looking great, I love the Interceptions.Builder.Targeted approach.