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

feat: Correct client evaluation typings. #554

Merged
merged 4 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/shared/common/src/api/data/LDEvaluationDetail.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { LDEvaluationReason } from './LDEvaluationReason';
import { LDFlagValue } from './LDFlagValue';

// TODO: On major version change "variationIndex" to only be optional and not nullable.
Copy link
Member Author

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.


/**
* An object that combines the result of a feature flag evaluation with information about
* how it was calculated.
Expand Down

This file was deleted.

9 changes: 1 addition & 8 deletions packages/shared/common/src/internal/evaluation/index.ts
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 };
14 changes: 8 additions & 6 deletions packages/shared/sdk-client/src/LDClientImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
internal,
LDClientError,
LDContext,
LDEvaluationDetail,
LDEvaluationDetailTyped,
LDFlagSet,
LDFlagValue,
LDLogger,
Expand All @@ -20,21 +18,25 @@
import { LDStreamProcessor } from '@launchdarkly/js-sdk-common/dist/api/subsystem';

import { ConnectionMode, LDClient, type LDOptions } from './api';
import LDEmitter, { EventName } from './api/LDEmitter';
import LDEmitter, { EventName } from './LDEmitter';

Check failure on line 21 in packages/shared/sdk-client/src/LDClientImpl.ts

View workflow job for this annotation

GitHub Actions / build-test-sdk-client

Delete `LDEmitter,·{·EventName·}·from·'./LDEmitter';⏎import·`
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';

Check failure on line 35 in packages/shared/sdk-client/src/LDClientImpl.ts

View workflow job for this annotation

GitHub Actions / build-test-sdk-client

Insert `r';⏎import·LDEmitter,·{·EventName·}·from·'./LDEmitte`
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;
Expand Down Expand Up @@ -483,7 +485,7 @@
defaultValue: any,
eventFactory: EventFactory,
typeChecker?: (value: any) => [boolean, string],
): LDFlagValue {
): LDEvaluationDetail {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type was wrong, basically it was any which means that the type checker could not reveal the problem.

if (!this.uncheckedContext) {
this.logger.debug(ClientMessages.missingContextKeyNoEvent);
return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue);
Expand Down
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is not used outside of the common client code, and therefore is not part of the API.

File renamed without changes.
10 changes: 2 additions & 8 deletions packages/shared/sdk-client/src/api/LDClient.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
import {
LDContext,
LDEvaluationDetail,
LDEvaluationDetailTyped,
LDFlagSet,
LDFlagValue,
LDLogger,
} from '@launchdarkly/js-sdk-common';
import { LDContext, LDFlagSet, LDFlagValue, LDLogger } from '@launchdarkly/js-sdk-common';

import ConnectionMode from './ConnectionMode';
import { LDEvaluationDetail, LDEvaluationDetailTyped } from './LDEvaluationDetail';
import { LDIdentifyOptions } from './LDIdentifyOptions';

/**
Expand Down
37 changes: 37 additions & 0 deletions packages/shared/sdk-client/src/api/LDEvaluationDetail.ts
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.
Copy link
Member Author

Choose a reason for hiding this comment

The 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'> & {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the LDEvaluationDetailTyped as CommonDetailTyped required to make the type checking / compilation happy?

Copy link
Member Author

Choose a reason for hiding this comment

The 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;
};
105 changes: 0 additions & 105 deletions packages/shared/sdk-client/src/api/LDInspection.ts
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inspectors were not implemented, and this is not in the interface, so remove it until we implement them. (Unless we only implement hooks.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not consumable publicly?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

1 change: 1 addition & 0 deletions packages/shared/sdk-client/src/api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@ import ConnectionMode from './ConnectionMode';

export * from './LDOptions';
export * from './LDClient';
export * from './LDEvaluationDetail';

export { ConnectionMode };
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ describe('Configuration', () => {
withReasons: false,
eventsUri: 'https://events.launchdarkly.com',
flushInterval: 30,
inspectors: [],
logger: {
destination: console.error,
logLevel: 1,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
} from '@launchdarkly/js-sdk-common';

import { ConnectionMode, type LDOptions } from '../api';
import { LDInspection } from '../api/LDInspection';
import validators from './validators';

const DEFAULT_POLLING_INTERVAL: number = 60 * 5;
Expand Down Expand Up @@ -41,7 +40,6 @@ export default class Configuration {
public readonly useReport = false;
public readonly withReasons = false;

public readonly inspectors: LDInspection[] = [];
public readonly privateAttributes: string[] = [];

public readonly initialConnectionMode: ConnectionMode = 'streaming';
Expand Down
12 changes: 0 additions & 12 deletions packages/shared/sdk-client/src/configuration/validators.ts
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed unimplemented fields.

Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
// eslint-disable-next-line max-classes-per-file
import { noop, TypeValidator, TypeValidators } from '@launchdarkly/js-sdk-common';

Check failure on line 2 in packages/shared/sdk-client/src/configuration/validators.ts

View workflow job for this annotation

GitHub Actions / build-test-sdk-client

'noop' is defined but never used. Allowed unused vars must match /^__/u

import { type LDOptions } from '../api';
import { LDInspection } from '../api/LDInspection';

class BootStrapValidator implements TypeValidator {

Check failure on line 6 in packages/shared/sdk-client/src/configuration/validators.ts

View workflow job for this annotation

GitHub Actions / build-test-sdk-client

'BootStrapValidator' is defined but never used. Allowed unused vars must match /^__/u
is(u: unknown): boolean {
return typeof u === 'object' || typeof u === 'undefined' || u === null;
}
Expand Down Expand Up @@ -46,22 +45,11 @@

pollInterval: TypeValidators.numberWithMin(30),

// TODO: inspectors
// @ts-ignore
inspectors: TypeValidators.createTypeArray<LDInspection>('LDInspection[]', {
type: 'flag-used',
method: noop,
name: '',
}),
privateAttributes: TypeValidators.StringArray,

applicationInfo: TypeValidators.Object,
// TODO: bootstrap
bootstrap: new BootStrapValidator(),
wrapperName: TypeValidators.String,
wrapperVersion: TypeValidators.String,
// TODO: hash
hash: TypeValidators.String,
};

export default validators;
26 changes: 26 additions & 0 deletions packages/shared/sdk-client/src/evaluation/evaluationDetail.ts
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,
};
}
15 changes: 13 additions & 2 deletions packages/shared/sdk-client/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
import LDClientImpl from './LDClientImpl';

export * from '@launchdarkly/js-sdk-common';

export * as platform from '@launchdarkly/js-sdk-common';
export * from './api';

export * from '@launchdarkly/js-sdk-common';
// To replace the exports from `export *` we need to name them.
// So the below exports replace them with the Node specific variants.

// These exports are explicit to override those from common.
export type {
LDEvaluationDetail,
LDEvaluationDetailTyped,
LDClient,
LDOptions,
ConnectionMode,
} from './api';

export { LDClientImpl };
18 changes: 14 additions & 4 deletions packages/shared/sdk-server/src/evaluation/EvalResult.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { internal, LDEvaluationDetail, LDEvaluationReason } from '@launchdarkly/

import Reasons from './Reasons';

const { createErrorEvaluationDetail, createSuccessEvaluationDetail } = internal;
/**
* A class which encapsulates the result of an evaluation. It allows for differentiating between
* successful and error result types.
Expand Down Expand Up @@ -31,11 +30,22 @@ export default class EvalResult {
}

static forError(errorKind: internal.ErrorKinds, message?: string, def?: any): EvalResult {
return new EvalResult(true, createErrorEvaluationDetail(errorKind, def), message);
return new EvalResult(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed this code back to the way it was before the refactoring.

true,
{
value: def ?? null,
variationIndex: null,
reason: { kind: 'ERROR', errorKind },
},
message,
);
}

static forSuccess(value: any, reason: LDEvaluationReason, variationIndex?: number) {
const successDetail = createSuccessEvaluationDetail(value, variationIndex, reason);
return new EvalResult(false, successDetail as LDEvaluationDetail);
return new EvalResult(false, {
value,
variationIndex: variationIndex === undefined ? null : variationIndex,
reason,
});
}
}
Loading