-
Notifications
You must be signed in to change notification settings - Fork 46
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: fire event when error is present and http call succeeds #185
feat: fire event when error is present and http call succeeds #185
Conversation
src/index.ts
Outdated
@@ -177,11 +179,13 @@ export class UnleashClient extends TinyEmitter { | |||
this.refreshInterval = disableRefresh ? 0 : refreshInterval * 1000; | |||
this.context = { appName, environment, ...context }; | |||
this.usePOSTrequests = usePOSTrequests; | |||
this.sdkError = null; |
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.
what is we had a union with a current SDK state to avoid nulls: type SdkState = 'initializing' | 'healthy' | 'error' or similar?
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 like it and for the event name, may I suggest RECOVERED
or RECOVERED_FROM_ERROR
?
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.
Great suggestions. I will implement in the morning.
@@ -81,6 +82,8 @@ const defaultVariant: IVariant = { | |||
}; | |||
const storeKey = 'repo'; | |||
|
|||
type SdkState = 'initializing' | 'healthy' | 'error'; |
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.
Just another small detail: If I see my SDK state is error, should I trust its results? Maybe stale
is more accurate to how our SDK behaves: if some request fails, we still respond correctly with potentially stale data.
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.
It's a private property though, so we only use it internally in the SDK to discern whether or not we should emit the recovered event. In this case I think it's actually more accurate for the reader to use 'error' and it's not exposed anywhere outside of the internals of the SDK.
This PR adds a new event that fires when an error was recorded and the subsequent HTTP call is successful.
Why
Our React Proxy SDK will set an error based on the error event fired from this SDK when the HTTP call fails. Without this event we have no way to effectively reset this error, because the only event that is fired consistently is the UPDATE event, which is only fired when the response is not 304. This does not cover the case when you intermittently lose connection and the next call succeeds with 304.
How
We keep track of the sdkState internally in the SDK. When the HTTP calls error we change the state of the SDK to be in an error state. If the internal SDK state is set to error, and the HTTP call is successful we will emit a RECOVERED event that will allow subscribers to clear out stale errors and set the sdkState back to 'healthy'.