ngxs / store

🚀 NGXS - State Management for Angular
http://ngxs.io
MIT License
3.53k stars 399 forks source link

this.store.dipatch.subscribe(value => {}) in case of action success value is state, otherwise it is error #313

Closed AlgoTrader closed 6 years ago

AlgoTrader commented 6 years ago

this.store.dipatch.subscribe(value => {})

When action was success value is state When action errored value is error

As a result, I always have to inspect value type in subscribe callback of any dispatch Also 'DISPATHCED' and 'COMPLETED' are capitalized. but 'Errored' is first capitalized

deebloo commented 6 years ago

I think that is a bug in that we don't want any value coming out of the subscribe. @amcdnl is that correct?

AlgoTrader commented 6 years ago

I would prefer this.dispatch(new Action) to push value on success and throw on error

this.dispatch(new DoComplexStuff)
   .subscribe(successCb. errorCb)

Now if action throws, I get successCb with error value. I don't like values that I have to deep inspect to decide dispatch was success or failure

deebloo commented 6 years ago

ah I see. Let me take a look and see how we are handling these errors

juristr commented 6 years ago

When action was success value is state

Also, the question is whether this is actually desired. Because right now, since a successful dispatch returns the entire state obj (which IMHO is useless), the following code from the docs also wouldn't work any more:

export class ZooComponent {
  @Select(state => state.animals) animals$: Observable<any>;
  constructor(private store: Store) {}
​
  addAnimal(name) {
    this.store.dispatch(new AddAnimal(name))
      .pipe(withLatestFrom(this.animals$))
      .subscribe(([animals]) => {
        // do something with animals
        this.form.reset();
      });
  }
}

Because now the value of the .subscribe(..) would be an array having as first value the entire state tree and as 2nd value (in this case) the last value from animals$.

Personally I'd prefer not having a return value from the action because it diverges a bit from the CQRS pattern, where you should separate "writes" from "reads". I'm perfectly fine to be able to subscribe to know when the action "is done".

AlgoTrader commented 6 years ago

I also think the entire state in callback is useless. First, I can select any state slice if I need it. Second, I often want to know the result of the last dispatch. I would prefer to see the result of action but not the whole state

deebloo commented 6 years ago

I agree with both of you. So there are two parts.

  1. no state should back in the callback.
  2. if there is an error it should fire the subscribers errors callback
AlgoTrader commented 6 years ago

Exactly. Errors should be explicit

amcdnl commented 6 years ago

I agree w/ @deebloo

amcdnl commented 6 years ago

I believe this is resolved in 3.0

juristr commented 6 years ago

@amcdnl "believing" is not enough Austin. We want proof 😉. As soon as I can I'm gonna test it and let you know in case