feat(component-store): add tapResponse operator

[alex-okrushko]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Adds a simple wrapper around tap that handles success and error + catchError(() => EMPTY).

The was some confusion as to whether catchError(() => EMPTY) was needed when tap({error: (error) => handleError(error)}) was used.

To avoid such confusion and remove a bit of boilerplate around not letting effect collapse, I suggest to have this very thin operator.

If not used, this operator is 100% tree-shakable from the bundle.

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[alex-okrushko]

FYI, we've been using this operator within Firebase Console for some time already 🙂 so this is pushing it upstream.

Also, if there are any other preferences around the naming - I'm all ears 🙂

[timdeschryver]

About the naming, what about mapResponse?
Reasoning, what is handle, and map feels RxJS-y.

[ngrxbot]

Preview docs changes for 3b25f3c at https://previews.ngrx.io/pr2763-3b25f3c1/

[markostanimirovic]

About the naming, what about mapResponse?
Reasoning, what is handle, and map feels RxJS-y.

@timdeschryver
All RxJS __map operators are used to transform emitted value and return a new one, but handleResponse is used to perform side effects 🤔

[timdeschryver]

That's a good point @markostanimirovic

[markostanimirovic]

I suggest to add two signatures for this function, similar to tap operator in order to make it more flexible.

export function tap<T>(next?: (x: T) => void, error?: (e: any) => void, complete?: () => void): MonoTypeOperatorFunction<T>;
export function tap<T>(observer: PartialObserver<T>): MonoTypeOperatorFunction<T>;

[alex-okrushko]

The goal is to not make it flexible. I want to force to handle the error.

[markostanimirovic]

Okay, I suggested that based on the implementation. According to the operator implementation too, I suggest the name safeTap or something similar. 🙂

[alex-okrushko]

Thanks @markostanimirovic that's a good suggestion.
We had similar discussions about safeCall operator for @ngrx/effect and eventually called it mapToAction and then renamed to act: #1224

Looking at the poll tapResponse appears to be the clear winner.

@beeman, technically this operator still maps, but only the error case :)

[markostanimirovic]

@alex-okrushko
Naming - the hardest thing 😄

I would also choose tapResponse between tapResponse, mapResponse and handleResponse.

Response as a part of name confused me a little bit, because it associates me with HTTP response 🙂

[alex-okrushko]

That's a good point @markostanimirovic

@timdeschryver

so tapResponse? 😅

[timdeschryver]

I was thinking the same thing @alex-okrushko
On the other hand we're still in an effect so everything is a side effect.
I would find mapResponse easier to remember and explain, because you would probably say you map the response to effect X or Y 😅

Sorry for not helping, naming things is hard...
I'm fine with the 3 options (handle, map, tap) we have, if you have a preference I'm happy to use that terminology.
If you don't have a preference we might have a problem, what about a quick poll?

[beeman]

My 2 cents, I think I like tapResponse best. With mapResponse I would expect that something would be returned, like with map() or mapTo().

[Ash-kosakyan]

Hi, can you observe this solution for tapResponse operator

export function tapResponse<T>(
  nextFn: (next: T) => void,
  errorFn: (error: unknown) => void,
  completeFn?: () => void
): (source: Observable<T>) => Observable<T> {
  return (source) =>
    source.pipe(
      tap(nextFn),
      catchError((error) => {
        errorFn(error);
        return EMPTY;
      }),
      finalize(completeFn || noop)
    );
}

[alex-okrushko]

Hi @Ash-kosakyan
What does your solution improve?

[Ash-kosakyan]

Hi @alex-okrushko
The difference: "finalize" is working every time, although observable can be errored

 /**
  * Effects
  * */
 readonly loadAll = this.effect((origin$: Observable<void>) =>
   origin$.pipe(
     tap(() => this.setLoading(true)),
     concatMap(() =>
       this.modelService.getAll().pipe(
         tap((models) => {
           this.setAll(models);
         }),
         catchError((error: HttpErrorResponse) => {
           this.setError(error.error);
           return EMPTY;
         }),
         finalize(() => {
           this.setLoading(false);
         })
       )
     )
   )
 );

[alex-okrushko]

@Ash-kosakyan - that's true. That also means that complete could bring the unexpected behavior to tapResponse.

Thanks for your input!
I think for this one tapReponse should be as close to tap as possible, so we'll keep the current implementation.

Thanks again.