Skip to content
This repository was archived by the owner on Jul 8, 2021. It is now read-only.

New @Effect decorator #43

Open
andreyan-andreev opened this issue Mar 2, 2018 · 3 comments
Open

New @Effect decorator #43

andreyan-andreev opened this issue Mar 2, 2018 · 3 comments

Comments

@andreyan-andreev
Copy link

andreyan-andreev commented Mar 2, 2018

Hi there, very nice project, great job.

This is not an actual issue but more a question or a suggestion, depending on how you look at it. I am still very very green in this whole observable stuff, ngrx and etc. I have setup a project that is still relatively small and the other day i saw that you added this new @effect() decorator that is allowing me to consolidate also the effects under my store file, so to the question:

@Effect(DeliverPizza)
deliverPizzaToCustomer(state, { payload }: DeliverPizza) {
    this.pizzaService.deliver(payload);
}

What is the reasoning in providing the state parameter as first argument to the decorated method?

I find it handy to have the state supplied to the method, but most of the time in my effects I do only care about the action payload. Its not a big deal it just makes the linter to underline it. besides that I need to also type it each an every time. Maybe if it comes as a second argument it will be better, as I can simply omit the state if I don't need it.

Apparently this is not valid for the @action decorator as the reducer method would definitely need both parameters. Mentioned this, because it maybe that you wanted to be consistent with the decorators signatures.

Great work again.

@andreyan-andreev
Copy link
Author

andreyan-andreev commented Mar 3, 2018

Very promising work again. I guess that this will not work with redux dev tools?

On the original comment:

    @Action(NewAnimal)
    newAnimal(state, { payload }) {
        return this.animalService.save(payload).map((res) => new AnimalSuccess(res));
    }

In ngxs @action() that has taken the role of ngrx-action @effect() as you can see even in your own example, the state is not used in the newAnimal method. Its true that it maybe be needed from time to time but most of the time it will not. My suggestion for this project @effect() and for the ngxs project @action() is to have the payload as first argument and state as second.

Thanks.

@abalad
Copy link

abalad commented Mar 5, 2018

@amcdnl LOL How cool is your project much more practical than NGRX.

@amcdnl
Copy link
Owner

amcdnl commented Mar 5, 2018

@abalad - You should checkout NGXS -> https://github.com/amcdnl/ngxs

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants