mrpmorris / Fluxor

Fluxor is a zero boilerplate Flux/Redux library for Microsoft .NET and Blazor.
MIT License
1.23k stars 139 forks source link

Make the IState interface covariant #342

Closed Venom1991 closed 1 year ago

Venom1991 commented 1 year ago

Hi there,

I'd like to be able to write something like:

abstract class RootState
{
    public boolean IsLoading { get; }

    public RootState()
    {
        IsLoading = false;
    }
}

[FeatureState]
class FooState : RootState { }

[FeatureState]
class BarState : RootState { }

public void Baz(IEnumerable<IState<TState>> allStates) where TState : RootState
{
    // do something with the "IsLoading" prop contained in all of the enumerable's items' "Value" props
}

// Main()

// get all configured states whose classes derive from RootState (instances of FooState and BarState, in this example)
var allStates = serviceProvider.GetServices<IState<RootState>>(); // MS DI in this example, it doesn't really matter

Baz(allStates);

Currently, this code does not compile - for obvious reasons.

Would it be possible to support a use case such as this one? It's fine even if it isn't, thank you so much for this awesome library!

mrpmorris commented 1 year ago

Feel free to fork, and then submit a pr to the release/5.5 branch :)

Out of curiosity, what are you doing?

Venom1991 commented 1 year ago

I'm trying to subscribe to changes made to the "IsLoading" property of all the state instances (concrete classes, derived from RootState) by using the CombineLatest operator.
If any of the flags is set to true a loader (an animated GIF) is shown on top of the current window whose content is also blurred. Otherwise, the loader is hidden and the current window's content blur is removed (its "Effect" prop is nullified, to be precise). It's a fairly simple WPF + ReactiveUI app.

Currenty, this feature is implemented like this:

private void InitializeIsLoading
    (
        IState<FooState> fooState,
        IState<BarState> barState,
        IState<BazState> bazState
    )
{
    Observable.CombineLatest
        (
            fooState.ToObservable(value => value.IsLoading),
            barState.ToObservable(value => value.IsLoading),
            bazState.ToObservable(value => value.IsLoading)
        )
        .Select(collection => collection.Contains(true))
        .ObserveOnMainThread()
        .ToPropertyEx(this, vm => vm.IsLoading);
}

It'd be "prettier" to pass a collection to the "InitalizeIsLoading" method and then simply call Select() on it and then pass the resulting collection to CombineLatest(). Also, further additions of feature states wouldn't require changes to be made to this method.
This works but it's a bit ugly, subjectively speaking.

mrpmorris commented 1 year ago

Try forking, work from release/5.5 and reference the libraries directly from your code. Update to see if it will do what you want, if so, submit a PR.

Venom1991 commented 1 year ago

I'll see what I can do about it when I find the time. Thank you.

andrewtsybulia commented 1 year ago

I'll see what I can do about it when I find the time. Thank you.

Hi, I've run into a problem with the task to handle the loading state in my app. Have you found some way out? It would be great if you share anything :)

Venom1991 commented 1 year ago

Hi, I've run into a problem with the task to handle the loading state in my app. Have you found some way out? It would be great if you share anything :)

Nope, I still haven't found the time - sorry. I'll try to follow @mrpmorris 's advice and report back here once I've come up with something that is PR-worthy. :+1:

berhir commented 1 year ago

I have created a PoC for Fluxor.Selectors. See #344 With selectors it should be easy to archive what you want:

var selectFooState = SelectorFactory.CreateFeatureSelector<FooState>();
var selectBarState = SelectorFactory.CreateFeatureSelector<BarState>();
var selectBazState = SelectorFactory.CreateFeatureSelector<BazState>();

var selectIsFooLoading = SelectorFactory.CreateSelector(selectFooState, state => state.IsLoading);
var selectIsBarLoading = SelectorFactory.CreateSelector(selectBarState, state => state.IsLoading);
var selectIsBazLoading = SelectorFactory.CreateSelector(selectBazState, state => state.IsLoading);

var selectIsAnyLoading = SelectorFactory.CreateSelector(
    selectIsFooLoading, 
    selectIsBarLoading,
    selectIsBazLoading,
    (isFooLoading, isBarLoading, isBazLoading) => isFooLoading || isBarLoading || isBazLoading);

// get the actual value
var isAnyLoading = store.Select(selectIsAnyLoading);

When using the SelectorFactory.CreateSelector and SelectorFactory.CreateFeatureSelector functions Fluxor.Selectors keep track of the latest arguments in which your selector function was invoked. Because selectors are pure functions, the last result can be returned when the arguments match without reinvoking your selector function. This can provide performance benefits, particularly with selectors that perform expensive computation. This practice is known as memoization.

Venom1991 commented 1 year ago

Yes, I'm familiar with the concept of memoized state selectors because I've also used NgRx in an Angular app, for quite some time.
Personally, I support your effort and would like to see this PoC being transformed into a PR and eventually merged but that depends mostly on what are @mrpmorris's recent thoughts on the subject. AFAIK, he isn't quite fond of it - see #213

As useful as it is, it still doesn't solve the issue of unnecessary code duplication and lack of genericness regarding the selection of properties common to all of the feature states in any given app.

Consider an introduction of a "SnafuState" whose defining class also inherits from RootState and thus contains the "IsLoading" property like all the others do. Your snippet of code would have to be significantly changed - only the last line would remain as it is.

var selectFooState = SelectorFactory.CreateFeatureSelector<FooState>();
var selectBarState = SelectorFactory.CreateFeatureSelector<BarState>();
var selectBazState = SelectorFactory.CreateFeatureSelector<BazState>();
var selectSnafuState = SelectorFactory.CreateFeatureSelector<SnafuState>();

var selectIsFooLoading = SelectorFactory.CreateSelector(selectFooState, state => state.IsLoading);
var selectIsBarLoading = SelectorFactory.CreateSelector(selectBarState, state => state.IsLoading);
var selectIsBazLoading = SelectorFactory.CreateSelector(selectBazState, state => state.IsLoading);
var selectIsSnafuLoading = SelectorFactory.CreateSelector(selectSnafuState, state => state.IsLoading);

var selectIsAnyLoading = SelectorFactory.CreateSelector(
    selectIsFooLoading, 
    selectIsBarLoading,
    selectIsBazLoading,
    selectIsSnafuLoading
    (isFooLoading, isBarLoading, isBazLoading, isSnafuLoading) => isFooLoading || isBarLoading || isBazLoading || isSnafuLoading);

// get the actual value
var isAnyLoading = store.Select(selectIsAnyLoading);

I'm not entirely sure but I believe this is a problem in NgRx, as well, but that could be caused by the inherent limitations (regarding the type system, static typing and such) of transpiling TS to JS.

berhir commented 1 year ago

Thank you for pointing me to the previous discussion on this topic.

I understand your point that there is a lot of "duplicate" code. But this way we can make use of memoization and recalculate the isAnyLoading value only if needed. In your implementation, it will be recalculated every time when any value in one of the states changes. In this case the calculation is very simple and it won't be a big deal. But if the calculation is more complex, memoization will improve the performance.

It's easy to create a generic selector for your use case without memoization:

// create a new factory method
public static Selector<List<TFeatureStateBase>> CreateFeatureSelectorForStateBase<TFeatureStateBase>()
{
    return new Selector<List<TFeatureStateBase>>(store =>
    {
        var states = new List<TFeatureStateBase>();

        foreach (var feature in store.Features.Values)
        {
            if(feature.GetState() is TFeatureStateBase state)
            {
                states.Add(state);
            }
        }

        return states;
    });
}

// base state type
abstract class RootState
{
    public bool IsLoading { get; }

    public RootState()
    {
        IsLoading = false;
    }
}

// create the selectors
var selectRootStates = SelectorFactory.CreateFeatureSelectorForStateBase<RootState>();
var selectIsAnyLoading = SelectorFactory.CreateSelector(selectRootStates, rootStates => rootStates.Any(state => state.IsLoading));

// get the actual value
var isAnyLoading = store.Select(selectIsAnyLoading);
mrpmorris commented 1 year ago

Do you still feel there is a need for this? If so, can we discuss it so I can consider it for V6?

Venom1991 commented 1 year ago

@mrpmorris

I've since left the company and consequently the project which utilized Fluxor so I suppose that there isn't any valid reason for implementing this enhancement - at least from my perspective.👍🏼

I'm sorry for not coming back here earlier with this "update".

mrpmorris commented 1 year ago

Thanks for letting me know!

What is the name of the company?

Venom1991 commented 1 year ago

Thanks for letting me know!

What is the name of the company?

King ICT - one of the bigger IT companies located here in Croatia where I'm from.

Why do you want to know, might I ask?

mrpmorris commented 1 year ago

Just curious. I mostly have no idea who uses my library, what it's used for, or what their experiences with it are.

Venom1991 commented 1 year ago

Just curious. I mostly have no idea who uses my library, what it's used for, or what their experiences with it are.

Sure, having some feedback (positive or negative) is always nice.

Personally, I have had only positive experiences with your library. It's simple, straightforward but also fairly powerful at the same time. Granted, one has to be at least somewhat acquainted with the Flux pattern itself but that's besides the point.