Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow DI in Effects #92

Open
nicolassargos opened this issue Apr 9, 2021 · 11 comments
Open

Allow DI in Effects #92

nicolassargos opened this issue Apr 9, 2021 · 11 comments
Labels
breaking changes Will break the current behavior of the project help wanted Extra attention is needed refactoring Improve source code which is not related to any bug or feature

Comments

@nicolassargos
Copy link

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
Copy link
Owner

Odonno commented Apr 10, 2021

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.

@Odonno Odonno added help wanted Extra attention is needed refactoring Improve source code which is not related to any bug or feature breaking changes Will break the current behavior of the project labels Apr 10, 2021
@nicolassargos
Copy link
Author

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
Copy link
Owner

Odonno commented Apr 11, 2021

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
Copy link
Author

I was mostly referencing Issue #76

@name1ess0ne
Copy link

name1ess0ne commented Feb 2, 2022

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
Copy link
Owner

Odonno commented Feb 2, 2022

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
Copy link

name1ess0ne commented Feb 2, 2022

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
Copy link
Author

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
Copy link
Owner

Odonno commented Feb 3, 2022

@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
Copy link
Owner

Odonno commented Feb 3, 2022

@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
Copy link
Author

nicolassargos commented Feb 19, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Will break the current behavior of the project help wanted Extra attention is needed refactoring Improve source code which is not related to any bug or feature
Projects
None yet
Development

No branches or pull requests

3 participants