fix: Identify incorrectly rejected in client-sdk (#426)

This is a bugfix. Previously, any `error` event from the emitter rejects
the identify promise, which is incorrect.

Author: yusinto

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<void>((_res, reject) => {
     setTimeout(() => {
       const e = `${taskName} timed out after ${t} seconds.`;
-      logger?.error(e);
       reject(new Error(e));
     }, t * 1000);
   });

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 {

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,9 @@ 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',
-    });
+    await expect(ldc.identify(carContext)).rejects.toThrow('test-error');
     expect(logger.error).toHaveBeenCalledTimes(1);
+    expect(logger.error).toHaveBeenCalledWith(expect.stringMatching(/^error:.*test-error/));
     expect(ldc.getContext()).toBeUndefined();
   });
 

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/);
   });
 

packages/shared/sdk-client/src/LDClientImpl.ts

@@ -46,8 +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 identifyErrorListener?: (c: LDContext, err: any) => void;
 
   private readonly clientContext: ClientContext;
 
@@ -81,6 +79,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)}`);
+    });
   }
 
   /**
@@ -244,32 +248,22 @@ export default class LDClientImpl implements LDClient {
 
   private createIdentifyPromise(timeout: number) {
     let res: any;
+    let rej: any;
+
     const slow = new Promise<void>((resolve, reject) => {
       res = resolve;
+      rej = reject;
+    });
 
-      if (this.identifyChangeListener) {
-        this.emitter.off('change', this.identifyChangeListener);
-      }
-      if (this.identifyErrorListener) {
-        this.emitter.off('error', this.identifyErrorListener);
+    const timed = timedPromise(timeout, 'identify');
+    const raced = Promise.race([timed, slow]).catch((e) => {
+      if (e.message.includes('timed out')) {
+        this.logger.error(`identify error: ${e}`);
       }
-
-      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.emitter.on('change', this.identifyChangeListener);
-      this.emitter.on('error', this.identifyErrorListener);
+      throw e;
     });
 
-    const timed = timedPromise(timeout, 'identify', this.logger);
-    const raced = Promise.race([timed, slow]);
-
-    return { identifyPromise: raced, identifyResolve: res };
+    return { identifyPromise: raced, identifyResolve: res, identifyReject: rej };
   }
 
   private async getFlagsFromStorage(canonicalKey: string): Promise<Flags | undefined> {
@@ -282,8 +276,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. It rejects when:
+   *
+   * 1. The context is unspecified or has no key.
    *
-   * @returns {Promise<void>}.
+   * 2. The identify timeout is exceeded. In client SDKs this defaults to 5s.
+   * You can customize this timeout with {@link LDIdentifyOptions | identifyOptions}.
+   *
+   * 3. A network error is encountered during initialization.
    */
   async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise<void> {
     if (identifyOptions?.timeout) {
@@ -303,13 +304,14 @@ 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);
       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);
@@ -340,7 +342,7 @@ export default class LDClientImpl implements LDClient {
         this.createStreamListeners(context, checkedContext.canonicalKey, identifyResolve),
         this.diagnosticsManager,
         (e) => {
-          this.logger.error(e);
+          identifyReject(e);
           this.emitter.emit('error', context, e);
         },
       );
@@ -387,12 +389,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 +410,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 +422,6 @@ export default class LDClientImpl implements LDClient {
       const error = new LDClientError(
         `Unknown feature flag "${flagKey}"; returning default value ${defVal}.`,
       );
-      this.logger.error(error);
       this.emitter.emit('error', this.context, error);
       this.eventProcessor?.sendEvent(
         this.eventFactoryDefault.unknownFlagEvent(flagKey, defVal, evalContext),
@@ -446,7 +447,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);
       }

packages/shared/sdk-client/src/LDClientImpl.variation.test.ts

@@ -0,0 +1,93 @@
+import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common';
+import { basicPlatform, logger, setupMockStreamingProcessor } from '@launchdarkly/private-js-mocks';
+
+import * as mockResponseJson from './evaluation/mockResponse.json';
+import LDClientImpl from './LDClientImpl';
+import { Flags } from './types';
+
+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 defaultPutResponse: Flags;
+
+describe('sdk-client object', () => {
+  beforeEach(() => {
+    defaultPutResponse = clone<Flags>(mockResponseJson);
+    setupMockStreamingProcessor(false, defaultPutResponse);
+    ldc = new LDClientImpl(testSdkKey, AutoEnvAttributes.Enabled, basicPlatform, {
+      logger,
+      sendEvents: false,
+    });
+    jest
+      .spyOn(LDClientImpl.prototype as any, 'createStreamUriPath')
+      .mockReturnValue('/stream/path');
+  });
+
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+
+  test('variation', async () => {
+    await ldc.identify(context);
+    const devTestFlag = ldc.variation('dev-test-flag');
+
+    expect(devTestFlag).toBe(true);
+  });
+
+  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);
+    setTimeout(() => {
+      // call variation in the next tick to give ldc a chance to hook up event emitter
+      ldc.variation('does-not-exist', 'not-found');
+    });
+
+    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 () => {
+    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,
+    });
+  });
+});

packages/shared/sdk-client/src/api/LDClient.ts

@@ -112,7 +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:
+   *
+   * 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 network error is encountered during initialization.
    */
   identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise<void>;