platform icon indicating copy to clipboard operation
platform copied to clipboard

RFC: Resubscribe on errors in ComponentStore.effect()

Open e-oz opened this issue 2 years ago • 4 comments

Which @ngrx/* package(s) are relevant/related to the feature request?

component-store

Information

Right now, there are two possible ways to accidentally "kill" an effect:

  1. Forgetting to catch an error in the body of the function passed to effect().
  2. Forgetting to catch an error in the observable that is passed as a value for an effect.

There is a simple way to avoid them in the effect() itself: resubscribe on errors using retry().

Describe any alternatives/workarounds you're currently using

Currently, the closest alternative for option(1) is tapResponse(), although it's easy to forget or to put it in the wrong place.

I would be willing to submit a PR to fix this issue

  • [X] Yes
  • [ ] No

e-oz avatar Jul 19 '23 18:07 e-oz

I like the idea, but the 'problem' with retry is that it will swallow the thrown error (https://stackblitz.com/edit/stackblitz-starters-fcaiuc?file=src%2Fmain.ts). Detecting swallowed errors can be very difficult, so I don't think that CS.effect should work like that.

We can fix this in the following way:

private readonly errorHandler = inject(ErrorHandler);

// ...

// effect impl:
const origin$ = new Subject<ObservableType>();
generator(origin$ as OriginType)
  .pipe(
    // forward unhandled error to `ErrorHandler` before re-subscribe
    tap({ error: (error) => this.errorHandler.handleError(error) }),
    retry(),
    takeUntil(this.destroy$)
  )
  .subscribe();

However, this change would break many codebases, mostly CS tests that don't use TestBed.

I'm not sure if it's worth it. 🤷‍♂️

markostanimirovic avatar Jul 19 '23 22:07 markostanimirovic

The whole idea is to swallow the error. I can’t imagine why effect() of ComponentStore would expect an error - only if someone forgot to catch an error. Function, generated by effect(), should live as long as the component lives - this intent is written in the code.

e-oz avatar Jul 20 '23 00:07 e-oz

Ah, wait, I understood your point - you mean the debugging purpose.

What if errors will be swallowed only when isDevMode() returns false?

e-oz avatar Jul 20 '23 01:07 e-oz

Or this way:

tap({ error: (error) => isDevMode() && console?.error(error) }),

https://stackblitz.com/edit/stackblitz-starters-sjluyf?file=package.json,src%2Fmain.ts

e-oz avatar Jul 20 '23 01:07 e-oz