Store: Cancelable Actions Decorator Option

[amcdnl]

Add the ability to cancel previously queued actions via a API like this:

@Action(RemoveTodo, { cancelable: true })
remove() { ... }

eliminating the need for doing this:

  @Action(RemoveTodo)
  removeTodo(state: StateContext<string[]>, action: RemoveTodo) {
    return timer(5000).pipe(
      tap(() => {
        state.setState(
          state.getState().filter((_, index) => index !== action.index)
        );
      }),
      takeUntil(this.actions.pipe(ofAction(RemoveTodo)))
    );
  }

Ref: #143

[markwhitfeld]

PS. You would probably have to support the option of "cancelable" or "cancellable" for US or UK english spelling (See here).

[amcdnl]

@markwhitfeld - any other thoughts? You had some interesting ideas here.

[deebloo]

There is another issue about this open

On Thu, Mar 29, 2018, 9:43 AM Austin [e-mail] wrote:

@markwhitfeld https://github.com/markwhitfeld - any other thoughts? You
had some interesting ideas here.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/ngxs/store/issues/191#issuecomment-377239152, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AGQnJe8NtvMvmT2OUxI-m_MaCSMbSD3Oks5tjOTugaJpZM4S_YC2
.

[deebloo]

see #192

[andre-davcev]

@amcdnl Small question here. Is there a reason we need to explicitly define whether it is cancelable? Was thinking it might be simpler to have it implicit.

[amcdnl]

@greycloak - u only want this for typeaheads, saves should always happen.

[markwhitfeld]

Just to add to the mix, I think that the option name of 'cancelable' is maybe a bit ambiguous regarding its impact and behavior. Maybe @greycloak was asking about it because it was unclear...?
To make it more user friendly there could be another setting name that would be better suited.
Here are some options of names from the top of my head:
cancelsPrevious, cancelPrevious, latestCallOverrides, latestCallWins, latestCallCancelsPrevious

In my opinon the cancelsPrevious is clean and to the point, and when I did a search about the differences between SwitchMap, MergeMap and ConcatMap, the SwitchMap was described as it cancels the previous observable, as would be implied explicitly by this setting name.

One subtle issue I have with the cancelable setting name is that this is US spelling and it would be cancellable as UK spelling (as mentioned previously). This would be sure to cause confusion with framework users.

My vote is for cancelsPrevious. Thoughts? Suggestions? @deebloo @amcdnl

PS. I think we would also have to create a preservesOrder setting to represent the equivalent of ConcatMap at some stage.

[andre-davcev]

@amcdnl @markwhitfeld We have an action that will kick off a chain of other actions depending on the success of the previous. My assumption was that we need to use a switchMap to cancel the current action. And then we can just subscribe to the original dispatch and will be notified when it finishes the whole chain or fails along the way. The problem is that we see two emits on the first success action and 4 emits on the 2nd. My thought was that this enhancement for 2.1 was related to that. Or it's also possible we're just using ngxs or rxjs wrong.

[amcdnl]

Implemented in 2.1.0-beta.0 using takeLast name.

[markwhitfeld]

Hi @amcdnl and @deebloo
I was wondering about the reasoning behind the option name of takeLast?
I find it a bit unintuitive because the takeLast rxjs operator will only apply once the observable completes. In our case the action stream never completes, so it means that we are creating a meaning for this that it outside its original meaning. Are we happy with this?
If I saw the option being used in code I wouldn't know what it meant until I read the docs.

Is it still possible to review the option name?

[amcdnl]

@deebloo - he liked that one, i'm indifferent.

[markwhitfeld]

@deebloo I looked at your doc comment where you describe the takeLast option. You describe it as follows:
* Cancel the previous uncompleted request(s).
Do you think we could change the name to be closer to the description?
ie. cancelPrevious, cancelUncompleted, cancelPreviousUncompleted

I think I prefer cancelUncompleted.
I'm happy to chat on gitter. Ping me when you are available.

[deebloo]

will do. I personally wanted it to be called "cancelPrevious". The inline comment should also be updated since it is canceling previous observables not necessarily previous requests.

[andre-davcev]

@amcdnl Whatever it is you changed, you fixed our observable emit permutations issue. Thanks! Awesome library. Boilerplate hell no more.