Odonno / ReduxSimple

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

Create state lenses (implicit/explicit) #75

Closed Odonno closed 4 years ago

Odonno commented 4 years ago

Ey @ltjax

I started to create feature state lenses like we discussed. Here is what I am currently headed.

I still need to do some stuff before completing the PR:

So, I managed to add two versions for implicit lense (pretty much like what we have now) and explicit lenses (both feature selector and state reducer). I also managed to include the pattern Builder so the code looks even simpler now.

Tell me what you think!

Closes #74

Odonno commented 4 years ago

Seems good to me.

Also, this PR does not introduce breaking changes since the old CreateSubReducers function is marked as [Obsolete] at the moment.

ltjax commented 4 years ago

Hey there! Sorry it took me so long to get back to this. Been a busy week. It looks like a nice improvement, but the thing that I'm sorely missing is the ability to specify a base action for the sub-reducer. This is critical for our application. It allows you to properly work with lists in your state without duplicating the dispatch to the correct index all the time. I'm still not a big fan of the way you derive the write-back in the implicit lens, I think this: https://stackoverflow.com/questions/671968/retrieving-property-name-from-lambda-expression is a better solution. Either way, is there any reason why you did not implement the implicit lens based on the explicit one?

Odonno commented 4 years ago

Hum, I am sorry, I am having to see what you ask and what you expect.

On the most part, I think you suggest me to use Expression. I thought it was a custom class but it seems to be a part of the language. I will take a look at it to see what we can do with it.

For your last question, I did it that way because I have to reuse the ISelector and I didn't see how to do it differently. Again, I will look into it.

ltjax commented 4 years ago

No worries, I did not mean to discourage you. I'm perfectly fine with using our own implementation for now. The action thing is more important to me, the Expression thing is just a less hacky Facade really. See the first code snippet in the issue I posted and how action.Index is used there for an example.

Odonno commented 4 years ago

Sorry. Been busy.

I looked at some examples of Expression and it seems interesting. But I don't see how to use it since I want a generic solution between the new Func way and most importantly the existing ISelector way.

On the other hand, I added new state lenses that use a TAction so you can apply a selector/reducer the same way you suggested in your issue. I slightly changed the order so here is how you can use it now:

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

That will indeed create a state lens with a strict check on the type of the Action. An Exception is thrown if the type does not match.

And then, you could use it like the any other state lens.

public static IEnumerable<On<State>> GetReducers()
{
    return BuildElementReducer()
        // As many Ons as you wish
        .ToList();
}
ltjax commented 4 years ago

Sorry for the delay. That's a not bad start! The main difference is that you only throw an exception when an action is dispatched, which is potentially a pretty late. My solution enforces this at compile time, which makes mismatches a little easier to spot. Does my solution have a drawback I'm not seeing?

I'll try to dig out a code sample for the Expressions..

ltjax commented 4 years ago

So I was thinking of using something like this:

public class ExpressionAccess<T>
{
    private static MemberExpression InitialMemberExpressionFor<X>(Expression<Func<T, X>> expression)
    {
        if (expression.Body is MemberExpression memberExpression)
            return memberExpression;

        if (expression.Body is UnaryExpression unaryExpression)
            return (MemberExpression)unaryExpression.Operand;

        throw new ArgumentException();
    }

    public static List<string> MemberListFor<X>(Expression<Func<T, X>> expression)
    {
        var parameter = expression.Parameters.Single();
        var memberExpression = InitialMemberExpressionFor(expression);

        var names = new List<string>();

        while (true)
        {
            names.Add(memberExpression.Member.Name);

            if (memberExpression.Expression == parameter)
                break;

            memberExpression = (MemberExpression)memberExpression.Expression;
        }

        // Turn outside-in to inside-out
        names.Reverse();
        return names;
    }

    public static dynamic ReadInnerValue(T container, List<string> memberList)
    {
        dynamic current = container;
        for (int i = 0; i < memberList.Count; ++i)
            current = current.GetType().GetProperty(memberList[i]).GetValue(current);
        return current;
    }

    public static void WriteInnerValue(T container, List<string> memberList, object value)
    {
        object current = container;

        // All but the last need to be read
        for (int i = 0; i < memberList.Count - 1; ++i)
            current = current.GetType().GetProperty(memberList[i]).GetValue(current);

        // The last one needs to be written
        current.GetType().GetProperty(memberList.Last()).SetValue(current, value);
    }
}

Of course, you'd need an immutable variant of WriteInnerValue. Let's call it WithInnerValue. You can then do something like:

public static IStateLens<TState, TStateElement> BuildReducerFrom<TState, TStateElement>(
  Expression<Func<TState,TStateElement>> expression)
{
    // Encode the expression as a string
    var path = ExpressionAccess<TState>.MemberListFor(expression);
    return CreateSubReducers<TState, object, TStateElement>(
        (state, _) => ExpressionAccess<TState>.ReadInnerValue(state, path), 
        (state, _, newValue) => ExpressionAccess<TState>.WithInnerValue(state, path, newValue)
    );
}

With that, you can do BuildReducerFrom(state => state.Something.AnotherThing.EvenDeeper). Of course, it could be improved by also encoding other forms of "access", like supporting the index operator, but it's pretty powerful for a start.

Odonno commented 4 years ago

Since it is a while I updated this PR and the topic is quite complicated, I will merge this PR for now.

If you think there is a place for improvement @ltjax , we can continue this interesting debate on another issue or PR. :)