-
Notifications
You must be signed in to change notification settings - Fork 63
Feat/lw 11939 improve error reporting on providers #1541
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
Feat/lw 11939 improve error reporting on providers #1541
Conversation
|
754c595 to
dda9794
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @AngelCastilloB
| if (error instanceof InvalidStringError) { | ||
| onFatalError?.(error); | ||
| cancelOnFatalError$.next(true); | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can drop the cancelOnFatalError$ observable. It will cancel anyway because we return false, which will cause the retry mechanism to throw.
Not a good idea. It will make the coldObservable throw an error instead of just completing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Please update commit message and PR title to indicate that this is only about logging before merging
- coldObservableProvider constructor now requires a logger object
867945d to
b2caa15
Compare
Context
Currently coldObservableProvider stays in an infinity loop retrying when there are provider errors. Errors are also not logged.
Proposed Solution
coldObservableProvider now stops retrying after maxRetries has been reached (max 10), and logs errors.