Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resubscribe on Error should work multiple times #2023

Closed
peterreisz opened this issue Jul 19, 2019 · 5 comments · Fixed by #2026 or TypescriptID/platform#244
Closed

Resubscribe on Error should work multiple times #2023

peterreisz opened this issue Jul 19, 2019 · 5 comments · Fixed by #2026 or TypescriptID/platform#244

Comments

@peterreisz
Copy link
Contributor

Minimal reproduction of the bug/regression with instructions:

$myEffect = createEffect(() =>
  this.actions$.pipe(
    ofType('MY_TYPE'),
    tap(() => console.log('I am here')),
    switchMap(() => throwError('error'))
  )
)

The console log shows only twice.

On the first error it switches to a new observable, but without any safeguard for the next error.
https://github.com/ngrx/platform/blob/master/modules/effects/src/effects_resolver.ts#L32

Expected behavior:

The resubscribe should work multiple times in case error happens in the stream
The error handling should be in a recursive function, something like this:

const handleErrors = obs$ => obs$.pipe(catchError(error => {
  if (errorHandler) errorHandler.handleError(error);
  // Return observable that produces this particular effect
  return handleErrors(obs$);
}));

const resubscribable$ = resubscribeOnError ? handleErrors(observable$) : observable$;

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

8.1.0

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

[X] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@timdeschryver
Copy link
Member

timdeschryver commented Jul 20, 2019

Good catch, do you want to create a PR for this @peterreisz ?

@peterreisz
Copy link
Contributor Author

Yes, done.

@alex-okrushko
Copy link
Member

Thanks for discovering and fixing the issue @peterreisz ! 👍

@peterreisz
Copy link
Contributor Author

peterreisz commented Oct 17, 2019

Thanks for discovering and fixing the issue @peterreisz !

@alex-okrushko You're welcome, but be aware the fix was removed because of some ci issue. It will be merged in #2165

@alex-okrushko
Copy link
Member

@peterreisz yes, I'm aware 😃

jordanpowell88 pushed a commit to jordanpowell88/platform that referenced this issue Nov 14, 2019
jordanpowell88 pushed a commit to jordanpowell88/platform that referenced this issue Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants