ngrx
`tapResponse` swallowing errors happening inside the `tapResponse`
Minimal reproduction of the bug/regression with instructions
// not working as expected
of(null)
.pipe(
tapResponse(
() => {
throw new Error(
`This error will be swallowed and you'll have a hard time debugging`
);
},
(error) => console.log('Some error happened', error)
)
)
.subscribe();
That error :point_up: is silently caught which was painful to debug in my case. Here's a live repro on stackblitz
https://stackblitz.com/edit/angular-ivy-7jhxew?file=src%2Fapp%2Fapp.component.ts
Minimal reproduction of the bug/regression with instructions
To have an error thrown correctly.
Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)
Other information
This very much looks like an issue I had with ngrxPush pipe here: https://github.com/ngrx/platform/issues/3100
I would be willing to submit a PR to fix this issue
- [X] Yes
- [ ] No
Not sure what's best here between
- not handling an error that'd happen in the
tapResponsenextcallback so that it's broadcasted all the way up - handle the error happening within the
tapResponsenextcallback within the catch error of that custom operator
@maxime1992
The tapResponse is used to keep the effect active if an error is thrown. Because of that, tapResponse swallows the thrown error. The error callback is not executed, because the error that is thrown within the next callback of tap operator will not be caught by its error callback:
**tap**({
next() {
throw new Error('ERROR!');
},
error(err) {
// ERROR! won't be caught here
}
})
The solution could be to move the error callback execution from tap to catchError:
function tapResponse(next, error, complete?) {
return pipe(
tap({ next, complete }),
catchError((err) => {
error(err);
return EMPTY;
});
);
}
cc @alex-okrushko
For now, you can use tap + catchError:
.pipe(
tap(() => {
throw new Error('ERROR!');
}),
catchError((err) => {
// handle error here
return EMPTY;
})
)
The solution could be to move the error callback execution from tap to catchError
I would find that weird. For me, or how I think about it, the error callback is to handle errors that are thrown while doing the request, not for errors in next.
What about adding a config, similar to the EffectConfig to bypass the default behavior?
I would find that weird. For me, or how I think about it, the error callback is to handle errors that are thrown while doing the request, not for errors in next.
Yes 😐, it can be confusing because the behavior would be different from the tap operator.
What about adding a config, similar to the EffectConfig to bypass the default behavior?
🤔 I'd not add config argument, because the implementation with tap + catchError looks simpler IMO:
tap+catchError:
tap(() => {
throw new Error('ERROR');
}),
catchError((err) => {
// handle error
return EMPTY;
}),
tapResponse+ config:
tapResponse(
() => {
throw new Error('ERROR'),
},
(err) => {
// handle error
},
{ handleUnexpectedErrors: true },
)
What about adding a config, similar to the EffectConfig to bypass the default behavior?
I don't think this would be good enough. Unless this is pretty much the first thing first first thing in the documentation, someone could miss it and the frustration to understand what's going on with a silently swallowed error can be really high. I do believe there's a need for a default behavior where you can handle this. I can't think of a case where someone would let its stream error without being warned at least
Probably a super naive solution, still I want to share it:
Try catch around the nextFn...
export function tapResponse<T, E = unknown>(
nextFn: (next: T) => void,
errorFn: (error: E) => void,
completeFn?: () => void
): (source: Observable<T>) => Observable<T> {
return (source) => {
return source.pipe(
tap({
next: (v) => {
try {
nextFn(v);
} catch (error) {
console.error('An error occurred in the next callback', error);
}
},
error: errorFn,
complete: completeFn,
}),
catchError(() => EMPTY)
);
};
}
OK, looking at it again a few days later... I do not like it anymore. The error callback and complete callback could swallow errors as well. These callbacks would need try catch as well. That would become ugly quickly :)
FYI: The effect method example from here (https://ngrx.io/guide/component-store/effect#effect-method) would swallow the next fn error as well:
The example could be rewritten to this:
switchMap((id) => apiCall(id).pipe(
tap({
next: (movie) => this.addMovie(movie)
}),
catchError(() => {
this.logError(e);
return EMPTY;
))),
Then api errors and next errors would be logged. I believe tapResponse should work like that. Thats pretty much what @markostanimirovic suggested already.
Fixed it with try catch in MiniRx :) https://github.com/spierala/mini-rx-store/pull/117/files
Just spent hours finding this problem. Here's the example showing the problem - https://stackblitz.com/edit/angular-ivy-nxf2la.
Just spent hours finding this problem. Here's the example showing the problem - https://stackblitz.com/edit/angular-ivy-nxf2la.
what is the solution for this? any work around @markostanimirovic?
Even through there is an expectation that next callback doesn't throw an error, we can adjust to catch errors in the next as well, I don't think there's a problem.
The question that I have is would you expect an error in the error callback to be handled somehow as well?
In my opnion, whatever happens in both - no exception must be swallowed. If it throws in next => error should be fired. If it's thrown in error => console.error by default. But add the option to override this.