Skip to content
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 'Interceptions' class with Builder #105

Merged
merged 1 commit into from
Mar 18, 2019

Conversation

etiennelenhart
Copy link
Owner

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.

@etiennelenhart etiennelenhart added this to the 5.0.0 milestone Mar 13, 2019
@etiennelenhart etiennelenhart self-assigned this Mar 13, 2019
@jordond
Copy link
Contributor

jordond commented Mar 13, 2019

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
Copy link
Owner Author

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

@etiennelenhart etiennelenhart force-pushed the simple-interceptions-builder branch 2 times, most recently from 521a1cf to 9533a91 Compare March 18, 2019 08:05
@etiennelenhart
Copy link
Owner Author

@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
Copy link
Contributor

jordond commented Mar 18, 2019

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

@etiennelenhart etiennelenhart merged commit a8f34d0 into master Mar 18, 2019
@etiennelenhart etiennelenhart deleted the simple-interceptions-builder branch March 18, 2019 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants