🚀[FEATURE]: Cancel method on actions

[thomas-ama]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => https://github.com/ngxs/store/blob/master/CONTRIBUTING.md
[ ] Other... Please describe:

Current behavior

From what I understand, an action can be cancelled only when the parameter { cancelUncompleted: true } is present and an action arrives while a previous action is still being processed.

Expected behavior

Add a cancel() method to cancel an action manually, as in the following example:

@Action(SetNbPages)
  public setNbPages(context: StateContext<IStateModel>, action: SetNbPages): void
  {
    if (action.nbPages < 0)
    {
      action.cancel();
      return;
    }

    context.patchState({
      nbPages: action.nbPages,
    });
  }

What is the motivation / use case for changing the behavior?

I want to execute an action handler only when an action completes successfully (i.e. a new nbPages is set in the given example):

this.actions$
  .pipe(ofActionSuccessful(SetNbPages))
  .subscribe((action: SetNbPages) =>
  {
    // Do something ONLY when a new nbPages is set
  });

Without this feature, the action dispatch must be included in a service method that decides if the action must be dispatched or not:

@Injectable()
export class MyService {
  ...
  public setNbPages(nbPages: number): void
  {
    if (nbPages > 0)
    {
      this.store.dispatch(new SetNbPages(nbPages));
    }
  }
  ...
}

However, I have to inject this service wherever I want to dispatch my action. This creates dependencies on this service at several places in the code.

Moreover, if another dev was not aware of the setNbPages() method, he may directly dispatch an action and the condition nbPages > 0 would never be checked. The idea is thus to centralise action guards in the state and cancel the action if the guards do not pass.

[vdjurdjevic]

I need this too. I have started a thread on slack, but I will write my use case and motivation here also. So, in NGRX world, it's common practice to store loading indicator and error object inside the state, and use MyActionSuccess and MyActionFailed pattern to control async action results. And it makes sense. If I have two parts of UI that visualize that part of the state, I want to show spinner in both components. The problem is lots of boilerplate with this. With NGXS actions$ handler, and ofActionSTATE operator, I think we have a great solution for reducing action boilerplate. But, there is one problem. We are not able to manually control action result ("DISPATCHED", "SUCCESSFUL", "CANCELED" or "ERRORED") manually, withing action handler. Imagine this scenario. I have modal which renders login-form component. Login form dispatches LoginAction, which is async action. If everything goes fine, modal is subscribed to ofActionSuccess(LoginAction) and closes modal. It's pretty much the same as the example with the alert in the documentation. I need to react on the action, not just on state change to close modal when login succeeds. The problem is with error scenario. My action handler has try catch block, and calls api with await. In catch block, i patch state with error. Since i handled an error, this action ends with "SUCCESSFUL" state, and get's picked up by ofActionSuccess operator, and closes modal. This is not desired outcome, because i would like to leave modal open, so that user can see that error message. Workarounds are:

1. Create LoginFailedAction and dispatch it in catch block. This defeats purpose of ofActionSTATE operator. It's basically: "This action succeeded in telling me that other action failed". Writing something like this: ofActionSuccess(LoginFailedAction) is not that great.

2. Rethrow error in catch block. This works but has another down break. Now the error is picked up by angulars global error handler as unhandled, and it's really not unhandled. Since I have a custom error handler that sends those errors to the server, I really don't need all these errors to clutter my logs.

So, the ability to manually fail or cancel action would be a great solution to this problem. It would allow actions to fail gracefully, and be handled both in terms of updating state with an appropriate message, and also react with alerts, snack bars, toasters or whatever when needed.

[tdekoekkoek]

This is a critical need. I need the ability as @thomas-ama has mentioned of having action guards. I think this should behave similar to route guards. The idea is that each action should be prevented from continuing successfully if certain rules are not met (such as not authenticated, incorrect role, etc.)

[autaut03]

@tdekoekkoek I believe implementing a guard system is overengineering, because the problem can really be solved easier for both developers using NGXS and those maintaining it. I personally like the proposed API, but it would mean that every action class must extend some other class (which provides cancel method).

However, while cancel API isn't bad, can someone explain why we can't have the ability to return values from actions? Currently, returned value is a whole state, which is pretty useless. Correct me if I'm wrong, but wouldn't it make sense to just pass the returned value to dispatch's observable?

This way, we could have even greater control without breaking existing action definitions:

	@Action(SendChatMessage)
	public sendMessage(ctx: StateContext<ChatStateModel>, action: SendChatMessage): Observable<boolean> {
		if (!this.store.selectSnapshot(AuthState.isAuthenticated)) {
			this.store.dispatch(new Navigate(['/', { outlets: { modal: 'auth/login' } }]));

			return of(false);
		}

		if (ctx.getState().sendingMessage) {
			return of(false);
		}

		ctx.patchState({
			sendingMessage: true,
		});

		return this.chatApi.sendMessage(ctx.getState().room, action.body).pipe(
			finalize(() =>
				ctx.patchState({
					sendingMessage: false,
				})
			),
			map(() => true),
		);
	}

This way, we won't feel the need of adding data kind of parameter to cancel() (because someone will definitely need to distinguish different cancelation reasons) and whole thing will just be simpler.

[arturovt]

@autaut03

This is by design. Action can be handled in 500 states and therefore it's not realistically possible to determine what value we have to return. Currently dispatch returns the whole state but you should not rely on this. It is not an explicit part of the interface and will be changing in v4. You should treat it as if it returns void.

Correct me if I'm wrong, but wouldn't it make sense to just pass the returned value to dispatch's observable?

No. Actions are commands in CQRS and there is no such concept as "returing result from command handler".

[autaut03]

@autaut03

This is by design. Action can be handled in 500 states and therefore it's not realistically possible to determine what value we have to return. Currently dispatch returns the whole state but you should not rely on this. It is not an explicit part of the interface and will be changing in v4. You should treat it as if it returns void.

Correct me if I'm wrong, but wouldn't it make sense to just pass the returned value to dispatch's observable?

No. Actions are commands in CQRS and there is no such concept as "returing result from command handler".

Okay, but we still need a way to cancel the action from inside. Then someone will need to pass additional data as to why the action was canceled. In the end, we are still returning a value, one way or another, aren't we?

[arturovt]

@autaut03

Action handlers can return Observable/Promise because NGXS is able to process asynchronous job. That code that you showed is a little bit unusual as I'm not able to understand what return of(false) is for (whereas single return; statement is enough) and what map(() => true) is for. Or this was just for demonstrating purposes? But this wasn't mentioned.

but we still need a way to cancel the action from inside

Actions are cancelable by providing cancelUncompleted option. The issue title is a bit dissimilar to the idea. This should be action guards or smth.

[autaut03]

@arturovt Sorry, I might have described it poorly. What I'm trying to achieve is to clear an input, but only if and after action succeeded. A simple, but concrete use case. In my example, I just used false and true values to represent canceled and successful action results respectively.

cancelUncompleted doesn't let me cancel the action from inside the action, the way @thomas-ama described.

I've read a little bit about CQRS. Commands might not return a value, but events can still be emitted (action handlers in case of NGXS) and can even carry values. However, ActionCompletion event does not allow us to do that:

interface ActionCompletion<T = any> {
  action: T;
  result: {
    successful: boolean;
    canceled: boolean;
    error?: Error;
  };
}

Allowing it to carry results from action completion would make sense and doesn't contradict CQRS. Will that be a solution?

[splincode]

@arturovt is actual?

[markwhitfeld]

Just to bring up this old discussion again, I think that this could be solved in a slightly different way...

Regarding the proposed solution:
If someone is invoking cancel() then they might have the expectation that other handlers for the same action would stop any further work. While it is possible to implement this sort of thing (by giving the same ctx to all of the handlers for the same dispatched action, and calling cancel on it basically makes a setState or patchState a no-op), I think that it could have side effects that are difficult to reason about. We don't want to open the door for difficult to understand execution flows.

A potential alternative is this:
We could have a ctx.setActionResult(...) this could allow for the user to gain control of the result status.
The ... argument could be created by a helper like one of the following:
actionCanceled(), actionSuccessful(), actionErrored(error)
This would allow for the user to control this result without the expectation of influence over other handlers of the same action. While I think that it is better in the majority of cases to write this sort of contextual information into the state so that it is available throughout the app, there may be some architectures that this type of thing makes more sense.
If we were to move forward with the change I propose, then there would need to be:

1. Some changes so that the ctx has more context of which action it is for, allowing for the setActionResult call to know which action it applies to
2. We would need to decide what happens if another handler also calls setActionResult. Would this throw an error/warning? Or would it override the previous result? Or would it be some sort of collated result where errors are higher priority than cancellations, etc.?
3. We need to decide if this violates the CQRS pattern, and that there is no better alternative or helper that we can provide to assist with this type of use case.
4. We need to decide what effect this result will have on the observable returned by the dispatch call... would the provided error propagate to the observable? would an error end up on the global error handler if it is not handled?