From 960da8adb32b6019ca3270c80a7d3ab203ed7f69 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Thu, 4 Apr 2024 16:41:26 -0700 Subject: [PATCH 01/14] fix: Initial attempt to fix wrongful rejection from variation error. --- .../shared/common/src/utils/timedPromise.ts | 6 +- .../shared/mocks/src/streamingProcessor.ts | 5 +- .../src/LDClientImpl.variation.test.ts | 106 ++++++++++++++++++ 3 files changed, 111 insertions(+), 6 deletions(-) create mode 100644 packages/shared/sdk-client/src/LDClientImpl.variation.test.ts diff --git a/packages/shared/common/src/utils/timedPromise.ts b/packages/shared/common/src/utils/timedPromise.ts index 6aa101d3bc..420bf95516 100644 --- a/packages/shared/common/src/utils/timedPromise.ts +++ b/packages/shared/common/src/utils/timedPromise.ts @@ -1,17 +1,13 @@ -import { LDLogger } from '../api'; - /** * Returns a promise which errors after t seconds. * * @param t Timeout in seconds. * @param taskName Name of task being timed for logging and error reporting. - * @param logger {@link LDLogger} object. */ -const timedPromise = (t: number, taskName: string, logger?: LDLogger) => +const timedPromise = (t: number, taskName: string) => new Promise((_res, reject) => { setTimeout(() => { const e = `${taskName} timed out after ${t} seconds.`; - logger?.error(e); reject(new Error(e)); }, t * 1000); }); diff --git a/packages/shared/mocks/src/streamingProcessor.ts b/packages/shared/mocks/src/streamingProcessor.ts index 87ba783cd2..f077cf8d0d 100644 --- a/packages/shared/mocks/src/streamingProcessor.ts +++ b/packages/shared/mocks/src/streamingProcessor.ts @@ -14,6 +14,7 @@ export const setupMockStreamingProcessor = ( patchResponseJson?: any, deleteResponseJson?: any, errorTimeoutSeconds: number = 0, + shouldPut: boolean = true, ) => { MockStreamingProcessor.mockImplementation( ( @@ -37,7 +38,9 @@ export const setupMockStreamingProcessor = ( }, errorTimeoutSeconds * 1000); } else { // execute put which will resolve the identify promise - setTimeout(() => listeners.get('put')?.processJson(putResponseJson)); + if (shouldPut) { + setTimeout(() => listeners.get('put')?.processJson(putResponseJson)); + } if (patchResponseJson) { setTimeout(() => listeners.get('patch')?.processJson(patchResponseJson)); diff --git a/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts b/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts new file mode 100644 index 0000000000..3420e75e95 --- /dev/null +++ b/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts @@ -0,0 +1,106 @@ +import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common'; +import { + basicPlatform, + hasher, + logger, + setupMockStreamingProcessor, +} from '@launchdarkly/private-js-mocks'; + +import LDEmitter from './api/LDEmitter'; +import * as mockResponseJson from './evaluation/mockResponse.json'; +import LDClientImpl from './LDClientImpl'; +import { Flags } from './types'; + +import useFakeTimers = jest.useFakeTimers; + +jest.mock('@launchdarkly/js-sdk-common', () => { + const actual = jest.requireActual('@launchdarkly/js-sdk-common'); + const actualMock = jest.requireActual('@launchdarkly/private-js-mocks'); + return { + ...actual, + ...{ + internal: { + ...actual.internal, + StreamingProcessor: actualMock.MockStreamingProcessor, + }, + }, + }; +}); + +const testSdkKey = 'test-sdk-key'; +const context: LDContext = { kind: 'org', key: 'Testy Pizza' }; + +let ldc: LDClientImpl; +let emitter: LDEmitter; +let defaultPutResponse: Flags; + +describe('sdk-client object', () => { + beforeEach(() => { + defaultPutResponse = clone(mockResponseJson); + // setupMockStreamingProcessor(false, defaultPutResponse); + basicPlatform.crypto.randomUUID.mockReturnValue('random1'); + hasher.digest.mockReturnValue('digested1'); + + ldc = new LDClientImpl(testSdkKey, AutoEnvAttributes.Enabled, basicPlatform, { + logger, + sendEvents: false, + }); + jest + .spyOn(LDClientImpl.prototype as any, 'createStreamUriPath') + .mockReturnValue('/stream/path'); + // @ts-ignore + emitter = ldc.emitter; + jest.spyOn(emitter as LDEmitter, 'emit'); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + test('variation', async () => { + await ldc.identify(context); + const devTestFlag = ldc.variation('dev-test-flag'); + + expect(devTestFlag).toBe(true); + }); + + test.only('variation flag not found', async () => { + setupMockStreamingProcessor(false, defaultPutResponse, undefined, undefined, 0, false); + // HACK: set context manually to pass validation + ldc.context = { kind: 'user', key: 'test-old-user' }; + + const p = ldc.identify(context); + + // GOTCHA: give ldc a chance to hook up with emitter. + setTimeout(() => { + ldc.variation('does-not-exist', 'not-found'); + }); + + // TODO: this should pass + await expect(p).resolves.toBeUndefined(); + }); + + test('variationDetail flag not found', async () => { + await ldc.identify(context); + const flag = ldc.variationDetail('does-not-exist', 'not-found'); + + expect(flag).toEqual({ + reason: { errorKind: 'FLAG_NOT_FOUND', kind: 'ERROR' }, + value: 'not-found', + variationIndex: null, + }); + }); + + test('variationDetail deleted flag not found', async () => { + await ldc.identify(context); + // @ts-ignore + ldc.flags['dev-test-flag'].deleted = true; + const flag = ldc.variationDetail('dev-test-flag', 'deleted'); + + expect(flag).toEqual({ + reason: { errorKind: 'FLAG_NOT_FOUND', kind: 'ERROR' }, + value: 'deleted', + variationIndex: null, + }); + }); +}); From 1e082f6e97f0f276b0b35502e4489aab08a3c7ed Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 00:05:06 -0700 Subject: [PATCH 02/14] fix: Listen to the correct error type in identify. Added IdentifyError and TimeoutError. Added LDStreamingError.eventName. --- packages/shared/common/src/errors.ts | 13 ++++++- .../src/internal/stream/StreamingProcessor.ts | 4 +- .../shared/common/src/utils/timedPromise.ts | 4 +- .../shared/mocks/src/streamingProcessor.ts | 6 +-- .../sdk-client/src/LDClientImpl.test.ts | 38 +------------------ .../shared/sdk-client/src/LDClientImpl.ts | 37 +++++++++++------- .../src/LDClientImpl.variation.test.ts | 35 ++++++----------- .../shared/sdk-client/src/types/errors.ts | 7 ++++ packages/shared/sdk-client/src/types/flags.ts | 23 +++++++++++ packages/shared/sdk-client/src/types/index.ts | 25 +----------- 10 files changed, 89 insertions(+), 103 deletions(-) create mode 100644 packages/shared/sdk-client/src/types/errors.ts create mode 100644 packages/shared/sdk-client/src/types/flags.ts diff --git a/packages/shared/common/src/errors.ts b/packages/shared/common/src/errors.ts index 600d0f03e8..1cf2d1dad9 100644 --- a/packages/shared/common/src/errors.ts +++ b/packages/shared/common/src/errors.ts @@ -1,6 +1,8 @@ // These classes are of trivial complexity. If they become // more complex, then they could be independent files. + /* eslint-disable max-classes-per-file */ +import { EventName } from './api'; export class LDFileDataSourceError extends Error { constructor(message: string) { @@ -21,10 +23,12 @@ export class LDPollingError extends Error { export class LDStreamingError extends Error { public readonly code?: number; + public readonly eventName?: EventName; - constructor(message: string, code?: number) { + constructor(message: string, code?: number, eventName?: EventName) { super(message); this.code = code; + this.eventName = eventName; this.name = 'LaunchDarklyStreamingError'; } } @@ -43,6 +47,13 @@ export class LDClientError extends Error { } } +export class TimeoutError extends Error { + constructor(message: string) { + super(message); + this.name = 'LaunchDarklyTimeoutError'; + } +} + /** * Check if the HTTP error is recoverable. This will return false if a request * made with any payload could not recover. If the reason for the failure diff --git a/packages/shared/common/src/internal/stream/StreamingProcessor.ts b/packages/shared/common/src/internal/stream/StreamingProcessor.ts index db67c4647c..5586cf03d0 100644 --- a/packages/shared/common/src/internal/stream/StreamingProcessor.ts +++ b/packages/shared/common/src/internal/stream/StreamingProcessor.ts @@ -135,7 +135,9 @@ class StreamingProcessor implements LDStreamProcessor { } processJson(dataJson); } else { - this.errorHandler?.(new LDStreamingError('Unexpected payload from event stream')); + this.errorHandler?.( + new LDStreamingError('Unexpected payload from event stream', undefined, eventName), + ); } }); }); diff --git a/packages/shared/common/src/utils/timedPromise.ts b/packages/shared/common/src/utils/timedPromise.ts index 420bf95516..2921f14ae0 100644 --- a/packages/shared/common/src/utils/timedPromise.ts +++ b/packages/shared/common/src/utils/timedPromise.ts @@ -1,3 +1,5 @@ +import { TimeoutError } from '../errors'; + /** * Returns a promise which errors after t seconds. * @@ -8,7 +10,7 @@ const timedPromise = (t: number, taskName: string) => new Promise((_res, reject) => { setTimeout(() => { const e = `${taskName} timed out after ${t} seconds.`; - reject(new Error(e)); + reject(new TimeoutError(e)); }, t * 1000); }); diff --git a/packages/shared/mocks/src/streamingProcessor.ts b/packages/shared/mocks/src/streamingProcessor.ts index f077cf8d0d..791c814915 100644 --- a/packages/shared/mocks/src/streamingProcessor.ts +++ b/packages/shared/mocks/src/streamingProcessor.ts @@ -14,7 +14,6 @@ export const setupMockStreamingProcessor = ( patchResponseJson?: any, deleteResponseJson?: any, errorTimeoutSeconds: number = 0, - shouldPut: boolean = true, ) => { MockStreamingProcessor.mockImplementation( ( @@ -33,14 +32,13 @@ export const setupMockStreamingProcessor = ( code: 401, name: 'LaunchDarklyStreamingError', message: 'test-error', + eventName: 'put', }; errorHandler(unauthorized); }, errorTimeoutSeconds * 1000); } else { // execute put which will resolve the identify promise - if (shouldPut) { - setTimeout(() => listeners.get('put')?.processJson(putResponseJson)); - } + setTimeout(() => listeners.get('put')?.processJson(putResponseJson)); if (patchResponseJson) { setTimeout(() => listeners.get('patch')?.processJson(patchResponseJson)); diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index 9c6416892c..e0277573ae 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -115,37 +115,6 @@ describe('sdk-client object', () => { }); }); - test('variation', async () => { - await ldc.identify(context); - const devTestFlag = ldc.variation('dev-test-flag'); - - expect(devTestFlag).toBe(true); - }); - - test('variationDetail flag not found', async () => { - await ldc.identify(context); - const flag = ldc.variationDetail('does-not-exist', 'not-found'); - - expect(flag).toEqual({ - reason: { errorKind: 'FLAG_NOT_FOUND', kind: 'ERROR' }, - value: 'not-found', - variationIndex: null, - }); - }); - - test('variationDetail deleted flag not found', async () => { - await ldc.identify(context); - // @ts-ignore - ldc.flags['dev-test-flag'].deleted = true; - const flag = ldc.variationDetail('dev-test-flag', 'deleted'); - - expect(flag).toEqual({ - reason: { errorKind: 'FLAG_NOT_FOUND', kind: 'ERROR' }, - value: 'deleted', - variationIndex: null, - }); - }); - test('identify success', async () => { defaultPutResponse['dev-test-flag'].value = false; const carContext: LDContext = { kind: 'car', key: 'test-car' }; @@ -240,11 +209,8 @@ describe('sdk-client object', () => { setupMockStreamingProcessor(true); const carContext: LDContext = { kind: 'car', key: 'test-car' }; - await expect(ldc.identify(carContext)).rejects.toMatchObject({ - code: 401, - message: 'test-error', - }); - expect(logger.error).toHaveBeenCalledTimes(1); + await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); + expect(logger.error).toHaveBeenCalledTimes(2); expect(ldc.getContext()).toBeUndefined(); }); diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 7aa983d3f5..c81d17ef03 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -25,7 +25,7 @@ import Configuration from './configuration'; import createDiagnosticsManager from './diagnostics/createDiagnosticsManager'; import createEventProcessor from './events/createEventProcessor'; import EventFactory from './events/EventFactory'; -import { DeleteFlag, Flags, PatchFlag } from './types'; +import { DeleteFlag, Flags, IdentifyError, PatchFlag } from './types'; import { addAutoEnv, calculateFlagChanges, ensureKey } from './utils'; const { createErrorEvaluationDetail, createSuccessEvaluationDetail, ClientMessages, ErrorKinds } = @@ -257,17 +257,22 @@ export default class LDClientImpl implements LDClient { this.identifyChangeListener = (c: LDContext, changedKeys: string[]) => { this.logger.debug(`change: context: ${JSON.stringify(c)}, flags: ${changedKeys}`); }; - this.identifyErrorListener = (c: LDContext, err: Error) => { - this.logger.debug(`error: ${err}, context: ${JSON.stringify(c)}`); - reject(err); + this.identifyErrorListener = (c: LDContext, err: IdentifyError) => { + if (err.name === 'IdentifyError') { + this.logger.debug(`identify error: ${err}, context: ${JSON.stringify(c)}`); + reject(err); + } }; this.emitter.on('change', this.identifyChangeListener); this.emitter.on('error', this.identifyErrorListener); }); - const timed = timedPromise(timeout, 'identify', this.logger); - const raced = Promise.race([timed, slow]); + const timed = timedPromise(timeout, 'identify'); + const raced = Promise.race([timed, slow]).catch((err) => { + this.logger.error(`identify race error: ${err}`); + throw err; + }); return { identifyPromise: raced, identifyResolve: res }; } @@ -302,8 +307,8 @@ export default class LDClientImpl implements LDClient { const checkedContext = Context.fromLDContext(context); if (!checkedContext.valid) { - const error = new Error('Context was unspecified or had no key'); - this.logger.error(error); + const error = new IdentifyError('Context was unspecified or had no key'); + this.logger.error(error.toString()); this.emitter.emit('error', context, error); return Promise.reject(error); } @@ -341,7 +346,13 @@ export default class LDClientImpl implements LDClient { this.diagnosticsManager, (e) => { this.logger.error(e); - this.emitter.emit('error', context, e); + + if (e.eventName === 'put') { + // alternatively identifyPromise reject here + this.emitter.emit('error', context, new IdentifyError(e.message)); + } else { + this.emitter.emit('error', context, e); + } }, ); this.streamer.start(); @@ -387,12 +398,12 @@ export default class LDClientImpl implements LDClient { track(key: string, data?: any, metricValue?: number): void { if (!this.context) { - this.logger?.warn(ClientMessages.missingContextKeyNoEvent); + this.logger.warn(ClientMessages.missingContextKeyNoEvent); return; } const checkedContext = Context.fromLDContext(this.context); if (!checkedContext.valid) { - this.logger?.warn(ClientMessages.missingContextKeyNoEvent); + this.logger.warn(ClientMessages.missingContextKeyNoEvent); return; } @@ -408,7 +419,7 @@ export default class LDClientImpl implements LDClient { typeChecker?: (value: any) => [boolean, string], ): LDFlagValue { if (!this.context) { - this.logger?.debug(ClientMessages.missingContextKeyNoEvent); + this.logger.debug(ClientMessages.missingContextKeyNoEvent); return createErrorEvaluationDetail(ErrorKinds.UserNotSpecified, defaultValue); } @@ -420,7 +431,7 @@ export default class LDClientImpl implements LDClient { const error = new LDClientError( `Unknown feature flag "${flagKey}"; returning default value ${defVal}.`, ); - this.logger.error(error); + this.logger.error(error.toString()); this.emitter.emit('error', this.context, error); this.eventProcessor?.sendEvent( this.eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext), diff --git a/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts b/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts index 3420e75e95..06ca449c0d 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.variation.test.ts @@ -1,18 +1,10 @@ import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common'; -import { - basicPlatform, - hasher, - logger, - setupMockStreamingProcessor, -} from '@launchdarkly/private-js-mocks'; +import { basicPlatform, logger, setupMockStreamingProcessor } from '@launchdarkly/private-js-mocks'; -import LDEmitter from './api/LDEmitter'; import * as mockResponseJson from './evaluation/mockResponse.json'; import LDClientImpl from './LDClientImpl'; import { Flags } from './types'; -import useFakeTimers = jest.useFakeTimers; - jest.mock('@launchdarkly/js-sdk-common', () => { const actual = jest.requireActual('@launchdarkly/js-sdk-common'); const actualMock = jest.requireActual('@launchdarkly/private-js-mocks'); @@ -31,16 +23,12 @@ const testSdkKey = 'test-sdk-key'; const context: LDContext = { kind: 'org', key: 'Testy Pizza' }; let ldc: LDClientImpl; -let emitter: LDEmitter; let defaultPutResponse: Flags; describe('sdk-client object', () => { beforeEach(() => { defaultPutResponse = clone(mockResponseJson); - // setupMockStreamingProcessor(false, defaultPutResponse); - basicPlatform.crypto.randomUUID.mockReturnValue('random1'); - hasher.digest.mockReturnValue('digested1'); - + setupMockStreamingProcessor(false, defaultPutResponse); ldc = new LDClientImpl(testSdkKey, AutoEnvAttributes.Enabled, basicPlatform, { logger, sendEvents: false, @@ -48,9 +36,6 @@ describe('sdk-client object', () => { jest .spyOn(LDClientImpl.prototype as any, 'createStreamUriPath') .mockReturnValue('/stream/path'); - // @ts-ignore - emitter = ldc.emitter; - jest.spyOn(emitter as LDEmitter, 'emit'); }); afterEach(() => { @@ -64,20 +49,22 @@ describe('sdk-client object', () => { expect(devTestFlag).toBe(true); }); - test.only('variation flag not found', async () => { - setupMockStreamingProcessor(false, defaultPutResponse, undefined, undefined, 0, false); - // HACK: set context manually to pass validation - ldc.context = { kind: 'user', key: 'test-old-user' }; + test('variation flag not found', async () => { + // set context manually to pass validation + ldc.context = { kind: 'user', key: 'test-user' }; + const errorListener = jest.fn().mockName('errorListener'); + ldc.on('error', errorListener); const p = ldc.identify(context); - - // GOTCHA: give ldc a chance to hook up with emitter. setTimeout(() => { + // call variation in the next tick to give ldc a chance to hook up event emitter ldc.variation('does-not-exist', 'not-found'); }); - // TODO: this should pass await expect(p).resolves.toBeUndefined(); + const error = errorListener.mock.calls[0][1]; + expect(errorListener).toHaveBeenCalledTimes(1); + expect(error.message).toMatch(/unknown feature/i); }); test('variationDetail flag not found', async () => { diff --git a/packages/shared/sdk-client/src/types/errors.ts b/packages/shared/sdk-client/src/types/errors.ts new file mode 100644 index 0000000000..b30f8c2556 --- /dev/null +++ b/packages/shared/sdk-client/src/types/errors.ts @@ -0,0 +1,7 @@ +// eslint-disable-next-line import/prefer-default-export +export class IdentifyError extends Error { + constructor(message: string) { + super(message); + this.name = 'IdentifyError'; + } +} diff --git a/packages/shared/sdk-client/src/types/flags.ts b/packages/shared/sdk-client/src/types/flags.ts new file mode 100644 index 0000000000..18b24736d6 --- /dev/null +++ b/packages/shared/sdk-client/src/types/flags.ts @@ -0,0 +1,23 @@ +import { LDEvaluationReason, LDFlagValue } from '@launchdarkly/js-sdk-common'; + +export interface Flag { + version: number; + flagVersion: number; + value: LDFlagValue; + variation: number; + trackEvents: boolean; + trackReason?: boolean; + reason?: LDEvaluationReason; + debugEventsUntilDate?: number; + deleted?: boolean; +} + +export interface PatchFlag extends Flag { + key: string; +} + +export type DeleteFlag = Pick; + +export type Flags = { + [k: string]: Flag; +}; diff --git a/packages/shared/sdk-client/src/types/index.ts b/packages/shared/sdk-client/src/types/index.ts index 18b24736d6..328e1135d1 100644 --- a/packages/shared/sdk-client/src/types/index.ts +++ b/packages/shared/sdk-client/src/types/index.ts @@ -1,23 +1,2 @@ -import { LDEvaluationReason, LDFlagValue } from '@launchdarkly/js-sdk-common'; - -export interface Flag { - version: number; - flagVersion: number; - value: LDFlagValue; - variation: number; - trackEvents: boolean; - trackReason?: boolean; - reason?: LDEvaluationReason; - debugEventsUntilDate?: number; - deleted?: boolean; -} - -export interface PatchFlag extends Flag { - key: string; -} - -export type DeleteFlag = Pick; - -export type Flags = { - [k: string]: Flag; -}; +export * from './errors'; +export * from './flags'; From eec70a5ef9aa94c3d86e4af4a0ae62c84ebf9709 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 16:59:53 -0700 Subject: [PATCH 03/14] fix: Simplified promise creation. Rollback previous unnecessary changes. --- packages/shared/common/src/errors.ts | 13 +-- .../src/internal/stream/StreamingProcessor.ts | 4 +- .../shared/common/src/utils/timedPromise.ts | 4 +- .../shared/mocks/src/streamingProcessor.ts | 1 - .../sdk-client/src/LDClientImpl.test.ts | 21 +++- .../shared/sdk-client/src/LDClientImpl.ts | 105 +++++++----------- .../shared/sdk-client/src/types/errors.ts | 7 -- packages/shared/sdk-client/src/types/flags.ts | 23 ---- packages/shared/sdk-client/src/types/index.ts | 25 ++++- 9 files changed, 84 insertions(+), 119 deletions(-) delete mode 100644 packages/shared/sdk-client/src/types/errors.ts delete mode 100644 packages/shared/sdk-client/src/types/flags.ts diff --git a/packages/shared/common/src/errors.ts b/packages/shared/common/src/errors.ts index 1cf2d1dad9..600d0f03e8 100644 --- a/packages/shared/common/src/errors.ts +++ b/packages/shared/common/src/errors.ts @@ -1,8 +1,6 @@ // These classes are of trivial complexity. If they become // more complex, then they could be independent files. - /* eslint-disable max-classes-per-file */ -import { EventName } from './api'; export class LDFileDataSourceError extends Error { constructor(message: string) { @@ -23,12 +21,10 @@ export class LDPollingError extends Error { export class LDStreamingError extends Error { public readonly code?: number; - public readonly eventName?: EventName; - constructor(message: string, code?: number, eventName?: EventName) { + constructor(message: string, code?: number) { super(message); this.code = code; - this.eventName = eventName; this.name = 'LaunchDarklyStreamingError'; } } @@ -47,13 +43,6 @@ export class LDClientError extends Error { } } -export class TimeoutError extends Error { - constructor(message: string) { - super(message); - this.name = 'LaunchDarklyTimeoutError'; - } -} - /** * Check if the HTTP error is recoverable. This will return false if a request * made with any payload could not recover. If the reason for the failure diff --git a/packages/shared/common/src/internal/stream/StreamingProcessor.ts b/packages/shared/common/src/internal/stream/StreamingProcessor.ts index 5586cf03d0..db67c4647c 100644 --- a/packages/shared/common/src/internal/stream/StreamingProcessor.ts +++ b/packages/shared/common/src/internal/stream/StreamingProcessor.ts @@ -135,9 +135,7 @@ class StreamingProcessor implements LDStreamProcessor { } processJson(dataJson); } else { - this.errorHandler?.( - new LDStreamingError('Unexpected payload from event stream', undefined, eventName), - ); + this.errorHandler?.(new LDStreamingError('Unexpected payload from event stream')); } }); }); diff --git a/packages/shared/common/src/utils/timedPromise.ts b/packages/shared/common/src/utils/timedPromise.ts index 2921f14ae0..420bf95516 100644 --- a/packages/shared/common/src/utils/timedPromise.ts +++ b/packages/shared/common/src/utils/timedPromise.ts @@ -1,5 +1,3 @@ -import { TimeoutError } from '../errors'; - /** * Returns a promise which errors after t seconds. * @@ -10,7 +8,7 @@ const timedPromise = (t: number, taskName: string) => new Promise((_res, reject) => { setTimeout(() => { const e = `${taskName} timed out after ${t} seconds.`; - reject(new TimeoutError(e)); + reject(new Error(e)); }, t * 1000); }); diff --git a/packages/shared/mocks/src/streamingProcessor.ts b/packages/shared/mocks/src/streamingProcessor.ts index 791c814915..87ba783cd2 100644 --- a/packages/shared/mocks/src/streamingProcessor.ts +++ b/packages/shared/mocks/src/streamingProcessor.ts @@ -32,7 +32,6 @@ export const setupMockStreamingProcessor = ( code: 401, name: 'LaunchDarklyStreamingError', message: 'test-error', - eventName: 'put', }; errorHandler(unauthorized); }, errorTimeoutSeconds * 1000); diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index e0277573ae..ddbec72b5b 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -10,6 +10,9 @@ import { import * as mockResponseJson from './evaluation/mockResponse.json'; import LDClientImpl from './LDClientImpl'; import { Flags } from './types'; +import { toMulti } from './utils/addAutoEnv'; + +import useFakeTimers = jest.useFakeTimers; jest.mock('@launchdarkly/js-sdk-common', () => { const actual = jest.requireActual('@launchdarkly/js-sdk-common'); @@ -47,6 +50,7 @@ let defaultPutResponse: Flags; describe('sdk-client object', () => { beforeEach(() => { + useFakeTimers(); defaultPutResponse = clone(mockResponseJson); setupMockStreamingProcessor(false, defaultPutResponse); basicPlatform.crypto.randomUUID.mockReturnValue('random1'); @@ -205,13 +209,22 @@ describe('sdk-client object', () => { expect(ldc.getContext()).toBeUndefined(); }); - test('identify error stream error', async () => { + test.only('identify error stream error', async () => { setupMockStreamingProcessor(true); const carContext: LDContext = { kind: 'car', key: 'test-car' }; - await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); - expect(logger.error).toHaveBeenCalledTimes(2); - expect(ldc.getContext()).toBeUndefined(); + // await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); + const p = ldc.identify(carContext); + await jest.runAllTimersAsync(); + // jest.runAllTimers(); + // jest.runOnlyPendingTimers(); + await expect(p).rejects.toEqual({ + code: 401, + message: 'test-error', + name: 'LaunchDarklyStreamingError', + }); + // expect(logger.error).toHaveBeenCalledTimes(2); + // expect(ldc.getContext()).toBeUndefined(); }); test('identify change and error listeners', async () => { diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index c81d17ef03..422f5d7646 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -25,7 +25,7 @@ import Configuration from './configuration'; import createDiagnosticsManager from './diagnostics/createDiagnosticsManager'; import createEventProcessor from './events/createEventProcessor'; import EventFactory from './events/EventFactory'; -import { DeleteFlag, Flags, IdentifyError, PatchFlag } from './types'; +import { DeleteFlag, Flags, PatchFlag } from './types'; import { addAutoEnv, calculateFlagChanges, ensureKey } from './utils'; const { createErrorEvaluationDetail, createSuccessEvaluationDetail, ClientMessages, ErrorKinds } = @@ -47,7 +47,6 @@ export default class LDClientImpl implements LDClient { private emitter: LDEmitter; private flags: Flags = {}; private identifyChangeListener?: (c: LDContext, changedKeys: string[]) => void; - private identifyErrorListener?: (c: LDContext, err: any) => void; private readonly clientContext: ClientContext; @@ -81,6 +80,12 @@ export default class LDClientImpl implements LDClient { !this.isOffline(), ); this.emitter = new LDEmitter(); + this.emitter.on('change', (c: LDContext, changedKeys: string[]) => { + this.logger.debug(`change: context: ${JSON.stringify(c)}, flags: ${changedKeys}`); + }); + this.emitter.on('error', (c: LDContext, err: any) => { + this.logger.error(`error: ${err}, context: ${JSON.stringify(c)}`); + }); } /** @@ -224,62 +229,18 @@ export default class LDClientImpl implements LDClient { return listeners; } - /** - * Generates the url path for streamer. - * - * For mobile key: /meval/${base64-encoded-context} - * For clientSideId: /eval/${envId}/${base64-encoded-context} - * - * the path. - * - * @protected This function must be overridden in subclasses for streamer - * to work. - * @param _context The LDContext object - */ - protected createStreamUriPath(_context: LDContext): string { - throw new Error( - 'createStreamUriPath not implemented. Client sdks must implement createStreamUriPath for streamer to work.', - ); - } - private createIdentifyPromise(timeout: number) { let res: any; + let rej: any; + const slow = new Promise((resolve, reject) => { res = resolve; - - if (this.identifyChangeListener) { - this.emitter.off('change', this.identifyChangeListener); - } - if (this.identifyErrorListener) { - this.emitter.off('error', this.identifyErrorListener); - } - - this.identifyChangeListener = (c: LDContext, changedKeys: string[]) => { - this.logger.debug(`change: context: ${JSON.stringify(c)}, flags: ${changedKeys}`); - }; - this.identifyErrorListener = (c: LDContext, err: IdentifyError) => { - if (err.name === 'IdentifyError') { - this.logger.debug(`identify error: ${err}, context: ${JSON.stringify(c)}`); - reject(err); - } - }; - - this.emitter.on('change', this.identifyChangeListener); - this.emitter.on('error', this.identifyErrorListener); + rej = reject; }); const timed = timedPromise(timeout, 'identify'); - const raced = Promise.race([timed, slow]).catch((err) => { - this.logger.error(`identify race error: ${err}`); - throw err; - }); - - return { identifyPromise: raced, identifyResolve: res }; - } - - private async getFlagsFromStorage(canonicalKey: string): Promise { - const f = await this.platform.storage?.get(canonicalKey); - return f ? JSON.parse(f) : undefined; + const raced = Promise.race([timed, slow]); + return { identifyPromise: raced, identifyResolve: res, identifyReject: rej }; } /** @@ -307,14 +268,15 @@ export default class LDClientImpl implements LDClient { const checkedContext = Context.fromLDContext(context); if (!checkedContext.valid) { - const error = new IdentifyError('Context was unspecified or had no key'); - this.logger.error(error.toString()); + const error = new Error('Context was unspecified or had no key'); this.emitter.emit('error', context, error); return Promise.reject(error); } this.eventProcessor?.sendEvent(this.eventFactoryDefault.identifyEvent(checkedContext)); - const { identifyPromise, identifyResolve } = this.createIdentifyPromise(this.identifyTimeout); + const { identifyPromise, identifyResolve, identifyReject } = this.createIdentifyPromise( + this.identifyTimeout, + ); this.logger.debug(`Identifying ${JSON.stringify(context)}`); const flagsStorage = await this.getFlagsFromStorage(checkedContext.canonicalKey); @@ -345,14 +307,8 @@ export default class LDClientImpl implements LDClient { this.createStreamListeners(context, checkedContext.canonicalKey, identifyResolve), this.diagnosticsManager, (e) => { - this.logger.error(e); - - if (e.eventName === 'put') { - // alternatively identifyPromise reject here - this.emitter.emit('error', context, new IdentifyError(e.message)); - } else { - this.emitter.emit('error', context, e); - } + identifyReject(e); + this.emitter.emit('error', context, e); }, ); this.streamer.start(); @@ -388,6 +344,29 @@ export default class LDClientImpl implements LDClient { } } + private async getFlagsFromStorage(canonicalKey: string): Promise { + const f = await this.platform.storage?.get(canonicalKey); + return f ? JSON.parse(f) : undefined; + } + + /** + * Generates the url path for streamer. + * + * For mobile key: /meval/${base64-encoded-context} + * For clientSideId: /eval/${envId}/${base64-encoded-context} + * + * the path. + * + * @protected This function must be overridden in subclasses for streamer + * to work. + * @param _context The LDContext object + */ + protected createStreamUriPath(_context: LDContext): string { + throw new Error( + 'createStreamUriPath not implemented. Client sdks must implement createStreamUriPath for streamer to work.', + ); + } + off(eventName: EventName, listener: Function): void { this.emitter.off(eventName, listener); } @@ -431,7 +410,6 @@ export default class LDClientImpl implements LDClient { const error = new LDClientError( `Unknown feature flag "${flagKey}"; returning default value ${defVal}.`, ); - this.logger.error(error.toString()); this.emitter.emit('error', this.context, error); this.eventProcessor?.sendEvent( this.eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext), @@ -457,7 +435,6 @@ export default class LDClientImpl implements LDClient { const error = new LDClientError( `Wrong type "${type}" for feature flag "${flagKey}"; returning default value`, ); - this.logger.error(error); this.emitter.emit('error', this.context, error); return createErrorEvaluationDetail(ErrorKinds.WrongType, defaultValue); } diff --git a/packages/shared/sdk-client/src/types/errors.ts b/packages/shared/sdk-client/src/types/errors.ts deleted file mode 100644 index b30f8c2556..0000000000 --- a/packages/shared/sdk-client/src/types/errors.ts +++ /dev/null @@ -1,7 +0,0 @@ -// eslint-disable-next-line import/prefer-default-export -export class IdentifyError extends Error { - constructor(message: string) { - super(message); - this.name = 'IdentifyError'; - } -} diff --git a/packages/shared/sdk-client/src/types/flags.ts b/packages/shared/sdk-client/src/types/flags.ts deleted file mode 100644 index 18b24736d6..0000000000 --- a/packages/shared/sdk-client/src/types/flags.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { LDEvaluationReason, LDFlagValue } from '@launchdarkly/js-sdk-common'; - -export interface Flag { - version: number; - flagVersion: number; - value: LDFlagValue; - variation: number; - trackEvents: boolean; - trackReason?: boolean; - reason?: LDEvaluationReason; - debugEventsUntilDate?: number; - deleted?: boolean; -} - -export interface PatchFlag extends Flag { - key: string; -} - -export type DeleteFlag = Pick; - -export type Flags = { - [k: string]: Flag; -}; diff --git a/packages/shared/sdk-client/src/types/index.ts b/packages/shared/sdk-client/src/types/index.ts index 328e1135d1..18b24736d6 100644 --- a/packages/shared/sdk-client/src/types/index.ts +++ b/packages/shared/sdk-client/src/types/index.ts @@ -1,2 +1,23 @@ -export * from './errors'; -export * from './flags'; +import { LDEvaluationReason, LDFlagValue } from '@launchdarkly/js-sdk-common'; + +export interface Flag { + version: number; + flagVersion: number; + value: LDFlagValue; + variation: number; + trackEvents: boolean; + trackReason?: boolean; + reason?: LDEvaluationReason; + debugEventsUntilDate?: number; + deleted?: boolean; +} + +export interface PatchFlag extends Flag { + key: string; +} + +export type DeleteFlag = Pick; + +export type Flags = { + [k: string]: Flag; +}; From 15b748827650427408860d6055965f4d452be68b Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:00:59 -0700 Subject: [PATCH 04/14] Update LDClientImpl.test.ts --- packages/shared/sdk-client/src/LDClientImpl.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index ddbec72b5b..f0fe388fb9 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -10,7 +10,6 @@ import { import * as mockResponseJson from './evaluation/mockResponse.json'; import LDClientImpl from './LDClientImpl'; import { Flags } from './types'; -import { toMulti } from './utils/addAutoEnv'; import useFakeTimers = jest.useFakeTimers; From 8edd0ba402af6c455795d2f2c0b7bc82caa156c9 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:11:44 -0700 Subject: [PATCH 05/14] fix: Broken identify stream put test. --- .../shared/sdk-client/src/LDClientImpl.test.ts | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index f0fe388fb9..eb7e2a0226 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -11,8 +11,6 @@ import * as mockResponseJson from './evaluation/mockResponse.json'; import LDClientImpl from './LDClientImpl'; import { Flags } from './types'; -import useFakeTimers = jest.useFakeTimers; - jest.mock('@launchdarkly/js-sdk-common', () => { const actual = jest.requireActual('@launchdarkly/js-sdk-common'); const actualMock = jest.requireActual('@launchdarkly/private-js-mocks'); @@ -49,7 +47,6 @@ let defaultPutResponse: Flags; describe('sdk-client object', () => { beforeEach(() => { - useFakeTimers(); defaultPutResponse = clone(mockResponseJson); setupMockStreamingProcessor(false, defaultPutResponse); basicPlatform.crypto.randomUUID.mockReturnValue('random1'); @@ -208,22 +205,14 @@ describe('sdk-client object', () => { expect(ldc.getContext()).toBeUndefined(); }); - test.only('identify error stream error', async () => { + test('identify error stream error', async () => { setupMockStreamingProcessor(true); const carContext: LDContext = { kind: 'car', key: 'test-car' }; - - // await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); - const p = ldc.identify(carContext); - await jest.runAllTimersAsync(); - // jest.runAllTimers(); - // jest.runOnlyPendingTimers(); - await expect(p).rejects.toEqual({ + await expect(ldc.identify(carContext)).rejects.toEqual({ code: 401, message: 'test-error', name: 'LaunchDarklyStreamingError', }); - // expect(logger.error).toHaveBeenCalledTimes(2); - // expect(ldc.getContext()).toBeUndefined(); }); test('identify change and error listeners', async () => { From 0f6ce38b6accb30c771fbf2f0f1b2e718a352c0f Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:28:04 -0700 Subject: [PATCH 06/14] fix: Correct mock streamer error type. --- packages/shared/mocks/src/streamingProcessor.ts | 8 +++----- packages/shared/sdk-client/src/LDClientImpl.test.ts | 6 +----- .../shared/sdk-client/src/LDClientImpl.timeout.test.ts | 1 - 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/packages/shared/mocks/src/streamingProcessor.ts b/packages/shared/mocks/src/streamingProcessor.ts index 87ba783cd2..260db1654e 100644 --- a/packages/shared/mocks/src/streamingProcessor.ts +++ b/packages/shared/mocks/src/streamingProcessor.ts @@ -28,11 +28,9 @@ export const setupMockStreamingProcessor = ( start: jest.fn(async () => { if (shouldError) { setTimeout(() => { - const unauthorized: LDStreamingError = { - code: 401, - name: 'LaunchDarklyStreamingError', - message: 'test-error', - }; + const unauthorized = new Error('test-error') as LDStreamingError; + // @ts-ignore + unauthorized.code = 401; errorHandler(unauthorized); }, errorTimeoutSeconds * 1000); } else { diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index eb7e2a0226..77a3aabffd 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -208,11 +208,7 @@ describe('sdk-client object', () => { test('identify error stream error', async () => { setupMockStreamingProcessor(true); const carContext: LDContext = { kind: 'car', key: 'test-car' }; - await expect(ldc.identify(carContext)).rejects.toEqual({ - code: 401, - message: 'test-error', - name: 'LaunchDarklyStreamingError', - }); + await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); }); test('identify change and error listeners', async () => { diff --git a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts index fbb1c53092..fe3e2c3c5c 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.timeout.test.ts @@ -61,7 +61,6 @@ describe('sdk-client identify timeout', () => { test('rejects with custom timeout', async () => { const timeout = 15; jest.advanceTimersByTimeAsync(timeout * 1000).then(); - await expect(ldc.identify(carContext, { timeout })).rejects.toThrow(/identify timed out/); }); From 5c6ece95d5bd5e6963e98958b633eede017796e6 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:43:35 -0700 Subject: [PATCH 07/14] chore: Log identify errors and then rethrow. --- packages/shared/sdk-client/src/LDClientImpl.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 422f5d7646..a42951c0b3 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -239,7 +239,11 @@ export default class LDClientImpl implements LDClient { }); const timed = timedPromise(timeout, 'identify'); - const raced = Promise.race([timed, slow]); + const raced = Promise.race([timed, slow]).catch((e) => { + this.logger.error(`identify error: ${e}`); + throw e; + }); + return { identifyPromise: raced, identifyResolve: res, identifyReject: rej }; } From a77a2fc5e81b978866973d32ae6a3063094347be Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:49:39 -0700 Subject: [PATCH 08/14] chore: Revert unnecessary code moves for easier review. --- .../shared/sdk-client/src/LDClientImpl.ts | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index a42951c0b3..d7dcd3e14d 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -46,7 +46,6 @@ export default class LDClientImpl implements LDClient { private eventFactoryWithReasons = new EventFactory(true); private emitter: LDEmitter; private flags: Flags = {}; - private identifyChangeListener?: (c: LDContext, changedKeys: string[]) => void; private readonly clientContext: ClientContext; @@ -229,6 +228,24 @@ export default class LDClientImpl implements LDClient { return listeners; } + /** + * Generates the url path for streamer. + * + * For mobile key: /meval/${base64-encoded-context} + * For clientSideId: /eval/${envId}/${base64-encoded-context} + * + * the path. + * + * @protected This function must be overridden in subclasses for streamer + * to work. + * @param _context The LDContext object + */ + protected createStreamUriPath(_context: LDContext): string { + throw new Error( + 'createStreamUriPath not implemented. Client sdks must implement createStreamUriPath for streamer to work.', + ); + } + private createIdentifyPromise(timeout: number) { let res: any; let rej: any; @@ -247,6 +264,11 @@ export default class LDClientImpl implements LDClient { return { identifyPromise: raced, identifyResolve: res, identifyReject: rej }; } + private async getFlagsFromStorage(canonicalKey: string): Promise { + const f = await this.platform.storage?.get(canonicalKey); + return f ? JSON.parse(f) : undefined; + } + /** * Identifies a context to LaunchDarkly. See {@link LDClient.identify}. * @@ -348,29 +370,6 @@ export default class LDClientImpl implements LDClient { } } - private async getFlagsFromStorage(canonicalKey: string): Promise { - const f = await this.platform.storage?.get(canonicalKey); - return f ? JSON.parse(f) : undefined; - } - - /** - * Generates the url path for streamer. - * - * For mobile key: /meval/${base64-encoded-context} - * For clientSideId: /eval/${envId}/${base64-encoded-context} - * - * the path. - * - * @protected This function must be overridden in subclasses for streamer - * to work. - * @param _context The LDContext object - */ - protected createStreamUriPath(_context: LDContext): string { - throw new Error( - 'createStreamUriPath not implemented. Client sdks must implement createStreamUriPath for streamer to work.', - ); - } - off(eventName: EventName, listener: Function): void { this.emitter.off(eventName, listener); } From 020aa9d3c591817cdeb7b3a9c82b6ef3787416dc Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:56:55 -0700 Subject: [PATCH 09/14] chore: Added more tests. --- packages/shared/sdk-client/src/LDClientImpl.test.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index 77a3aabffd..721094e207 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -208,7 +208,13 @@ describe('sdk-client object', () => { test('identify error stream error', async () => { setupMockStreamingProcessor(true); const carContext: LDContext = { kind: 'car', key: 'test-car' }; + await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); + expect(logger.error).toHaveBeenCalledWith( + expect.stringMatching(/^identify error:.*test-error/), + ); + expect(logger.error).toHaveBeenCalledWith(expect.stringMatching(/^error:.*test-error/)); + expect(ldc.getContext()).toBeUndefined(); }); test('identify change and error listeners', async () => { From 171ce38c9e8f8b0fab865064f6dbfa5345cfdad0 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 21:59:58 -0700 Subject: [PATCH 10/14] chore: Improved identify comments. --- packages/shared/sdk-client/src/LDClientImpl.ts | 7 ++++++- packages/shared/sdk-client/src/api/LDClient.ts | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index d7dcd3e14d..8f6a5a5444 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -274,8 +274,13 @@ export default class LDClientImpl implements LDClient { * * @param pristineContext The LDContext object to be identified. * @param identifyOptions Optional configuration. See {@link LDIdentifyOptions}. + * @returns + * A Promise which resolves when the flag values for the specified context + * are available. * - * @returns {Promise}. + * The promise rejects if the identify timeout is exceeded. In client SDKs + * this defaults to 5s. You can provide a custom timeout in + * {@link LDIdentifyOptions | identifyOptions}. */ async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { if (identifyOptions?.timeout) { diff --git a/packages/shared/sdk-client/src/api/LDClient.ts b/packages/shared/sdk-client/src/api/LDClient.ts index 98b3bd924e..199c7aa74c 100644 --- a/packages/shared/sdk-client/src/api/LDClient.ts +++ b/packages/shared/sdk-client/src/api/LDClient.ts @@ -112,7 +112,12 @@ export interface LDClient { * @param identifyOptions * Optional configuration. Please see {@link LDIdentifyOptions}. * @returns - * A Promise which resolves when the flag values for the specified context are available. + * A Promise which resolves when the flag values for the specified context + * are available. + * + * The promise rejects if the identify timeout is exceeded. In client SDKs + * this defaults to 5s. You can provide a custom timeout in + * {@link LDIdentifyOptions | identifyOptions}. */ identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise; From 301de89fc16d3c1ef78cfb0eeebcd876fe2cc585 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 22:15:53 -0700 Subject: [PATCH 11/14] chore: Include more rejection comments. --- packages/shared/sdk-client/src/LDClientImpl.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 8f6a5a5444..42e14e10bc 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -274,13 +274,15 @@ export default class LDClientImpl implements LDClient { * * @param pristineContext The LDContext object to be identified. * @param identifyOptions Optional configuration. See {@link LDIdentifyOptions}. - * @returns - * A Promise which resolves when the flag values for the specified context - * are available. + * @returns A Promise which resolves when the flag values for the specified + * context are available. It rejects when: * - * The promise rejects if the identify timeout is exceeded. In client SDKs - * this defaults to 5s. You can provide a custom timeout in - * {@link LDIdentifyOptions | identifyOptions}. + * 1. The context is unspecified or has no key. + * + * 2. The identify timeout is exceeded. In client SDKs this defaults to 5s. + * You can customize this timeout with {@link LDIdentifyOptions | identifyOptions}. + * + * 3. A streaming error occurs. */ async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { if (identifyOptions?.timeout) { From 533db7af8cd9497fc63d25a7fee99aead790a5e0 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Fri, 5 Apr 2024 22:17:31 -0700 Subject: [PATCH 12/14] Update LDClient.ts --- packages/shared/sdk-client/src/api/LDClient.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/shared/sdk-client/src/api/LDClient.ts b/packages/shared/sdk-client/src/api/LDClient.ts index 199c7aa74c..80880f8418 100644 --- a/packages/shared/sdk-client/src/api/LDClient.ts +++ b/packages/shared/sdk-client/src/api/LDClient.ts @@ -112,12 +112,15 @@ export interface LDClient { * @param identifyOptions * Optional configuration. Please see {@link LDIdentifyOptions}. * @returns - * A Promise which resolves when the flag values for the specified context - * are available. + * A Promise which resolves when the flag values for the specified + * context are available. It rejects when: * - * The promise rejects if the identify timeout is exceeded. In client SDKs - * this defaults to 5s. You can provide a custom timeout in - * {@link LDIdentifyOptions | identifyOptions}. + * 1. The context is unspecified or has no key. + * + * 2. The identify timeout is exceeded. In client SDKs this defaults to 5s. + * You can customize this timeout with {@link LDIdentifyOptions | identifyOptions}. + * + * 3. A streaming error occurs. */ identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise; From 2c4c9af390df70b85bbded0e2d3a2de9e670ac2e Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 9 Apr 2024 10:30:29 -0700 Subject: [PATCH 13/14] chore: Pr feedback to improve identify reject comments. --- packages/shared/sdk-client/src/LDClientImpl.ts | 6 ++++-- packages/shared/sdk-client/src/api/LDClient.ts | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 42e14e10bc..8cdf53dc38 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -257,7 +257,9 @@ export default class LDClientImpl implements LDClient { const timed = timedPromise(timeout, 'identify'); const raced = Promise.race([timed, slow]).catch((e) => { - this.logger.error(`identify error: ${e}`); + if (e.message.includes('timed out')) { + this.logger.error(`identify error: ${e}`); + } throw e; }); @@ -282,7 +284,7 @@ export default class LDClientImpl implements LDClient { * 2. The identify timeout is exceeded. In client SDKs this defaults to 5s. * You can customize this timeout with {@link LDIdentifyOptions | identifyOptions}. * - * 3. A streaming error occurs. + * 3. A network error is encountered during initialization. */ async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise { if (identifyOptions?.timeout) { diff --git a/packages/shared/sdk-client/src/api/LDClient.ts b/packages/shared/sdk-client/src/api/LDClient.ts index 80880f8418..1eb7cd3fb8 100644 --- a/packages/shared/sdk-client/src/api/LDClient.ts +++ b/packages/shared/sdk-client/src/api/LDClient.ts @@ -120,7 +120,7 @@ export interface LDClient { * 2. The identify timeout is exceeded. In client SDKs this defaults to 5s. * You can customize this timeout with {@link LDIdentifyOptions | identifyOptions}. * - * 3. A streaming error occurs. + * 3. A network error is encountered during initialization. */ identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise; From 2b5e62796c0e07c71e8a65ebd429b2fab2afbf89 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Tue, 9 Apr 2024 10:46:15 -0700 Subject: [PATCH 14/14] fix: Fixed broken unit test. --- packages/shared/sdk-client/src/LDClientImpl.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.test.ts b/packages/shared/sdk-client/src/LDClientImpl.test.ts index 721094e207..e803d86122 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.test.ts @@ -210,9 +210,7 @@ describe('sdk-client object', () => { const carContext: LDContext = { kind: 'car', key: 'test-car' }; await expect(ldc.identify(carContext)).rejects.toThrow('test-error'); - expect(logger.error).toHaveBeenCalledWith( - expect.stringMatching(/^identify error:.*test-error/), - ); + expect(logger.error).toHaveBeenCalledTimes(1); expect(logger.error).toHaveBeenCalledWith(expect.stringMatching(/^error:.*test-error/)); expect(ldc.getContext()).toBeUndefined(); });