-
Notifications
You must be signed in to change notification settings - Fork 20
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: Correct client evaluation typings. #554
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,4 @@ | ||
import ErrorKinds from './ErrorKinds'; | ||
import { createErrorEvaluationDetail, createSuccessEvaluationDetail } from './evaluationDetail'; | ||
import EventFactoryBase, { EvalEventArgs } from './EventFactoryBase'; | ||
|
||
export { | ||
createSuccessEvaluationDetail, | ||
createErrorEvaluationDetail, | ||
ErrorKinds, | ||
EvalEventArgs, | ||
EventFactoryBase, | ||
}; | ||
export { ErrorKinds, EvalEventArgs, EventFactoryBase }; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,6 @@ import { | |
internal, | ||
LDClientError, | ||
LDContext, | ||
LDEvaluationDetail, | ||
LDEvaluationDetailTyped, | ||
LDFlagSet, | ||
LDFlagValue, | ||
LDLogger, | ||
|
@@ -20,21 +18,25 @@ import { | |
import { LDStreamProcessor } from '@launchdarkly/js-sdk-common/dist/api/subsystem'; | ||
|
||
import { ConnectionMode, LDClient, type LDOptions } from './api'; | ||
import LDEmitter, { EventName } from './api/LDEmitter'; | ||
import { LDEvaluationDetail, LDEvaluationDetailTyped } from './api/LDEvaluationDetail'; | ||
import { LDIdentifyOptions } from './api/LDIdentifyOptions'; | ||
import Configuration from './configuration'; | ||
import { addAutoEnv } from './context/addAutoEnv'; | ||
import { ensureKey } from './context/ensureKey'; | ||
import createDiagnosticsManager from './diagnostics/createDiagnosticsManager'; | ||
import { | ||
createErrorEvaluationDetail, | ||
createSuccessEvaluationDetail, | ||
} from './evaluation/evaluationDetail'; | ||
import createEventProcessor from './events/createEventProcessor'; | ||
import EventFactory from './events/EventFactory'; | ||
import FlagManager from './flag-manager/FlagManager'; | ||
import { ItemDescriptor } from './flag-manager/ItemDescriptor'; | ||
import LDEmitter, { EventName } from './LDEmitter'; | ||
import PollingProcessor from './polling/PollingProcessor'; | ||
import { DeleteFlag, Flags, PatchFlag } from './types'; | ||
|
||
const { createErrorEvaluationDetail, createSuccessEvaluationDetail, ClientMessages, ErrorKinds } = | ||
internal; | ||
const { ClientMessages, ErrorKinds } = internal; | ||
|
||
export default class LDClientImpl implements LDClient { | ||
private readonly config: Configuration; | ||
|
@@ -483,7 +485,7 @@ export default class LDClientImpl implements LDClient { | |
defaultValue: any, | ||
eventFactory: EventFactory, | ||
typeChecker?: (value: any) => [boolean, string], | ||
): LDFlagValue { | ||
): LDEvaluationDetail { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This type was wrong, basically it was |
||
if (!this.uncheckedContext) { | ||
this.logger.debug(ClientMessages.missingContextKeyNoEvent); | ||
return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import { | ||
LDEvaluationDetail as CommonDetail, | ||
LDEvaluationDetailTyped as CommonDetailTyped, | ||
LDEvaluationReason, | ||
} from '@launchdarkly/js-sdk-common'; | ||
|
||
// Implementation note: In client-side SDKs the reason is optional. The common type, which is also | ||
// used by server SDKs, has a required reason. This file contains a client specific | ||
// LDEvaluationDetail which has an optional reason. | ||
|
||
// TODO: On major version change "reason" to be optional instead of nullable. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reason should have been optional from day 1, but it was not. Second it should not be null, but that is what the code was actually returning, so it will continue to do so. The null fields were only for compatibility with the pre-typescript node API. |
||
|
||
/** | ||
* An object that combines the result of a feature flag evaluation with information about | ||
* how it was calculated. | ||
* | ||
* This is the result of calling `LDClient.variationDetail`. | ||
*/ | ||
export type LDEvaluationDetail = Omit<CommonDetail, 'reason'> & { | ||
/** | ||
* An optional object describing the main factor that influenced the flag evaluation value. | ||
*/ | ||
reason: LDEvaluationReason | null; | ||
}; | ||
|
||
/** | ||
* An object that combines the result of a feature flag evaluation with information about | ||
* how it was calculated. | ||
* | ||
* This is the result of calling detailed variation methods. | ||
*/ | ||
export type LDEvaluationDetailTyped<TFlag> = Omit<CommonDetailTyped<TFlag>, 'reason'> & { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am exporting a new type with the same name as the original type. So I need to import the original type under a different name, otherwise it will conflict. |
||
/** | ||
* An optional object describing the main factor that influenced the flag evaluation value. | ||
*/ | ||
reason: LDEvaluationReason | null; | ||
}; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not consumable publicly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was exported, but it wasn't in the options and it wasn't used. So, another technically breaking change, but unrelated to any usable code. We may not actually implement inspectors at all, as hooks have now replaced them. |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { internal, LDEvaluationReason, LDFlagValue } from '@launchdarkly/js-sdk-common'; | ||
|
||
import { LDEvaluationDetail } from '../api'; | ||
|
||
export function createErrorEvaluationDetail( | ||
errorKind: internal.ErrorKinds, | ||
def?: LDFlagValue, | ||
): LDEvaluationDetail { | ||
return { | ||
value: def ?? null, | ||
variationIndex: null, | ||
reason: { kind: 'ERROR', errorKind }, | ||
}; | ||
} | ||
|
||
export function createSuccessEvaluationDetail( | ||
value: LDFlagValue, | ||
variationIndex?: number, | ||
reason?: LDEvaluationReason, | ||
): LDEvaluationDetail { | ||
return { | ||
value, | ||
variationIndex: variationIndex ?? null, | ||
reason: reason ?? 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.
The variationIndex can be null because we maintained the 7.0 API when we released 8. We should change it to just be optional.