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 all commits
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 };
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ import {
setupMockStreamingProcessor,
} from '@launchdarkly/private-js-mocks';

import LDEmitter from './api/LDEmitter';
import { toMulti } from './context/addAutoEnv';
import * as mockResponseJson from './evaluation/mockResponse.json';
import LDClientImpl from './LDClientImpl';
import LDEmitter from './LDEmitter';
import { DeleteFlag, Flags, PatchFlag } from './types';

let mockPlatform: ReturnType<typeof createBasicPlatform>;
Expand Down
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 @@ import {
internal,
LDClientError,
LDContext,
LDEvaluationDetail,
LDEvaluationDetailTyped,
LDFlagSet,
LDFlagValue,
LDLogger,
Expand All @@ -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;
Expand Down Expand Up @@ -483,7 +485,7 @@ export default class LDClientImpl implements LDClient {
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
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
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 Expand Up @@ -80,17 +79,6 @@ describe('Configuration', () => {
);
});

test('invalid bootstrap should use default', () => {
// @ts-ignore
const config = new Configuration({ bootstrap: 'localStora' });

expect(config.bootstrap).toBeUndefined();
expect(console.error).toHaveBeenNthCalledWith(
1,
expect.stringMatching(/should be of type LDFlagSet, got string/i),
);
});

test('recognize maxCachedContexts', () => {
const config = new Configuration({ maxCachedContexts: 3 });

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
24 changes: 1 addition & 23 deletions packages/shared/sdk-client/src/configuration/validators.ts
Original file line number Diff line number Diff line change
@@ -1,18 +1,7 @@
// eslint-disable-next-line max-classes-per-file
import { noop, TypeValidator, TypeValidators } from '@launchdarkly/js-sdk-common';
import { TypeValidator, TypeValidators } from '@launchdarkly/js-sdk-common';

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

class BootStrapValidator implements TypeValidator {
is(u: unknown): boolean {
return typeof u === 'object' || typeof u === 'undefined' || u === null;
}

getType(): string {
return `LDFlagSet`;
}
}

class ConnectionModeValidator implements TypeValidator {
is(u: unknown): boolean {
Expand Down Expand Up @@ -46,22 +35,11 @@ const validators: Record<keyof LDOptions, TypeValidator> = {

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;
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ const createDiagnosticsInitConfig = (config: Configuration): DiagnosticsInitConf
reconnectTimeMillis: secondsToMillis(config.streamInitialReconnectDelay),
diagnosticRecordingIntervalMillis: secondsToMillis(config.diagnosticRecordingInterval),
allAttributesPrivate: config.allAttributesPrivate,
usingSecureMode: !!config.hash,
bootstrapMode: !!config.bootstrap,
// TODO: Implement when corresponding features are implemented.
usingSecureMode: false,
bootstrapMode: false,
});

export default createDiagnosticsInitConfig;
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,
};
}
Loading