Skip to content

Commit 7f5f468

Browse files
joker23kinyoklion
andauthored
feat: adding start() method to common client sdk package (#1244)
This PR will consolidate the `start()` method into the shared client sdk package. To do this we also: - introduce an internal option called `requiresStart` which should be `true` for new SDKs (and only RN has it default `false`) - `requiresStart` is, effectively, a feature flag for whether the SDK is using the new `create` + `start` initialization pattern - consolidated the unit tests for the `start` method into client common - moved `start` method out of browser and electron sdks <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches core initialization/identify behavior across multiple SDKs and changes when bootstrap parsing and goal tracking occur, which could affect startup sequencing and existing integrations if edge cases regress. > > **Overview** > Moves the `start()` implementation (including bootstrap flag preloading and init promise caching/timeout behavior) from the Browser and Electron SDKs into the shared `packages/shared/sdk-client` `LDClientImpl`. > > Introduces `LDStartOptions` in the shared API and adds internal options `requiresStart` and `initialContext` so platform SDKs can enforce the new create+`start()` initialization pattern; `identifyResult()` now blocks pre-`start()` calls when `requiresStart` is enabled and updates the logged error message. > > Updates Browser/Electron wrappers and tests accordingly (Browser/Electron remove their custom `start()` logic, Browser ensures goal tracking only starts after `start()`), and adds a consolidated `LDClientImpl.start()` test suite in the shared package. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 5c9e61f. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/launchdarkly/js-core/pull/1244" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> --------- Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
1 parent 7bf3c31 commit 7f5f468

12 files changed

Lines changed: 479 additions & 230 deletions

File tree

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

Lines changed: 1 addition & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -246,37 +246,6 @@ describe('given a mock platform for a BrowserClient', () => {
246246
expect(client.getContext()).toEqual({ kind: 'user', key: 'bob' });
247247
});
248248

249-
it('parses bootstrap data only once when using start()', async () => {
250-
const bootstrapModule = await import('@launchdarkly/js-client-sdk-common');
251-
const readFlagsFromBootstrapSpy = jest.spyOn(bootstrapModule, 'readFlagsFromBootstrap');
252-
253-
const client = makeClient(
254-
'client-side-id',
255-
{ kind: 'user', key: 'bob' },
256-
AutoEnvAttributes.Disabled,
257-
{
258-
streaming: false,
259-
logger,
260-
diagnosticOptOut: true,
261-
},
262-
platform,
263-
);
264-
265-
await client.start({
266-
identifyOptions: {
267-
bootstrap: goodBootstrapDataWithReasons,
268-
},
269-
});
270-
271-
expect(readFlagsFromBootstrapSpy).toHaveBeenCalledTimes(1);
272-
expect(readFlagsFromBootstrapSpy).toHaveBeenCalledWith(
273-
expect.anything(),
274-
goodBootstrapDataWithReasons,
275-
);
276-
277-
readFlagsFromBootstrapSpy.mockRestore();
278-
});
279-
280249
it('uses the latest bootstrap data when identify is called with new bootstrap data', async () => {
281250
const initialBootstrapData = {
282251
'string-flag': 'is bob',
@@ -866,7 +835,7 @@ describe('given a mock platform for a BrowserClient', () => {
866835

867836
// Verify that the logger was called with the error message
868837
expect(logger.error).toHaveBeenCalledWith(
869-
'Client must be started before it can identify a context, did you forget to call start()?',
838+
'The client must be started before a context can be identified. Call start() prior to identifying a context.',
870839
);
871840

872841
// Verify that no fetch calls were made

packages/sdk/browser/src/BrowserClient.ts

Lines changed: 18 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,8 @@ import {
2121
LDIdentifyResult,
2222
LDPluginEnvironmentMetadata,
2323
LDWaitForInitializationOptions,
24-
LDWaitForInitializationResult,
2524
MODE_TABLE,
2625
Platform,
27-
readFlagsFromBootstrap,
2826
resolveForegroundMode,
2927
safeRegisterDebugOverridePlugins,
3028
} from '@launchdarkly/js-client-sdk-common';
@@ -45,13 +43,9 @@ class BrowserClientImpl extends LDClientImpl {
4543
private readonly _goalManager?: GoalManager;
4644
private readonly _plugins?: LDPlugin[];
4745

48-
private _initialContext?: LDContext;
49-
50-
// NOTE: This also keeps track of when we tried to initialize the client.
51-
private _startPromise?: Promise<LDWaitForInitializationResult>;
52-
5346
constructor(
5447
clientSideId: string,
48+
initialContext: LDContext,
5549
autoEnvAttributes: AutoEnvAttributes,
5650
options: BrowserOptions = {},
5751
overridePlatform?: Platform,
@@ -156,6 +150,8 @@ class BrowserClientImpl extends LDClientImpl {
156150
getImplementationHooks: (environmentMetadata: LDPluginEnvironmentMetadata) =>
157151
internal.safeGetHooks(logger, environmentMetadata, validatedBrowserOptions.plugins),
158152
credentialType: 'clientSideId',
153+
requiresStart: true,
154+
initialContext,
159155
});
160156

161157
this.setEventSendingEnabled(true, false);
@@ -234,10 +230,6 @@ class BrowserClientImpl extends LDClientImpl {
234230
}
235231
}
236232

237-
setInitialContext(context: LDContext): void {
238-
this._initialContext = context;
239-
}
240-
241233
override async identify(context: LDContext, identifyOptions?: LDIdentifyOptions): Promise<void> {
242234
return super.identify(context, identifyOptions);
243235
}
@@ -246,79 +238,18 @@ class BrowserClientImpl extends LDClientImpl {
246238
context: LDContext,
247239
identifyOptions?: LDIdentifyOptions,
248240
): Promise<LDIdentifyResult> {
249-
if (!this._startPromise) {
250-
this.logger.error(
251-
'Client must be started before it can identify a context, did you forget to call start()?',
252-
);
253-
return { status: 'error', error: new Error('Identify called before start') };
254-
}
255-
256-
const identifyOptionsWithUpdatedDefaults = {
257-
...identifyOptions,
258-
};
259-
if (identifyOptions?.sheddable === undefined) {
260-
identifyOptionsWithUpdatedDefaults.sheddable = true;
241+
const options =
242+
identifyOptions?.sheddable === undefined
243+
? { ...identifyOptions, sheddable: true }
244+
: identifyOptions;
245+
const res = await super.identifyResult(context, options);
246+
// Ensure that we do not start the goal manager if start() is not called.
247+
if (this.startPromise) {
248+
this._goalManager?.startTracking();
261249
}
262-
263-
const res = await super.identifyResult(context, identifyOptionsWithUpdatedDefaults);
264-
265-
this._goalManager?.startTracking();
266250
return res;
267251
}
268252

269-
start(options?: LDStartOptions): Promise<LDWaitForInitializationResult> {
270-
if (this.initializeResult) {
271-
return Promise.resolve(this.initializeResult);
272-
}
273-
if (this._startPromise) {
274-
return this._startPromise;
275-
}
276-
if (!this._initialContext) {
277-
this.logger.error('Initial context not set');
278-
return Promise.resolve({ status: 'failed', error: new Error('Initial context not set') });
279-
}
280-
281-
// When we get to this point, we assume this is the first time that start is being
282-
// attempted. This line should only be called once during the lifetime of the client.
283-
const identifyOptions = {
284-
...(options?.identifyOptions ?? {}),
285-
286-
// Initial identify operations are not sheddable.
287-
sheddable: false,
288-
};
289-
290-
// If the bootstrap data is provided in the start options, and the identify options do not have bootstrap data,
291-
// then use the bootstrap data from the start options.
292-
if (options?.bootstrap && !identifyOptions.bootstrap) {
293-
identifyOptions.bootstrap = options.bootstrap;
294-
}
295-
296-
if (identifyOptions?.bootstrap) {
297-
try {
298-
if (!identifyOptions.bootstrapParsed) {
299-
identifyOptions.bootstrapParsed = readFlagsFromBootstrap(
300-
this.logger,
301-
identifyOptions.bootstrap,
302-
);
303-
}
304-
this.presetFlags(identifyOptions.bootstrapParsed!);
305-
} catch (error) {
306-
this.logger.error('Failed to bootstrap data', error);
307-
}
308-
}
309-
310-
if (!this.initializedPromise) {
311-
this.initializedPromise = new Promise((resolve) => {
312-
this.initResolve = resolve;
313-
});
314-
}
315-
316-
this._startPromise = this.promiseWithTimeout(this.initializedPromise!, options?.timeout ?? 5);
317-
318-
this.identifyResult(this._initialContext!, identifyOptions);
319-
return this._startPromise;
320-
}
321-
322253
setConnectionMode(mode?: FDv2ConnectionMode): void {
323254
if (!this.dataManager.setConnectionMode) {
324255
this.logger.warn(
@@ -366,8 +297,13 @@ export function makeClient(
366297
options: BrowserOptions = {},
367298
overridePlatform?: Platform,
368299
): LDClient {
369-
const impl = new BrowserClientImpl(clientSideId, autoEnvAttributes, options, overridePlatform);
370-
impl.setInitialContext(initialContext);
300+
const impl = new BrowserClientImpl(
301+
clientSideId,
302+
initialContext,
303+
autoEnvAttributes,
304+
options,
305+
overridePlatform,
306+
);
371307

372308
// Return a PIMPL style implementation. This decouples the interface from the interface of the implementation.
373309
// In the future we should consider updating the common SDK code to not use inheritance and instead compose

packages/sdk/browser/src/LDClient.ts

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import {
22
LDClient as CommonClient,
3+
LDStartOptions as CommonLDStartOptions,
34
FDv2ConnectionMode,
45
LDContext,
56
LDIdentifyResult,
@@ -9,18 +10,11 @@ import {
910

1011
import { BrowserIdentifyOptions as LDIdentifyOptions } from './BrowserIdentifyOptions';
1112

12-
export interface LDStartOptions extends LDWaitForInitializationOptions {
13-
/**
14-
* Optional bootstrap data to use for the identify operation. If {@link LDIdentifyOptions.bootstrap} is provided, it will be ignored.
15-
*/
16-
bootstrap?: unknown;
17-
18-
/**
19-
* Optional identify options to use for the identify operation. See {@link LDIdentifyOptions} for more information.
20-
*
21-
* @remarks
22-
* Since the first identify option should never be sheddable, we omit the sheddable option from the interface to avoid confusion.
23-
*/
13+
/**
14+
* Browser-specific start options that extend the common start options
15+
* with browser-specific identify options (see {@link LDIdentifyOptions}).
16+
*/
17+
export interface LDStartOptions extends CommonLDStartOptions {
2418
identifyOptions?: Omit<LDIdentifyOptions, 'sheddable'>;
2519
}
2620

packages/sdk/electron/__tests__/ElectronClient.test.ts

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -766,10 +766,7 @@ it('can use bootstrap data with identify', async () => {
766766
expect(mockedCreateEventSource).toHaveBeenCalled();
767767
});
768768

769-
it('parses bootstrap data only once when identify is called with bootstrap', async () => {
770-
const commonModule = await import('@launchdarkly/js-client-sdk-common');
771-
const readFlagsFromBootstrapSpy = jest.spyOn(commonModule, 'readFlagsFromBootstrap');
772-
769+
it('parses bootstrap data when start is called with bootstrap', async () => {
773770
(ElectronPlatform as jest.Mock).mockReturnValue({
774771
crypto: new ElectronCrypto(),
775772
info: new ElectronInfo(),
@@ -796,7 +793,8 @@ it('parses bootstrap data only once when identify is called with bootstrap', asy
796793

797794
await client.start({ bootstrap: goodBootstrapData });
798795

799-
expect(readFlagsFromBootstrapSpy).toHaveBeenCalledTimes(1);
800-
expect(readFlagsFromBootstrapSpy).toHaveBeenCalledWith(expect.anything(), goodBootstrapData);
801-
readFlagsFromBootstrapSpy.mockRestore();
796+
// Verify that bootstrap data was parsed and flags are available.
797+
expect(client.allFlags().killswitch).toBe(true);
798+
expect(client.allFlags()['string-flag']).toBe('is bob');
799+
expect(client.allFlags().cat).toBe(false);
802800
});

packages/sdk/electron/src/ElectronClient.ts

Lines changed: 8 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import {
2323
LDWaitForInitializationOptions,
2424
LDWaitForInitializationResult,
2525
mobileFdv1Endpoints,
26-
readFlagsFromBootstrap,
2726
} from '@launchdarkly/js-client-sdk-common';
2827

2928
import ElectronDataManager from './ElectronDataManager';
@@ -43,10 +42,6 @@ import ElectronPlatform from './platform/ElectronPlatform';
4342
const VALID_LOG_LEVELS: ReadonlySet<string> = new Set(['error', 'warn', 'info', 'debug']);
4443

4544
export class ElectronClient extends LDClientImpl {
46-
private readonly _initialContext: LDContext;
47-
48-
private _startPromise?: Promise<LDWaitForInitializationResult>;
49-
5045
private readonly _plugins: LDPlugin[];
5146

5247
private _ipcNamespace?: string;
@@ -93,6 +88,8 @@ export class ElectronClient extends LDClientImpl {
9388
getImplementationHooks: (_environmentMetadata: LDPluginEnvironmentMetadata) =>
9489
internal.safeGetHooks(logger, _environmentMetadata, validatedElectronOptions.plugins),
9590
credentialType: useClientSideId ? 'clientSideId' : 'mobileKey',
91+
requiresStart: true,
92+
initialContext,
9693
};
9794

9895
const platform = new ElectronPlatform(logger, options);
@@ -125,7 +122,6 @@ export class ElectronClient extends LDClientImpl {
125122
internalOptions,
126123
);
127124

128-
this._initialContext = initialContext;
129125
this._plugins = validatedElectronOptions.plugins;
130126
this.setEventSendingEnabled(!this.isOffline(), false);
131127

@@ -142,76 +138,15 @@ export class ElectronClient extends LDClientImpl {
142138
internal.safeRegisterPlugins(this.logger, this.environmentMetadata, client, this._plugins);
143139
}
144140

145-
start(options?: LDStartOptions): Promise<LDWaitForInitializationResult> {
146-
if (this.initializeResult !== undefined) {
147-
return Promise.resolve(this.initializeResult);
148-
}
149-
if (this._startPromise) {
150-
return this._startPromise;
151-
}
152-
if (!this._initialContext) {
153-
this.logger.error('Initial context not set');
154-
return Promise.resolve({ status: 'failed', error: new Error('Initial context not set') });
155-
}
156-
157-
const identifyOptions: LDIdentifyOptions = {
158-
...(options?.identifyOptions ?? {}),
159-
sheddable: false,
160-
};
161-
162-
if (
163-
options?.bootstrap !== undefined &&
164-
options?.bootstrap !== null &&
165-
!identifyOptions.bootstrap
166-
) {
167-
identifyOptions.bootstrap = options.bootstrap;
168-
}
169-
170-
if (identifyOptions.bootstrap) {
171-
try {
172-
if (!identifyOptions.bootstrapParsed) {
173-
identifyOptions.bootstrapParsed = readFlagsFromBootstrap(
174-
this.logger,
175-
identifyOptions.bootstrap,
176-
);
177-
}
178-
this.presetFlags(identifyOptions.bootstrapParsed!);
179-
} catch (error) {
180-
this.logger.error('Failed to bootstrap data', error);
181-
}
182-
}
183-
184-
if (!this.initializedPromise) {
185-
this.initializedPromise = new Promise((resolve) => {
186-
this.initResolve = resolve;
187-
});
188-
}
189-
190-
this._startPromise = this.promiseWithTimeout(this.initializedPromise!, options?.timeout ?? 5);
191-
192-
this.identifyResult(this._initialContext, identifyOptions);
193-
return this._startPromise;
194-
}
195-
196141
override async identifyResult(
197-
pristineContext: LDContext,
142+
context: LDContext,
198143
identifyOptions?: LDIdentifyOptions,
199144
): Promise<LDIdentifyResult> {
200-
if (!this._startPromise) {
201-
this.logger.error(
202-
'Client must be started before it can identify a context, did you forget to call start()?',
203-
);
204-
return { status: 'error', error: new Error('Identify called before start') };
205-
}
206-
207-
const identifyOptionsWithUpdatedDefaults = {
208-
...identifyOptions,
209-
};
210-
if (identifyOptions?.sheddable === undefined) {
211-
identifyOptionsWithUpdatedDefaults.sheddable = true;
212-
}
213-
214-
return super.identifyResult(pristineContext, identifyOptionsWithUpdatedDefaults);
145+
const options =
146+
identifyOptions?.sheddable === undefined
147+
? { ...identifyOptions, sheddable: true }
148+
: identifyOptions;
149+
return super.identifyResult(context, options);
215150
}
216151

217152
async setConnectionMode(mode: ConnectionMode): Promise<void> {

packages/sdk/electron/src/LDClient.ts

Lines changed: 2 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,11 @@ import type {
44
LDContext,
55
LDIdentifyOptions,
66
LDIdentifyResult,
7-
LDWaitForInitializationOptions,
7+
LDStartOptions,
88
LDWaitForInitializationResult,
99
} from '@launchdarkly/js-client-sdk-common';
1010

11-
export interface LDStartOptions extends LDWaitForInitializationOptions {
12-
/**
13-
* Optional bootstrap data to use for the identify operation. If
14-
* {@link LDIdentifyOptions.bootstrap} is provided in identifyOptions, it takes precedence.
15-
*/
16-
bootstrap?: unknown;
17-
18-
/**
19-
* Optional identify options to use for the first identify. Since the first identify is not
20-
* sheddable, the sheddable option is omitted from this type.
21-
*/
22-
identifyOptions?: Omit<LDIdentifyOptions, 'sheddable'>;
23-
}
11+
export type { LDStartOptions };
2412

2513
export interface LDClient extends Omit<LDClientBase, 'identify'> {
2614
/**

0 commit comments

Comments
 (0)