From f1331b59d5a8bba8b0c402babf317881d1ad5060 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 25 Mar 2024 12:22:20 -0700 Subject: [PATCH 1/5] fix: Send identify event. --- .../shared/sdk-client/src/LDClientImpl.ts | 53 +++++++++++-------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.ts b/packages/shared/sdk-client/src/LDClientImpl.ts index 5a97255c2f..57efc8bd4a 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.ts @@ -168,20 +168,8 @@ export default class LDClientImpl implements LDClient { deserializeData: JSON.parse, processJson: async (dataJson: Flags) => { this.logger.debug(`Streamer PUT: ${Object.keys(dataJson)}`); - const changedKeys = calculateFlagChanges(this.flags, dataJson); - this.context = context; - this.flags = dataJson; + this.onIdentifyResolve(identifyResolve, dataJson, context, 'streamer PUT'); await this.platform.storage?.set(canonicalKey, JSON.stringify(this.flags)); - - if (changedKeys.length > 0) { - this.logger.debug(`Emitting changes from PUT: ${changedKeys}`); - // emitting change resolves identify - this.emitter.emit('change', context, changedKeys); - } else { - // manually resolve identify - this.logger.debug('Not emitting changes from PUT'); - identifyResolve(); - } }, }); @@ -249,7 +237,7 @@ export default class LDClientImpl implements LDClient { ); } - private createPromiseWithListeners() { + private createIdentifyPromise() { let res: any; const p = new Promise((resolve, reject) => { res = resolve; @@ -263,7 +251,6 @@ export default class LDClientImpl implements LDClient { this.identifyChangeListener = (c: LDContext, changedKeys: string[]) => { this.logger.debug(`change: context: ${JSON.stringify(c)}, flags: ${changedKeys}`); - resolve(); }; this.identifyErrorListener = (c: LDContext, err: any) => { this.logger.debug(`error: ${err}, context: ${JSON.stringify(c)}`); @@ -297,17 +284,14 @@ export default class LDClientImpl implements LDClient { return Promise.reject(error); } - const { identifyPromise, identifyResolve } = this.createPromiseWithListeners(); + this.eventProcessor?.sendEvent(this.eventFactoryDefault.identifyEvent(checkedContext)); + const { identifyPromise, identifyResolve } = this.createIdentifyPromise(); this.logger.debug(`Identifying ${JSON.stringify(context)}`); const flagsStorage = await this.getFlagsFromStorage(checkedContext.canonicalKey); if (flagsStorage) { this.logger.debug('Using storage'); - - const changedKeys = calculateFlagChanges(this.flags, flagsStorage); - this.context = context; - this.flags = flagsStorage; - this.emitter.emit('change', context, changedKeys); + this.onIdentifyResolve(identifyResolve, flagsStorage, context, 'identify storage'); } if (this.isOffline()) { @@ -342,6 +326,33 @@ export default class LDClientImpl implements LDClient { return identifyPromise; } + /** + * Performs common tasks when resolving the identify promise: + * - resolve the promise + * - update in memory context + * - update in memory flags + * - emit change event if needed + * + * @param resolve + * @param flags + * @param context + * @param source For logging purposes + * @private + */ + private onIdentifyResolve(resolve: any, flags: Flags, context: LDContext, source: string) { + resolve(); + const changedKeys = calculateFlagChanges(this.flags, flags); + this.context = context; + this.flags = flags; + + if (changedKeys.length > 0) { + this.emitter.emit('change', context, changedKeys); + this.logger.debug(`OnIdentifyResolve emitting changes from: ${source}.`); + } else { + this.logger.debug(`OnIdentifyResolve no changes to emit from: ${source}.`); + } + } + off(eventName: EventName, listener: Function): void { this.emitter.off(eventName, listener); } From 9c244b7684cec290f75dcef5e5d3424e2e698c7b Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 25 Mar 2024 14:16:48 -0700 Subject: [PATCH 2/5] chore: Added unit test. --- .../common/src/internal/events/index.ts | 3 +- packages/shared/mocks/src/eventProcessor.ts | 19 +++++ packages/shared/mocks/src/index.ts | 3 + .../src/LDClientImpl.events.test.ts | 77 +++++++++++++++++++ .../src/events/createEventProcessor.ts | 3 +- 5 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 packages/shared/mocks/src/eventProcessor.ts create mode 100644 packages/shared/sdk-client/src/LDClientImpl.events.test.ts diff --git a/packages/shared/common/src/internal/events/index.ts b/packages/shared/common/src/internal/events/index.ts index 27035ef5ee..4744c33530 100644 --- a/packages/shared/common/src/internal/events/index.ts +++ b/packages/shared/common/src/internal/events/index.ts @@ -1,5 +1,5 @@ import ClientMessages from './ClientMessages'; -import EventProcessor from './EventProcessor'; +import EventProcessor, { EventProcessorOptions } from './EventProcessor'; import InputCustomEvent from './InputCustomEvent'; import InputEvalEvent from './InputEvalEvent'; import InputEvent from './InputEvent'; @@ -17,6 +17,7 @@ export { InputIdentifyEvent, InputMigrationEvent, EventProcessor, + EventProcessorOptions, shouldSample, NullEventProcessor, LDInternalOptions, diff --git a/packages/shared/mocks/src/eventProcessor.ts b/packages/shared/mocks/src/eventProcessor.ts new file mode 100644 index 0000000000..ffa577e16f --- /dev/null +++ b/packages/shared/mocks/src/eventProcessor.ts @@ -0,0 +1,19 @@ +import type { ClientContext, internal, subsystem } from '@common'; + +export const MockEventProcessor = jest.fn(); + +export const setupMockEventProcessor = () => { + MockEventProcessor.mockImplementation( + ( + _config: internal.EventProcessorOptions, + _clientContext: ClientContext, + _contextDeduplicator?: subsystem.LDContextDeduplicator, + _diagnosticsManager?: internal.DiagnosticsManager, + _start: boolean = true, + ) => ({ + close: jest.fn(), + flush: jest.fn(), + sendEvent: jest.fn(), + }), + ); +}; diff --git a/packages/shared/mocks/src/index.ts b/packages/shared/mocks/src/index.ts index 8c6927ba78..c299841036 100644 --- a/packages/shared/mocks/src/index.ts +++ b/packages/shared/mocks/src/index.ts @@ -1,6 +1,7 @@ import { clientContext } from './clientContext'; import ContextDeduplicator from './contextDeduplicator'; import { hasher } from './crypto'; +import { MockEventProcessor, setupMockEventProcessor } from './eventProcessor'; import logger from './logger'; import mockFetch from './mockFetch'; import { basicPlatform } from './platform'; @@ -13,6 +14,8 @@ export { mockFetch, logger, ContextDeduplicator, + MockEventProcessor, + setupMockEventProcessor, MockStreamingProcessor, setupMockStreamingProcessor, }; diff --git a/packages/shared/sdk-client/src/LDClientImpl.events.test.ts b/packages/shared/sdk-client/src/LDClientImpl.events.test.ts new file mode 100644 index 0000000000..be70e03d93 --- /dev/null +++ b/packages/shared/sdk-client/src/LDClientImpl.events.test.ts @@ -0,0 +1,77 @@ +import { AutoEnvAttributes, clone, LDContext } from '@launchdarkly/js-sdk-common'; +import { InputIdentifyEvent } from '@launchdarkly/js-sdk-common/dist/internal'; +import { + basicPlatform, + hasher, + logger, + MockEventProcessor, + setupMockEventProcessor, + setupMockStreamingProcessor, +} from '@launchdarkly/private-js-mocks'; + +import * as mockResponseJson from './evaluation/mockResponse.json'; +import LDClientImpl from './LDClientImpl'; +import { Flags } from './types'; +import { toMulti } from './utils/addAutoEnv'; + +jest.mock('@launchdarkly/js-sdk-common', () => { + const actual = jest.requireActual('@launchdarkly/js-sdk-common'); + const m = jest.requireActual('@launchdarkly/private-js-mocks'); + return { + ...actual, + ...{ + internal: { + ...actual.internal, + StreamingProcessor: m.MockStreamingProcessor, + EventProcessor: m.MockEventProcessor, + }, + }, + }; +}); + +const testSdkKey = 'test-sdk-key'; +let ldc: LDClientImpl; +let defaultPutResponse: Flags; + +describe('sdk-client object', () => { + beforeEach(() => { + defaultPutResponse = clone(mockResponseJson); + setupMockEventProcessor(); + setupMockStreamingProcessor(false, defaultPutResponse); + basicPlatform.crypto.randomUUID.mockReturnValue('random1'); + hasher.digest.mockReturnValue('digested1'); + + ldc = new LDClientImpl(testSdkKey, AutoEnvAttributes.Enabled, basicPlatform, { + logger, + }); + jest + .spyOn(LDClientImpl.prototype as any, 'createStreamUriPath') + .mockReturnValue('/stream/path'); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + test('identify event', async () => { + defaultPutResponse['dev-test-flag'].value = false; + const carContext: LDContext = { kind: 'car', key: 'test-car' }; + + await ldc.identify(carContext); + + expect(MockEventProcessor).toHaveBeenCalled(); + expect(ldc.eventProcessor!.sendEvent).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + kind: 'identify', + context: expect.objectContaining({ + contexts: expect.objectContaining({ + car: { key: 'test-car' }, + }), + }), + creationDate: expect.any(Number), + samplingRatio: expect.any(Number), + }), + ); + }); +}); diff --git a/packages/shared/sdk-client/src/events/createEventProcessor.ts b/packages/shared/sdk-client/src/events/createEventProcessor.ts index ad0f33fd9a..8e9075e7da 100644 --- a/packages/shared/sdk-client/src/events/createEventProcessor.ts +++ b/packages/shared/sdk-client/src/events/createEventProcessor.ts @@ -1,5 +1,4 @@ import { ClientContext, internal, Platform } from '@launchdarkly/js-sdk-common'; -import { EventProcessor } from '@launchdarkly/js-sdk-common/dist/internal'; import Configuration from '../configuration'; @@ -9,7 +8,7 @@ const createEventProcessor = ( platform: Platform, diagnosticsManager?: internal.DiagnosticsManager, start: boolean = false, -): EventProcessor | undefined => { +): internal.EventProcessor | undefined => { if (config.sendEvents) { return new internal.EventProcessor( { ...config, eventsCapacity: config.capacity }, From 583de03bf1b3a92c6cc7d24c340ccb3e7871b8c5 Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 25 Mar 2024 14:19:17 -0700 Subject: [PATCH 3/5] chore: Remove unused import. --- packages/shared/sdk-client/src/LDClientImpl.events.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.events.test.ts b/packages/shared/sdk-client/src/LDClientImpl.events.test.ts index be70e03d93..17a705d097 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.events.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.events.test.ts @@ -12,7 +12,6 @@ import { import * as mockResponseJson from './evaluation/mockResponse.json'; import LDClientImpl from './LDClientImpl'; import { Flags } from './types'; -import { toMulti } from './utils/addAutoEnv'; jest.mock('@launchdarkly/js-sdk-common', () => { const actual = jest.requireActual('@launchdarkly/js-sdk-common'); From 74660793b35effffc61049fbd6892f2cb2a35e9b Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 25 Mar 2024 14:23:09 -0700 Subject: [PATCH 4/5] fix: Broken unit test. --- packages/shared/sdk-client/src/LDClientImpl.storage.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts b/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts index c3370e41db..7cc67fdac0 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts @@ -174,7 +174,9 @@ describe('sdk-client storage', () => { 'org:Testy Pizza', JSON.stringify(defaultPutResponse), ); - expect(ldc.logger.debug).toHaveBeenCalledWith('Not emitting changes from PUT'); + expect(ldc.logger.debug).toHaveBeenCalledWith( + 'OnIdentifyResolve no changes to emit from: streamer PUT.', + ); // this is defaultPutResponse expect(ldc.allFlags()).toEqual({ From 24e5d3a4ac63af0065dcad6514c5602edbdf181e Mon Sep 17 00:00:00 2001 From: Yusinto Ngadiman Date: Mon, 25 Mar 2024 14:58:44 -0700 Subject: [PATCH 5/5] chore: Added more tests. --- .../src/LDClientImpl.storage.test.ts | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts b/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts index 7cc67fdac0..a999923bfa 100644 --- a/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts +++ b/packages/shared/sdk-client/src/LDClientImpl.storage.test.ts @@ -159,6 +159,32 @@ describe('sdk-client storage', () => { }); }); + test('not emitting change event', async () => { + jest.doMock('./utils', () => { + const actual = jest.requireActual('./utils'); + return { + ...actual, + calculateFlagChanges: () => [], + }; + }); + let LDClientImplTestNoChange; + jest.isolateModules(async () => { + LDClientImplTestNoChange = jest.requireActual('./LDClientImpl').default; + ldc = new LDClientImplTestNoChange(testSdkKey, AutoEnvAttributes.Enabled, basicPlatform, { + logger, + sendEvents: false, + }); + }); + + // @ts-ignore + emitter = ldc.emitter; + jest.spyOn(emitter as LDEmitter, 'emit'); + + await identifyGetAllFlags(true, defaultPutResponse); + + expect(emitter.emit).not.toHaveBeenCalled(); + }); + test('no storage, cold start from streamer', async () => { // fake previously cached flags even though there's no storage for this context // @ts-ignore @@ -207,14 +233,13 @@ describe('sdk-client storage', () => { test('syncing storage when a flag is added', async () => { const putResponse = clone(defaultPutResponse); - const newFlag = { + putResponse['another-dev-test-flag'] = { version: 1, flagVersion: 2, value: false, variation: 1, trackEvents: false, }; - putResponse['another-dev-test-flag'] = newFlag; const allFlags = await identifyGetAllFlags(false, putResponse); expect(allFlags).toMatchObject({ 'another-dev-test-flag': false });