Skip to content

Commit

Permalink
feat: Correct client evaluation typings. (#554)
Browse files Browse the repository at this point in the history
Some code was refactored to make server and client share evaluation
results. This should not have been done because the results are not the
same in client and server SDKs. Server SDKs can always produce a reason
and client SDKs cannot.

This meant that the typing said that reason was required in the client
SDK, but it could be null.

This is a 'feat' to ensure a minor version bump in case of minor
incompatibilities.
  • Loading branch information
kinyoklion authored Aug 28, 2024
1 parent 115bd82 commit 64ab88d
Show file tree
Hide file tree
Showing 18 changed files with 109 additions and 191 deletions.
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.

/**
* An object that combines the result of a feature flag evaluation with information about
* how it was calculated.
Expand Down
18 changes: 0 additions & 18 deletions packages/shared/common/src/internal/evaluation/evaluationDetail.ts

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 {
if (!this.uncheckedContext) {
this.logger.debug(ClientMessages.missingContextKeyNoEvent);
return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue);
Expand Down
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.

/**
* 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'> & {
/**
* 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

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 };
12 changes: 0 additions & 12 deletions packages/shared/sdk-client/src/configuration/Configuration.test.ts
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
2 changes: 0 additions & 2 deletions packages/shared/sdk-client/src/configuration/Configuration.ts
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

0 comments on commit 64ab88d

Please sign in to comment.