Odonno / ReduxSimple

Simple Stupid Redux Store using Reactive Extensions
http://www.nuget.org/packages/ReduxSimple/
MIT License
144 stars 20 forks source link

Feature reducer improvements / State lenses #74

Closed ltjax closed 4 years ago

ltjax commented 4 years ago

Hello David. As I teased on twitter, we have been using a different kind of sub/feature reducer. I noticed that the sub reducers that are already in ReduxSimple really dispatch to the type of the property, which led to our homegrown reducer for "parts of the state", with a few differences.

  1. You define the sub part by a slicer and a joiner function
  2. The sub-reducer can be restricted to a base action, so that its properties can be used to direct to a dynamic part of the state
  3. We're using the Builder pattern to avoid repeating the State type

We're using the name "LensedReducer" internally, but I'm pretty sure we're not using the concept of the functional lense correctly here. Here's how you'd define such a thing:

        public static LensedReducer<State, StateElement, ElementBaseAction> BuildElementReducer()
        {
            return ReducerBuilder.ActionDependentLens<State, StateElement, ElementBaseAction>(
                (state, action) => state.Parts[action.Index],
                (state, newSilo, action) => state.With(new { Parts= state.Parts.SetItem(action.Index, newSilo) })
            );
        }

Adn we use that like this to generate a list of Ons:

            return BuildElementReducer()
                .On<ActionA>(SetAggregateSiloDenotation)
                .On<ActionB>(SetAggregateSiloCode)
                .ToList();

Here are the classes:

    public class LensedReducer<TState, TSlicedState, TBaseAction>
        where TState : class, new()
        where TBaseAction : class
    {
        private readonly Func<TState, TBaseAction, TSlicedState> slicer;
        private readonly Func<TState, TSlicedState, TBaseAction, TState> joiner;
        private readonly ImmutableList<On<TState>> reducers = ImmutableList<On<TState>>.Empty;

        public LensedReducer(Func<TState, TBaseAction, TSlicedState> slicer, Func<TState, TSlicedState, TBaseAction, TState> joiner)
            : this(slicer, joiner, ImmutableList<On<TState>>.Empty)
        {
        }

        private LensedReducer(Func<TState, TBaseAction, TSlicedState> slicer, Func<TState, TSlicedState, TBaseAction, TState> joiner, ImmutableList<On<TState>> previous)
        {
            this.slicer = slicer;
            this.joiner = joiner;
            this.reducers = previous;
        }

        public LensedReducer<TState, TSlicedState, TBaseAction> On<TAction>(Func<TSlicedState, TAction, TSlicedState> handler)
            where TAction : class, TBaseAction
        {
            TState Reduce(TState state, TAction action)
            {
                var oldSlicedState = slicer(state, action);
                var newSlicedState = handler(oldSlicedState, action);
                return joiner(state, newSlicedState, action);
            }

            var withNewReducer = reducers.Add(ReduxSimple.Reducers.On<TAction, TState>(Reduce));
            return new LensedReducer<TState, TSlicedState, TBaseAction>(slicer, joiner, withNewReducer);
        }

        public IReadOnlyList<On<TState>> ToList()
        {
            return reducers;
        }
    }

    static class ReducerBuilder
    {
        /// <summary>
        /// Create a lense that always focuses the same part of the state
        /// </summary>
        public static LensedReducer<TState, TSlicedState, object> StaticLens<TState, TSlicedState>(
            Func<TState, TSlicedState> slicer, Func<TState, TSlicedState, TState> joiner)
            where TState : class, new()
        {
            return new LensedReducer<TState, TSlicedState, object>(
                (state, _) => slicer(state),
                (state, sliced, _) => joiner(state, sliced));
        }

        /// <summary>
        /// Create a lense that can use the action to focus specific parts of the state
        /// </summary>
        public static LensedReducer<TState, TSlicedState, TBaseAction> ActionDependentLens<TState, TSlicedState, TBaseAction>(
            Func<TState, TBaseAction, TSlicedState> slicer,
            Func<TState, TSlicedState, TBaseAction, TState> joiner
            )
            where TState : class, new()
            where TBaseAction : class
        {
            return new LensedReducer<TState, TSlicedState, TBaseAction>(
                slicer, joiner);
        }
    }

What do you think?

Odonno commented 4 years ago

Hi Marius,

Thanks for the explanation. I find your Lens reducer feature interesting.

From what I understand, I see 2 distinct points :

For the first point, I can think to use it with the current sub-reducers feature. I don't think it is related to your Lenses. So why not enjoy this feature, I will look at it.

And for the second point, I want you to know that ReduxSimple is mainly inspired by @ngrx. On this part, @ngrx uses feature key (the string key of a property inside the Root state) to handle nested reducers. But even in a js/ts application using @ngrx, I am not really a fan, I would rather prefer a lambda function. And this case, I think that 1. reusing a selector is smart (no code duplication) and 2. the selectors act as lenses since we use them to access the data and we use .NET reflection to update the data.

So, I will definitely look into the first point to improve the writing of reducers. And about the lenses, if you agree or if you want to share a more concrete example, please do. It's possible that I may have missed something.

And again, thank you for your sharing.

ltjax commented 4 years ago

I'm not entirely sure what your status is here. But I see that you recently (11 days ago) changed your implementation from writing to the first property with the right type to the first property that has the same value. This does, however, also break easiely with anything that has a proper == - you just need to try to reduce to the second of two ints with the same value, and it'll break. The current sub reducer in ReduxSimple breaks in a lot of strange ways because it does not have a sensible "write-back" path. Besides the value-equality problem, it'll break when you select something that is deeper in your state, for example we use a selector like state => state.SomeList[i]. Correct me, if I'm wrong, but I believe that will just silently fail (i.e. do nothing) with the ReduxSimple CreateSubReducers.

The lens thing is conceptually a super-set of your current solution, with an explicit write-back path (I call this "join"). And you can very easiely layer a property-key based selector as a facade on top of it. If you want to use reflection, you probably need to use an Expression<> or you will only ever get enough info for the "slice" part, not the write-path/join.

Odonno commented 4 years ago

I see your point and so here are the different use cases:

So, in order to fix this, I see that we can have an ImplicitLens (current implementation) and an ExplicitLens, like you suggested.

And if I try to prototype an API that could make this work:

internal class ImplicitStateLens : IStateLens<TState, TFeatureState> {}
internal class ExplicitStateLens : IStateLens<TState, TFeatureState> {}

static IStateLens<TState, TFeatureState> CreateSubReducers(Func<TState, TFeatureState> featureSelector) {} // implicit lens
static IStateLens<TState, TFeatureState> CreateSubReducers(ISelectorWithoutProps<TState, TFeatureState?> featureSelector) {} // implicit lens
static IStateLens<TState, TFeatureState> CreateSubReducers(
    Func<TState, TFeatureState> featureSelector,
    Func<TState, TFeatureState, TState> featureReducer
) {} // explicit lens
static IStateLens<TState, TFeatureState> CreateSubReducers(
    ISelectorWithoutProps<TState, TFeatureState?> featureSelector,
    Func<TState, TFeatureState, TState> featureReducer
) {} // explicit lens

In a sense that the implicit Lens will work exactly like now but it will write a console warning, things that I should have done already...

And of course, we can still use the Builder pattern to make it even better.

public static IEnumerable<On<RootState>> GetReducers()
{
    return CreateSubReducers(SelectCounterState, ReduceCounterState)
        .On<ActionA>(ReduceOnActionA)
        .On<ActionB>(ReduceOnActionB)
        .ToList();
}

public static IEnumerable<On<RootState>> CreateReducers()
{
    return CombineReducers(
        Counter.Reducers.GetReducers(),
        TicTacToe.Reducers.GetReducers(),
        TodoList.Reducers.GetReducers(),
        Pokedex.Reducers.GetReducers()
    );
}

I'd love to see that. What do you think?

ltjax commented 4 years ago

Yea, it looks nice! The indexed thing will only work if you can also read something from the action to affect the index, otherwise you can just mirror it onto another property on the state. But then it is very useful. Either way, the "implicit" sub reducer should still be able to access more deeply nested properties like state => state.A.B I think you can still use a "real" property key / reflection to make the implicit implementation better. To do this, you must first decode an expression like state => state.A.B into a "property key" like List<string>{"A", "B"}. Then you can access the properties for reading and writing. This can be done with .NET reflection, but it is a bit slow. That shouldn't be a problem, since it only needs to be done once when setting up the reducers. I can give you the code to decode the expressions, if you want.

Odonno commented 4 years ago

I opened a draft PR if you want to read the code. I think it really stay dumb and simple.

Clearly, the explicit state lens is way better in many way. I just wanted something simple as reuse the selector and just go for it. Anyway, I will make the explicit version one the default one in the doc.

Odonno commented 4 years ago

@ltjax Like I said, I merged the PR as it is. If you want, you can open another issue/PR if you want this to go further. I will also add you in the contributions list.