Odonno / ReduxSimple

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

Allow DI in Effects #92

Open nicolassargos opened 3 years ago

nicolassargos commented 3 years ago

Hi there, And first of all congratz for your work :)

Just starting with ReduxSimple and works great (despite the misleading examples as described in a Issues post), but I have encountered an unexpected issue: As Effects, Actions and so on are static classes, I can't see a way to inject a httpClient to be able to fetch data. Am I missing something here ? I can't imagine creating a httpClient as a private static field.

Regards, Nico.

Odonno commented 3 years ago

Hi Nicolas, thanks for your words.

Indeed, the original implementation of ReduxSimple was and is still Static in order to give you a singleton object to use anywhere in your project. Now, that you may know, ReduxSimple is strongly inspired by the ngrx library. And by design, since it's an Angular project, almost everything is an injectable class : at least Store, Actions and Effects.

So, your point is very relevant but at the same time, it is a huge decision because it will break the entire app for current developers & it will take a huge amount of time to change the source code.

I'll put it in the backlog for the next major release then.

nicolassargos commented 3 years ago

Hey, you're welcome.

Indeed, changing the implementation from static to an instatiable class seems a huge amount of work (and breaking change for everybody updating to the new version). I'm using ngrx at work, and that is the reason why I chose your library: I really feel at home.

So right now, I'm trying to implement a store to keep my state in a Blazor Server application because of dynamic effects in the screens. I did a custom implementation at first with Observable collections and INotifyPropertyChanged but, due to my lack of experience, it started to become a mess.

Anyway, I guess the only solution is to use a static httpClient with hardcoded routes. Thank you anyway, can't wait to see your next upgrades!

Sidenote: I really would update the doc if I were you cause now it is misleading and can be frustrating.

Odonno commented 3 years ago

Yep, sure, you can create your own Observable operator, and inside this operator you could instantiate the HttpClient on each use. But I admit it's a lot of work and the simplicity would be to use the Injected HttpClient like you'd do in ngrx.

What part of the documentation is misleading?

nicolassargos commented 3 years ago

I was mostly referencing Issue #76

name1ess0ne commented 2 years ago

Hello Odonno! Thank you very much for the great library! I have suggestion that may help with DI in Effects. I think it could be done similarly to https://github.com/reduxjs/redux-thunk Basically, you can still have static Effects, but you will need to add EffectArgs field to ReduxStore.

Something like:

public static Effect<RootState> GetTodos = CreateEffect<RootState>(
    (store) => store.ObserveAction<GetTodosAction>()
        .Select(_ =>
            store.EffectArgs.TodoApi.GetTodos() // <--- all API objects could be hold in this field. User will have to specify them during store creation
                .Select(todos =>
                {
                    return new GetTodosFulfilledAction
                    {
                        Todos = todos.ToImmutableList()
                    };
                })
                .Catch(e =>
                {
                    return Observable.Return(
                        new GetTodosFailedAction
                        {
                            StatusCode = e.StatusCode,
                            Reason = e.Reason
                        }
                    );
                })
        )
        .Switch(),
    true
);

This is pseudo code. I'm not sure if there is possibility to keep EffectArgs strongly typed. But if you are interested in this approach I may investigate more and publish PR.

Odonno commented 2 years ago

Hi @name1ess0ne

As I am primarily focused on being close to the ngrx implementation, I don't think I am going to follow what you're suggesting. However, I really don't have much time to work on this project at the moment. If you'd like to contribute, it would be great.

But I know this issue is a more than an EPIC as we need to rewrite every element in this lib (store, effects, etc..) to be injectable.

Anyway, thank you for your interest on this.

name1ess0ne commented 2 years ago

Hi @Odonno! Unfortunately, I'm not really familiar with ngrx implementation. I'm guy from react/redux world =) Not sure if I will be able to correctly implement ngrx approach without experience in it =( Let me read about it.

nicolassargos commented 2 years ago

Hi, as I didn't know exactly from where to write you, I'll post here. Finally ended a couple of months ago a tiny library with restricted features to use in a Blazor server app, with a common state to multiple cients, allowing real time synchronization instead of long polling. Works well actually as I use it in my app and it allows DI in effects with all its benefits. For now, the nuget is private as I consider you wrote 75% of the library, but I was thinking of making it public. In a way, it's just a fancy (except for the declarative expression) way to use the command patter coupled with with singalR.

Anyway, if you are interested, don't hesitate to contact me. Once again, thx a lot, your project was an amazing help to do mine.

Odonno commented 2 years ago

@name1ess0ne No worries. I have done projects with both and both have really interesting features. I tend to prefer the ngrx implementation, at the core/architecture level and because ReduxSimple is using Rx the same way ngrx does, I will continue that way. However, I am not against improvement coming from Redux Toolkit, like slices #99.

Odonno commented 2 years ago

@nicolassargos Yeh, good to know. From what I understand, you rewrote some parts of ReduxSimple with adding some edge cases with realtime features, like SignalR. Is that correct?

I would really like to see you contribute to this repository. As I have started ReduxSimple.Uwp packages for UWP apps, it would be amazing to see ReduxSimple.Blazor packages at the same level, maybe on a greater level. ;)

However, ReduxSimple should only focus on its sole purpose: State Management. So, the SignalR part should be decoupled. You can of course create another repository called ReduxSimple.SignalR and publish this package on your own. A repository that you can make public if you desire.

Thanks for your dedication on this.

nicolassargos commented 2 years ago

Hi @Odonno

Sry for answering so late, I've been quite busy lately. I added you as a contributor as for now it's still a private repo. It's very light, so you can have a quick look at it if you want just to have a better idea on what it is about and how it's done. (Also, I'm french too, so feel free to talk in our native language ;) ). Obviously, remarks are welcome as I'm in development for only 3 years, so I get that I still have many things to learn.

Cheers, Nico.