feat: adds datasource status to sdk-client #590
Conversation
| this.name = 'LaunchDarklyStreamingError'; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
For reviewers: These moved to datasource/errors.ts
| @@ -1,6 +1,7 @@ | |||
| import { LDLogger } from '@launchdarkly/js-sdk-common'; | |||
|
|
|||
| export type EventName = 'error' | 'change'; | |||
| // TODO: is one emitter with many event names what we want long term? | |||
| export type EventName = 'error' | 'change' | 'dataSourceStatus'; | |||
There was a problem hiding this comment.
For reviewers: Please provide thoughts.
There was a problem hiding this comment.
Yeah, this is intended to provide an EventTarget like interface to our objects to make them familiar. https://developer.mozilla.org/en-US/docs/Web/API/EventTarget
There was a problem hiding this comment.
For reviewers: Majority of content for handling in this file is from LDClientImpl
|
|
||
| handlePollingError(error: LDPollingError) { | ||
| this.statusManager.setError(error.kind, error.message, error.status, error.recoverable); | ||
| } |
There was a problem hiding this comment.
For reviewers: If there is a better way to handle the mapping of StreamingError.code and PollingError.status to status I can unify these functions.
| export type PollingErrorHandler = (err: LDPollingError) => void; | ||
| // TODO: revisit usage of internal and figure out best practice | ||
| const { DataSourceErrorKind, LDPollingError } = internal; | ||
| export type PollingErrorHandler = (err: internal.LDPollingError) => void; |
There was a problem hiding this comment.
For reviewers: Please provide thoughts on internal import usage here.
packages/shared/common/src/internal/datasource/DataSourceErrorKinds.ts
Outdated
Show resolved
Hide resolved
…nitializing can occur more than once
| @@ -172,64 +170,6 @@ export default class LDClientImpl implements LDClient { | |||
| return this.checkedContext; | |||
| } | |||
|
|
|||
| private createStreamListeners( | |||
There was a problem hiding this comment.
For reviewers: This was unused.
| /* eslint-disable max-classes-per-file */ | ||
| import { DataSourceErrorKind } from './DataSourceErrorKinds'; | ||
|
|
||
| export class LDFileDataSourceError extends Error { |
There was a problem hiding this comment.
Errors are not intended to be internal. In that the internal package should only be used by SDK implementations, but instance of LDPollingError theoretically is something you could do in an application. Maybe we don't want that, but this one is maybe breaking.
There was a problem hiding this comment.
Maybe not expected to be constructed outside though. That part is likely fine.
There was a problem hiding this comment.
Started fixing this, but running into some import/export configuration issue. Pushed most recent commit before having to leave for flight.
| @@ -256,8 +251,8 @@ describe('sdk-client storage', () => { | |||
| JSON.stringify(putResponse), | |||
| ); | |||
|
|
|||
| expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys); | |||
There was a problem hiding this comment.
I may need some explanation on these changes also, as the ordering of events mattered and that isn't tested anymore.
There was a problem hiding this comment.
I thought about that too. I can go through and get them all to be passing, but I don’t think there is any specification around when a change event should come out vs when a dataSourceStatus change should come out and that is leading to them all breaking as I added dataSourceStatus events.
Perhaps an improved way could be to filter to just “change” and then do positional checking on the filtered output.
| @@ -30,4 +31,4 @@ export { DataSourcePaths } from './streaming'; | |||
| export { BaseDataManager } from './DataManager'; | |||
| export { Requestor }; | |||
|
|
|||
| export { LDClientImpl, LDClientInternalOptions }; | |||
| export { DataSourceStatus, LDClientImpl, LDClientInternalOptions }; | |||
There was a problem hiding this comment.
Need to add DataSourceState, DataSourceStatusErrorInfo, and DataSourceErrorKind
Requirements
Related issues
SDK-170
Describe the solution you've provided
Adds DataSourceStatusManager.
Refactors data source errors into common.
Adds DataSourceErrorKind to classify errors so manager can track state.