From 39a115580016999da7ed488dc768f13777cb6682 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 14 Oct 2024 13:02:28 -0700 Subject: [PATCH 1/5] feat: Add support for client-side prerequisite events. --- src/__tests__/LDClient-events-test.js | 76 +++++++++++++++++++++++++++ src/index.js | 9 +++- 2 files changed, 84 insertions(+), 1 deletion(-) diff --git a/src/__tests__/LDClient-events-test.js b/src/__tests__/LDClient-events-test.js index 70bc068..9ce8f31 100644 --- a/src/__tests__/LDClient-events-test.js +++ b/src/__tests__/LDClient-events-test.js @@ -1,3 +1,4 @@ +// @ts-nocheck import * as messages from '../messages'; import { withCloseable, sleepAsync } from 'launchdarkly-js-test-helpers'; @@ -253,6 +254,81 @@ describe('LDClient events', () => { }); }); + it('sends events for prerequisites', async () => { + const initData = makeBootstrap({ + 'is-prereq': { + value: true, + variation: 1, + reason: { + kind: 'FALLTHROUGH', + }, + version: 1, + trackEvents: true, + trackReason: true, + }, + 'has-prereq-depth-1': { + value: true, + variation: 0, + prerequisites: ['is-prereq'], + reason: { + kind: 'FALLTHROUGH', + }, + version: 4, + trackEvents: true, + trackReason: true, + }, + 'has-prereq-depth-2': { + value: true, + variation: 0, + prerequisites: ['has-prereq-depth-1'], + reason: { + kind: 'FALLTHROUGH', + }, + version: 5, + trackEvents: true, + trackReason: true, + }, + }); + await withClientAndEventProcessor(user, { bootstrap: initData }, async (client, ep) => { + await client.waitForInitialization(5); + client.variation('has-prereq-depth-2', false); + + // An identify event and 3 feature events. + expect(ep.events.length).toEqual(4); + expectIdentifyEvent(ep.events[0], user); + expect(ep.events[1]).toMatchObject({ + kind: 'feature', + key: 'is-prereq', + variation: 1, + value: true, + version: 1, + reason: { + kind: 'FALLTHROUGH', + }, + }); + expect(ep.events[2]).toMatchObject({ + kind: 'feature', + key: 'has-prereq-depth-1', + variation: 0, + value: true, + version: 4, + reason: { + kind: 'FALLTHROUGH', + }, + }); + expect(ep.events[3]).toMatchObject({ + kind: 'feature', + key: 'has-prereq-depth-2', + variation: 0, + value: true, + version: 5, + reason: { + kind: 'FALLTHROUGH', + }, + }); + }); + }); + it('sends a feature event on receiving a new flag value', async () => { const oldFlags = { foo: { value: 'a', variation: 1, version: 2, flagVersion: 2000 } }; const newFlags = { foo: { value: 'b', variation: 2, version: 3, flagVersion: 2001 } }; diff --git a/src/index.js b/src/index.js index df85745..f623825 100644 --- a/src/index.js +++ b/src/index.js @@ -308,9 +308,10 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { function variationDetailInternal(key, defaultValue, sendEvent, includeReasonInEvent, isAllFlags) { let detail; + let flag; if (flags && utils.objectHasOwnProperty(flags, key) && flags[key] && !flags[key].deleted) { - const flag = flags[key]; + flag = flags[key]; detail = getFlagDetail(flag); if (flag.value === null || flag.value === undefined) { detail.value = defaultValue; @@ -320,6 +321,12 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } if (sendEvent) { + // An event will be send for each of these by virtue of sending events for all flags. + if (!isAllFlags) { + flag?.prerequisites?.forEach(key => { + variation(key, undefined); + }); + } sendFlagEvent(key, detail, defaultValue, includeReasonInEvent); } From 6458672aab23d5762ead8faab9e1c46b03bb4e06 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Mon, 14 Oct 2024 14:52:03 -0700 Subject: [PATCH 2/5] Fix link in docs. --- typings.d.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/typings.d.ts b/typings.d.ts index c2f179d..615c8a4 100644 --- a/typings.d.ts +++ b/typings.d.ts @@ -552,7 +552,7 @@ declare module 'launchdarkly-js-sdk-common' { /** * Describes the reason that a flag evaluation produced a particular value. This is - * part of the {@link LDEvaluationDetail} object returned by {@link LDClient.variationDetail]]. + * part of the {@link LDEvaluationDetail} object returned by {@link LDClient.variationDetail}. */ export interface LDEvaluationReason { /** From f7efc4ca3df595848ca74609e4447fc702b454cf Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 18 Oct 2024 08:47:50 -0700 Subject: [PATCH 3/5] Do not run inspections for prerequisites. --- src/__tests__/LDClient-inspectors-test.js | 108 +++++++++++++++++++++- src/index.js | 19 ++-- 2 files changed, 114 insertions(+), 13 deletions(-) diff --git a/src/__tests__/LDClient-inspectors-test.js b/src/__tests__/LDClient-inspectors-test.js index cc1897c..4a31c33 100644 --- a/src/__tests__/LDClient-inspectors-test.js +++ b/src/__tests__/LDClient-inspectors-test.js @@ -5,6 +5,41 @@ const stubPlatform = require('./stubPlatform'); const envName = 'UNKNOWN_ENVIRONMENT_ID'; const context = { key: 'context-key' }; +const flagPayload = { + 'is-prereq': { + value: true, + variation: 1, + reason: { + kind: 'FALLTHROUGH', + }, + version: 1, + trackEvents: true, + trackReason: true, + }, + 'has-prereq-depth-1': { + value: true, + variation: 0, + prerequisites: ['is-prereq'], + reason: { + kind: 'FALLTHROUGH', + }, + version: 4, + trackEvents: true, + trackReason: true, + }, + 'has-prereq-depth-2': { + value: true, + variation: 0, + prerequisites: ['has-prereq-depth-1'], + reason: { + kind: 'FALLTHROUGH', + }, + version: 5, + trackEvents: true, + trackReason: true, + }, +}; + describe.each([true, false])('given a streaming client with registered inspectors, synchronous: %p', synchronous => { const eventQueue = new AsyncQueue(); @@ -63,7 +98,7 @@ describe.each([true, false])('given a streaming client with registered inspector beforeEach(async () => { platform = stubPlatform.defaults(); const server = platform.testing.http.newServer(); - server.byDefault(respondJson({})); + server.byDefault(respondJson(flagPayload)); const config = { streaming: true, baseUrl: server.url, inspectors, sendEvents: false }; client = platform.testing.makeClient(envName, context, config); await client.waitUntilReady(); @@ -91,7 +126,29 @@ describe.each([true, false])('given a streaming client with registered inspector const flagsEvent = await eventQueue.take(); expect(flagsEvent).toMatchObject({ type: 'flag-details-changed', - details: {}, + details: { + 'is-prereq': { + value: true, + variationIndex: 1, + reason: { + kind: 'FALLTHROUGH', + }, + }, + 'has-prereq-depth-1': { + value: true, + variationIndex: 0, + reason: { + kind: 'FALLTHROUGH', + }, + }, + 'has-prereq-depth-2': { + value: true, + variationIndex: 0, + reason: { + kind: 'FALLTHROUGH', + }, + }, + }, }); }); @@ -129,4 +186,51 @@ describe.each([true, false])('given a streaming client with registered inspector flagDetail: { value: false }, }); }); + + it('emits an event when a flag is used', async () => { + // Take initial events. + eventQueue.take(); + eventQueue.take(); + + await platform.testing.eventSourcesCreated.take(); + client.variation('is-prereq', false); + const updateEvent = await eventQueue.take(); + expect(updateEvent).toMatchObject({ + type: 'flag-used', + flagKey: 'is-prereq', + flagDetail: { value: true }, + }); + // Two inspectors are handling this + const updateEvent2 = await eventQueue.take(); + expect(updateEvent2).toMatchObject({ + type: 'flag-used', + flagKey: 'is-prereq', + flagDetail: { value: true }, + }); + }); + + it('does not execute flag-used for prerequisites', async () => { + // Take initial events. + eventQueue.take(); + eventQueue.take(); + + await platform.testing.eventSourcesCreated.take(); + client.variation('has-prereq-depth-2', false); + // There would be many more than 2 events if prerequisites were inspected. + const updateEvent = await eventQueue.take(); + expect(updateEvent).toMatchObject({ + type: 'flag-used', + flagKey: 'has-prereq-depth-2', + flagDetail: { value: true }, + }); + // Two inspectors are handling this + const updateEvent2 = await eventQueue.take(); + expect(updateEvent2).toMatchObject({ + type: 'flag-used', + flagKey: 'has-prereq-depth-2', + flagDetail: { value: true }, + }); + + expect(eventQueue.length()).toEqual(0); + }); }); diff --git a/src/index.js b/src/index.js index f623825..dc10e07 100644 --- a/src/index.js +++ b/src/index.js @@ -299,14 +299,14 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } function variation(key, defaultValue) { - return variationDetailInternal(key, defaultValue, true, false, false).value; + return variationDetailInternal(key, defaultValue, true, false, true).value; } function variationDetail(key, defaultValue) { - return variationDetailInternal(key, defaultValue, true, true, false); + return variationDetailInternal(key, defaultValue, true, true, true); } - function variationDetailInternal(key, defaultValue, sendEvent, includeReasonInEvent, isAllFlags) { + function variationDetailInternal(key, defaultValue, sendEvent, includeReasonInEvent, notifyInspectionUsed) { let detail; let flag; @@ -321,17 +321,14 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } if (sendEvent) { - // An event will be send for each of these by virtue of sending events for all flags. - if (!isAllFlags) { - flag?.prerequisites?.forEach(key => { - variation(key, undefined); - }); - } + flag?.prerequisites?.forEach(key => { + variationDetailInternal(key, undefined, sendEvent, false, false); + }); sendFlagEvent(key, detail, defaultValue, includeReasonInEvent); } // For the all flags case `onFlags` will be called instead. - if (!isAllFlags) { + if (notifyInspectionUsed) { notifyInspectionFlagUsed(key, detail); } @@ -358,7 +355,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { for (const key in flags) { if (utils.objectHasOwnProperty(flags, key) && !flags[key].deleted) { - results[key] = variationDetailInternal(key, null, !options.sendEventsOnlyForVariation, false, true).value; + results[key] = variationDetailInternal(key, null, !options.sendEventsOnlyForVariation, false, false).value; } } From 6387ebb7eed765eb375ca024c485ce61b24e1b9f Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 18 Oct 2024 09:01:02 -0700 Subject: [PATCH 4/5] Refactoring. --- src/__tests__/LDClient-events-test.js | 16 ++++++++ src/index.js | 59 ++++++++++++++++----------- 2 files changed, 51 insertions(+), 24 deletions(-) diff --git a/src/__tests__/LDClient-events-test.js b/src/__tests__/LDClient-events-test.js index 9ce8f31..e882baf 100644 --- a/src/__tests__/LDClient-events-test.js +++ b/src/__tests__/LDClient-events-test.js @@ -403,6 +403,22 @@ describe('LDClient events', () => { }); }); + it('does not send duplicate events for prerequisites with all flags.', async () => { + const initData = makeBootstrap({ + foo: { value: 'a', variation: 1, version: 2 }, + bar: { value: 'b', variation: 1, version: 3, prerequisites: ['foo'] }, + }); + await withClientAndEventProcessor(user, { bootstrap: initData }, async (client, ep) => { + await client.waitForInitialization(5); + client.allFlags(); + + expect(ep.events.length).toEqual(3); + expectIdentifyEvent(ep.events[0], user); + expectFeatureEvent({ e: ep.events[1], key: 'foo', user, value: 'a', variation: 1, version: 2, defaultVal: null }); + expectFeatureEvent({ e: ep.events[2], key: 'bar', user, value: 'b', variation: 1, version: 3, defaultVal: null }); + }); + }); + it('does not send feature events for allFlags() if sendEventsOnlyForVariation is set', async () => { const initData = makeBootstrap({ foo: { value: 'a', variation: 1, version: 2 }, diff --git a/src/index.js b/src/index.js index dc10e07..d77a37d 100644 --- a/src/index.js +++ b/src/index.js @@ -50,14 +50,14 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { const diagnosticsAccumulator = diagnosticsEnabled ? diagnostics.DiagnosticsAccumulator(new Date().getTime()) : null; const diagnosticsManager = diagnosticsEnabled ? diagnostics.DiagnosticsManager( - platform, - persistentStorage, - diagnosticsAccumulator, - eventSender, - environment, - options, - diagnosticId - ) + platform, + persistentStorage, + diagnosticsAccumulator, + eventSender, + environment, + options, + diagnosticId + ) : null; const stream = Stream(platform, options, environment, diagnosticsAccumulator); @@ -299,14 +299,14 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } function variation(key, defaultValue) { - return variationDetailInternal(key, defaultValue, true, false, true).value; + return variationDetailInternal(key, defaultValue, true, false, false, true).value; } function variationDetail(key, defaultValue) { - return variationDetailInternal(key, defaultValue, true, true, true); + return variationDetailInternal(key, defaultValue, true, true, false, true); } - function variationDetailInternal(key, defaultValue, sendEvent, includeReasonInEvent, notifyInspectionUsed) { + function variationDetailInternal(key, defaultValue, sendEvent, includeReasonInEvent, isAllFlags, notifyInspection) { let detail; let flag; @@ -321,14 +321,18 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } if (sendEvent) { - flag?.prerequisites?.forEach(key => { - variationDetailInternal(key, undefined, sendEvent, false, false); - }); + // For an all-flags evaluation, with events enabled, each flag will get an event, so we do not + // need to duplicate the prerequisites. + if (!isAllFlags) { + flag?.prerequisites?.forEach(key => { + variationDetailInternal(key, undefined, sendEvent, false, false, false); + }); + } sendFlagEvent(key, detail, defaultValue, includeReasonInEvent); } // For the all flags case `onFlags` will be called instead. - if (notifyInspectionUsed) { + if (!isAllFlags && notifyInspection) { notifyInspectionFlagUsed(key, detail); } @@ -355,7 +359,14 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { for (const key in flags) { if (utils.objectHasOwnProperty(flags, key) && !flags[key].deleted) { - results[key] = variationDetailInternal(key, null, !options.sendEventsOnlyForVariation, false, false).value; + results[key] = variationDetailInternal( + key, + null, + !options.sendEventsOnlyForVariation, + false, + true, + false + ).value; } } @@ -419,7 +430,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } }; stream.connect(ident.getContext(), hash, { - ping: function() { + ping: function () { logger.debug(messages.debugStreamPing()); const contextAtTimeOfPingEvent = ident.getContext(); requestor @@ -435,7 +446,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { emitter.maybeReportError(new errors.LDFlagFetchError(messages.errorFetchingFlags(err))); }); }, - put: function(e) { + put: function (e) { const data = tryParseData(e.data); if (!data) { return; @@ -445,7 +456,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { // Don't wait for this Promise to be resolved; note that replaceAllFlags is guaranteed // never to have an unhandled rejection }, - patch: function(e) { + patch: function (e) { const data = tryParseData(e.data); if (!data) { return; @@ -472,7 +483,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { logger.debug(messages.debugStreamPatchIgnored(data.key)); } }, - delete: function(e) { + delete: function (e) { const data = tryParseData(e.data); if (!data) { return; @@ -529,7 +540,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { notifyInspectionFlagsChanged(); - return handleFlagChanges(changes).catch(() => {}); // swallow any exceptions from this Promise + return handleFlagChanges(changes).catch(() => { }); // swallow any exceptions from this Promise } // Returns a Promise which will be resolved when we have dispatched all change events and updated @@ -785,8 +796,8 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { if (timeout > highTimeoutThreshold) { logger.warn( 'The waitForInitialization function was called with a timeout greater than ' + - `${highTimeoutThreshold} seconds. We recommend a timeout of ` + - `${highTimeoutThreshold} seconds or less.` + `${highTimeoutThreshold} seconds. We recommend a timeout of ` + + `${highTimeoutThreshold} seconds or less.` ); } @@ -810,7 +821,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } logger.warn( 'The waitForInitialization function was called without a timeout specified.' + - ' In a future version a default timeout will be applied.' + ' In a future version a default timeout will be applied.' ); return initializationStateTracker.getInitializationPromise(); } From 0a8f078183032b886f779682f128ae3be6cac0b0 Mon Sep 17 00:00:00 2001 From: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com> Date: Fri, 18 Oct 2024 09:06:21 -0700 Subject: [PATCH 5/5] Lint index. --- src/index.js | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/index.js b/src/index.js index d77a37d..5b3c503 100644 --- a/src/index.js +++ b/src/index.js @@ -50,14 +50,14 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { const diagnosticsAccumulator = diagnosticsEnabled ? diagnostics.DiagnosticsAccumulator(new Date().getTime()) : null; const diagnosticsManager = diagnosticsEnabled ? diagnostics.DiagnosticsManager( - platform, - persistentStorage, - diagnosticsAccumulator, - eventSender, - environment, - options, - diagnosticId - ) + platform, + persistentStorage, + diagnosticsAccumulator, + eventSender, + environment, + options, + diagnosticId + ) : null; const stream = Stream(platform, options, environment, diagnosticsAccumulator); @@ -430,7 +430,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } }; stream.connect(ident.getContext(), hash, { - ping: function () { + ping: function() { logger.debug(messages.debugStreamPing()); const contextAtTimeOfPingEvent = ident.getContext(); requestor @@ -446,7 +446,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { emitter.maybeReportError(new errors.LDFlagFetchError(messages.errorFetchingFlags(err))); }); }, - put: function (e) { + put: function(e) { const data = tryParseData(e.data); if (!data) { return; @@ -456,7 +456,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { // Don't wait for this Promise to be resolved; note that replaceAllFlags is guaranteed // never to have an unhandled rejection }, - patch: function (e) { + patch: function(e) { const data = tryParseData(e.data); if (!data) { return; @@ -483,7 +483,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { logger.debug(messages.debugStreamPatchIgnored(data.key)); } }, - delete: function (e) { + delete: function(e) { const data = tryParseData(e.data); if (!data) { return; @@ -540,7 +540,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { notifyInspectionFlagsChanged(); - return handleFlagChanges(changes).catch(() => { }); // swallow any exceptions from this Promise + return handleFlagChanges(changes).catch(() => {}); // swallow any exceptions from this Promise } // Returns a Promise which will be resolved when we have dispatched all change events and updated @@ -796,8 +796,8 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { if (timeout > highTimeoutThreshold) { logger.warn( 'The waitForInitialization function was called with a timeout greater than ' + - `${highTimeoutThreshold} seconds. We recommend a timeout of ` + - `${highTimeoutThreshold} seconds or less.` + `${highTimeoutThreshold} seconds. We recommend a timeout of ` + + `${highTimeoutThreshold} seconds or less.` ); } @@ -821,7 +821,7 @@ function initialize(env, context, specifiedOptions, platform, extraOptionDefs) { } logger.warn( 'The waitForInitialization function was called without a timeout specified.' + - ' In a future version a default timeout will be applied.' + ' In a future version a default timeout will be applied.' ); return initializationStateTracker.getInitializationPromise(); }