-
Notifications
You must be signed in to change notification settings - Fork 31
fix: Send identify event. #407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(), | ||
| }), | ||
| ); | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| 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'; | ||
|
|
||
| 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<Flags>(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<InputIdentifyEvent>({ | ||
| kind: 'identify', | ||
| context: expect.objectContaining({ | ||
| contexts: expect.objectContaining({ | ||
| car: { key: 'test-car' }, | ||
| }), | ||
| }), | ||
| creationDate: expect.any(Number), | ||
| samplingRatio: expect.any(Number), | ||
| }), | ||
| ); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<void>((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(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolve is now explicitly called when appropriate, rather than here. This is more sensible because not all 'change' events need to resolve the identify promise. |
||
| }; | ||
| 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)); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was the missing piece to send |
||
| 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); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,4 @@ | ||
| import { ClientContext, internal, Platform } from '@launchdarkly/js-sdk-common'; | ||
| import { EventProcessor } from '@launchdarkly/js-sdk-common/dist/internal'; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an unrelated bug. I'm not sure why this wasn't caught by the linter. |
||
|
|
||
| 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 }, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dry'd this to
onIdentifyResolve.