Merge branch 'main' into ta/sc-249239/data-source-status-rebase

Author: kinyoklion

packages/sdk/browser/__tests__/BrowserDataManager.test.ts

@@ -74,9 +74,6 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
     };
     config = {
       logger,
-      baseUri: 'string',
-      eventsUri: 'string',
-      streamUri: 'string',
       maxCachedContexts: 5,
       capacity: 100,
       diagnosticRecordingInterval: 1000,
@@ -128,7 +125,7 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
       off: jest.fn(),
     } as unknown as jest.Mocked<FlagManager>;
 
-    browserConfig = validateOptions({ streaming: false }, logger);
+    browserConfig = validateOptions({}, logger);
     baseHeaders = {};
     emitter = {
       emit: jest.fn(),
@@ -277,7 +274,7 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
     await dataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
 
     expect(platform.requests.createEventSource).not.toHaveBeenCalled();
-    dataManager.startDataSource();
+    dataManager.setForcedStreaming(true);
     expect(platform.requests.createEventSource).toHaveBeenCalled();
   });
 
@@ -292,8 +289,8 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
     await dataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
 
     expect(platform.requests.createEventSource).not.toHaveBeenCalled();
-    dataManager.startDataSource();
-    dataManager.startDataSource();
+    dataManager.setForcedStreaming(true);
+    dataManager.setForcedStreaming(true);
     expect(platform.requests.createEventSource).toHaveBeenCalledTimes(1);
     expect(logger.debug).toHaveBeenCalledWith(
       '[BrowserDataManager] Update processor already active. Not changing state.',
@@ -302,10 +299,66 @@ describe('given a BrowserDataManager with mocked dependencies', () => {
 
   it('does not start a stream if identify has not been called', async () => {
     expect(platform.requests.createEventSource).not.toHaveBeenCalled();
-    dataManager.startDataSource();
+    dataManager.setForcedStreaming(true);
     expect(platform.requests.createEventSource).not.toHaveBeenCalledTimes(1);
     expect(logger.debug).toHaveBeenCalledWith(
       '[BrowserDataManager] Context not set, not starting update processor.',
     );
   });
+
+  it('starts a stream on demand when not forced on/off', async () => {
+    const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
+    const identifyOptions: LDIdentifyOptions = { waitForNetworkResults: false };
+    const identifyResolve = jest.fn();
+    const identifyReject = jest.fn();
+
+    flagManager.loadCached.mockResolvedValue(false);
+
+    await dataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
+
+    expect(platform.requests.createEventSource).not.toHaveBeenCalled();
+    dataManager.setAutomaticStreamingState(true);
+    expect(platform.requests.createEventSource).toHaveBeenCalled();
+    expect(logger.debug).toHaveBeenCalledWith('[BrowserDataManager] Starting update processor.');
+    expect(logger.debug).toHaveBeenCalledWith(
+      '[BrowserDataManager] Updating streaming state. forced(undefined) automatic(true)',
+    );
+  });
+
+  it('does not start a stream when forced off', async () => {
+    const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
+    const identifyOptions: LDIdentifyOptions = { waitForNetworkResults: false };
+    const identifyResolve = jest.fn();
+    const identifyReject = jest.fn();
+
+    dataManager.setForcedStreaming(false);
+
+    flagManager.loadCached.mockResolvedValue(false);
+
+    await dataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
+
+    expect(platform.requests.createEventSource).not.toHaveBeenCalled();
+    dataManager.setAutomaticStreamingState(true);
+    expect(platform.requests.createEventSource).not.toHaveBeenCalled();
+    expect(logger.debug).toHaveBeenCalledWith(
+      '[BrowserDataManager] Updating streaming state. forced(false) automatic(true)',
+    );
+  });
+
+  it('starts streaming on identify if the automatic state is true', async () => {
+    const context = Context.fromLDContext({ kind: 'user', key: 'test-user' });
+    const identifyOptions: LDIdentifyOptions = { waitForNetworkResults: false };
+    const identifyResolve = jest.fn();
+    const identifyReject = jest.fn();
+
+    dataManager.setForcedStreaming(undefined);
+    dataManager.setAutomaticStreamingState(true);
+    expect(platform.requests.createEventSource).not.toHaveBeenCalled();
+
+    flagManager.loadCached.mockResolvedValue(false);
+
+    await dataManager.identify(identifyResolve, identifyReject, context, identifyOptions);
+
+    expect(platform.requests.createEventSource).toHaveBeenCalled();
+  });
 });

packages/sdk/browser/src/BrowserClient.ts

@@ -1,9 +1,9 @@
 import {
   AutoEnvAttributes,
   base64UrlEncode,
-  BasicLogger,
   LDClient as CommonClient,
   Configuration,
+  createSafeLogger,
   Encoding,
   FlagManager,
   internal,
@@ -14,6 +14,7 @@ import {
   Platform,
 } from '@launchdarkly/js-client-sdk-common';
 import { LDIdentifyOptions } from '@launchdarkly/js-client-sdk-common/dist/api/LDIdentifyOptions';
+import { EventName } from '@launchdarkly/js-client-sdk-common/dist/LDEmitter';
 
 import BrowserDataManager from './BrowserDataManager';
 import GoalManager from './goals/GoalManager';
@@ -29,24 +30,40 @@ export type LDClient = Omit<
   CommonClient,
   'setConnectionMode' | 'getConnectionMode' | 'getOffline'
 > & {
-  setStreaming(streaming: boolean): void;
+  /**
+   * Specifies whether or not to open a streaming connection to LaunchDarkly for live flag updates.
+   *
+   * If this is true, the client will always attempt to maintain a streaming connection; if false,
+   * it never will. If you leave the value undefined (the default), the client will open a streaming
+   * connection if you subscribe to `"change"` or `"change:flag-key"` events (see {@link LDClient.on}).
+   *
+   * This can also be set as the `streaming` property of {@link LDOptions}.
+   */
+  setStreaming(streaming?: boolean): void;
 };
 
 export class BrowserClient extends LDClientImpl {
   private readonly goalManager?: GoalManager;
+
   constructor(
     private readonly clientSideId: string,
     autoEnvAttributes: AutoEnvAttributes,
     options: BrowserOptions = {},
     overridePlatform?: Platform,
   ) {
     const { logger: customLogger, debug } = options;
+    // Overrides the default logger from the common implementation.
     const logger =
       customLogger ??
-      new BasicLogger({
-        level: debug ? 'debug' : 'info',
+      createSafeLogger({
+        // eslint-disable-next-line no-console
+        debug: debug ? console.debug : () => {},
+        // eslint-disable-next-line no-console
+        info: console.info,
         // eslint-disable-next-line no-console
-        destination: console.log,
+        warn: console.warn,
+        // eslint-disable-next-line no-console
+        error: console.error,
       });
 
     // TODO: Use the already-configured baseUri from the SDK config. SDK-560
@@ -59,7 +76,7 @@ export class BrowserClient extends LDClientImpl {
       clientSideId,
       autoEnvAttributes,
       platform,
-      filterToBaseOptions(options),
+      filterToBaseOptions({ ...options, logger }),
       (
         flagManager: FlagManager,
         configuration: Configuration,
@@ -164,12 +181,27 @@ export class BrowserClient extends LDClientImpl {
     this.goalManager?.startTracking();
   }
 
-  setStreaming(streaming: boolean): void {
+  setStreaming(streaming?: boolean): void {
+    // With FDv2 we may want to consider if we support connection mode directly.
+    // Maybe with an extension to connection mode for 'automatic'.
     const browserDataManager = this.dataManager as BrowserDataManager;
-    if (streaming) {
-      browserDataManager.startDataSource();
-    } else {
-      browserDataManager.stopDataSource();
-    }
+    browserDataManager.setForcedStreaming(streaming);
+  }
+
+  private updateAutomaticStreamingState() {
+    const browserDataManager = this.dataManager as BrowserDataManager;
+    // This will need changed if support for listening to individual flag change
+    // events it added.
+    browserDataManager.setAutomaticStreamingState(!!this.emitter.listenerCount('change'));
+  }
+
+  override on(eventName: EventName, listener: Function): void {
+    super.on(eventName, listener);
+    this.updateAutomaticStreamingState();
+  }
+
+  override off(eventName: EventName, listener: Function): void {
+    super.off(eventName, listener);
+    this.updateAutomaticStreamingState();
   }
 }

packages/sdk/browser/src/BrowserDataManager.ts

@@ -18,6 +18,22 @@ import { ValidatedOptions } from './options';
 const logTag = '[BrowserDataManager]';
 
 export default class BrowserDataManager extends BaseDataManager {
+  // If streaming is forced on or off, then we follow that setting.
+  // Otherwise we automatically manage streaming state.
+  private forcedStreaming?: boolean = undefined;
+  private automaticStreamingState: boolean = false;
+
+  // +-----------+-----------+---------------+
+  // |  forced   | automatic |     state     |
+  // +-----------+-----------+---------------+
+  // | true      | false     | streaming     |
+  // | true      | true      | streaming     |
+  // | false     | true      | not streaming |
+  // | false     | false     | not streaming |
+  // | undefined | true      | streaming     |
+  // | undefined | false     | not streaming |
+  // +-----------+-----------+---------------+
+
   constructor(
     platform: Platform,
     flagManager: FlagManager,
@@ -41,6 +57,7 @@ export default class BrowserDataManager extends BaseDataManager {
       emitter,
       diagnosticsManager,
     );
+    this.forcedStreaming = browserConfig.streaming;
   }
 
   private debugLog(message: any, ...args: any[]) {
@@ -71,17 +88,43 @@ export default class BrowserDataManager extends BaseDataManager {
       identifyReject(e);
     }
 
-    if (this.browserConfig.streaming) {
-      this.setupConnection(context);
+    this.updateStreamingState();
+  }
+
+  setForcedStreaming(streaming?: boolean) {
+    this.forcedStreaming = streaming;
+    this.updateStreamingState();
+  }
+
+  setAutomaticStreamingState(streaming: boolean) {
+    this.automaticStreamingState = streaming;
+    this.updateStreamingState();
+  }
+
+  private updateStreamingState() {
+    const shouldBeStreaming =
+      this.forcedStreaming || (this.automaticStreamingState && this.forcedStreaming === undefined);
+
+    this.debugLog(
+      `Updating streaming state. forced(${this.forcedStreaming}) automatic(${this.automaticStreamingState})`,
+    );
+
+    if (shouldBeStreaming) {
+      this.startDataSource();
+    } else {
+      this.stopDataSource();
     }
   }
 
-  stopDataSource() {
+  private stopDataSource() {
+    if (this.updateProcessor) {
+      this.debugLog('Stopping update processor.');
+    }
     this.updateProcessor?.close();
     this.updateProcessor = undefined;
   }
 
-  startDataSource() {
+  private startDataSource() {
     if (this.updateProcessor) {
       this.debugLog('Update processor already active. Not changing state.');
       return;
@@ -132,5 +175,4 @@ export default class BrowserDataManager extends BaseDataManager {
     const uri = getPollingUri(this.config.serviceEndpoints, path, parameters);
     return new Requestor(this.platform.requests, uri, headers, method, body);
   }
-  // TODO: Automatically start streaming if event handlers are registered.
 }

packages/sdk/react-native/__tests__/MobileDataManager.test.ts

@@ -63,9 +63,6 @@ describe('given a MobileDataManager with mocked dependencies', () => {
     };
     config = {
       logger,
-      baseUri: 'string',
-      eventsUri: 'string',
-      streamUri: 'string',
       maxCachedContexts: 5,
       capacity: 100,
       diagnosticRecordingInterval: 1000,

packages/shared/sdk-client/__tests__/LDClientImpl.test.ts

@@ -282,7 +282,9 @@ describe('sdk-client object', () => {
     const carContext2: LDContext = { kind: 'car', key: 'test-car-2' };
     await ldc.identify(carContext2);
 
-    expect(emitter.listenerCount('change')).toEqual(1);
+    // No default listeners. This is important for clients to be able to determine if there are
+    // any listeners and act on that information.
+    expect(emitter.listenerCount('change')).toEqual(0);
     expect(emitter.listenerCount('error')).toEqual(1);
   });
 

packages/shared/sdk-client/__tests__/configuration/Configuration.test.ts

@@ -1,4 +1,6 @@
 /* eslint-disable no-console */
+import { createSafeLogger } from '@launchdarkly/js-sdk-common';
+
 import ConfigurationImpl from '../../src/configuration/Configuration';
 
 describe('Configuration', () => {
@@ -120,3 +122,48 @@ describe('Configuration', () => {
     },
   );
 });
+
+it('makes a safe logger', () => {
+  const badLogger = {
+    debug: () => {
+      throw new Error('bad');
+    },
+    info: () => {
+      throw new Error('bad');
+    },
+    warn: () => {
+      throw new Error('bad');
+    },
+    error: () => {
+      throw new Error('bad');
+    },
+  };
+  const config = new ConfigurationImpl({
+    logger: badLogger,
+  });
+
+  expect(() => config.logger.debug('debug')).not.toThrow();
+  expect(() => config.logger.info('info')).not.toThrow();
+  expect(() => config.logger.warn('warn')).not.toThrow();
+  expect(() => config.logger.error('error')).not.toThrow();
+  expect(config.logger).not.toBe(badLogger);
+});
+
+it('does not wrap already safe loggers', () => {
+  const logger = createSafeLogger({
+    debug: () => {
+      throw new Error('bad');
+    },
+    info: () => {
+      throw new Error('bad');
+    },
+    warn: () => {
+      throw new Error('bad');
+    },
+    error: () => {
+      throw new Error('bad');
+    },
+  });
+  const config = new ConfigurationImpl({ logger });
+  expect(config.logger).toBe(logger);
+});

packages/shared/sdk-client/__tests__/context/addAutoEnv.test.ts

@@ -233,8 +233,8 @@ describe('automatic environment attributes', () => {
 
       await addAutoEnv(context, mockPlatform, config);
 
-      expect(config.logger.warn).toHaveBeenCalledTimes(1);
-      expect(config.logger.warn).toHaveBeenCalledWith(
+      expect(logger.warn).toHaveBeenCalledTimes(1);
+      expect(logger.warn).toHaveBeenCalledWith(
         expect.stringMatching(/ld_application.*already exists/),
       );
     });
@@ -251,10 +251,8 @@ describe('automatic environment attributes', () => {
 
       await addAutoEnv(context, mockPlatform, config);
 
-      expect(config.logger.warn).toHaveBeenCalledTimes(1);
-      expect(config.logger.warn).toHaveBeenCalledWith(
-        expect.stringMatching(/ld_device.*already exists/),
-      );
+      expect(logger.warn).toHaveBeenCalledTimes(1);
+      expect(logger.warn).toHaveBeenCalledWith(expect.stringMatching(/ld_device.*already exists/));
     });
 
     test('single context with an attribute called ld_application should have auto env attributes', async () => {