search

etiennelenhart / Eiffel

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

Change 'Filter' and 'Interception' to allow preventing Actions from reaching 'Update' #66

Closed jordond closed 5 years ago

jordond commented 5 years ago

I'm just looking for a little clarification on the Filter interception.

Based on the naming, and wording of the code documentation. I was under the assumption that if the predicate returned true it would block that action from being received by the updater thus not modifying the state. (I was wrong)

But it only seems to block the other interceptions from working with it. I was wondering if you could provide a sample use-case for that. I'm not sure if it's just my Monday brain or not, but I can't think of a use-case for it.

(I understand the README isn't up-to-date, as v5 is unreleased, so it can wait if you're busy)

etiennelenhart commented 5 years ago

Intended behavior is that Filter prevents any Actions for which the predicate returns false from passing through the interceptor chain, basically to prevent execution of any side effects.

A simple example would be a command that is triggered by a button press. Touching the button dispatches some kind of action, let's say SampleAction.GetSomething and a Command interception triggers a network request and then adapts it to SampleAction.LoadingSomething that reaches the updater. A new State is emitted that correctly represents the loading state. Now if the button is pressed again during this process, since it couldn't be disabled fast enough in reaction to the "loading" state, the action would be queued, processed and the network request would be triggered again. A Filter interception can check whether the current state is already "loading", prevent the action from reaching the Command and simply return SampleAction.GetSomething. Since it's a "side effect trigger action" it would fall into the else case of the updater and therefore won't update the state.

I know that this is probably not the best behavior, since it is hard to understand and a simple Filter that should just block some actions won't work as expected.

I'm planning to revisit the implementation when I've got more time and prevent the action from actually reaching the updater. The only simple solution I could think of was to allow interceptions to return a null action, indicating that it shouldn't be passed to the updater but I'm not sure whether that's a good idea.

jordond commented 5 years ago

Ah okay, now it makes much more sense. Thanks for taking the time to explain it!

I definitely like the idea of being able to filter or block an action from reaching the updater. Any particular reason why you don't like returning null?

I can take a stab at trying out a new Interception maybe something like Block. I'll see if I can think of another solution other than returning null if you're really opposed to it.

etiennelenhart commented 5 years ago

If you want to take a stab at it maybe just modify Filter so that it blocks actions from reaching the updater since that is really what I had in mind when creating it. I'm not generally opposed to returning null, it's just considered bad practice to conditionally switch the command flow depending on a null value as far as I know.

I would be okay with it though, if Interception allows null actions and the specialized versions like Pipe, Adapter, Command, etc. override invoke to only allow non-nullable actions, if that's possible. Otherwise it would be weird if someone adds like a Command that returns null for its immediateAction. It would work but is definitely not intended.

I'm a bit busy at the moment but I really appreciate your contributions. I'll take a look at your PRs as soon as possible. And if you'd ever like to join as a collaborator, just let me know. :)