-
Notifications
You must be signed in to change notification settings - Fork 31
refactor!: align browser v4 intialization flow to specs #1040
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
Conversation
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
ee357a3 to
b00509f
Compare
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.
Bug: Compat layer bootstrap data now silently ignored
The removal of bootstrap handling from identifyResult breaks the compat layer's bootstrap functionality. LDClientCompatImpl._initIdentify passes bootstrap to this._client.identify(), which maps to identifyResult. However, identifyResult no longer processes the bootstrap option (that code was moved to start() only). This causes bootstrap data passed through the compat API to be silently ignored, breaking backwards compatibility for compat layer users who rely on bootstrap.
packages/sdk/browser/src/BrowserClient.ts#L274-L285
js-core/packages/sdk/browser/src/BrowserClient.ts
Lines 274 to 285 in b00509f
| const res = await super.identifyResult(context, identifyOptionsWithUpdatedDefaults); | |
| if (res.status === 'completed') { | |
| this._initializeResult = { status: 'complete' }; | |
| this._initResolve?.(this._initializeResult); | |
| } else if (res.status === 'error') { | |
| this._initializeResult = { status: 'failed', error: res.error }; | |
| this._initResolve?.(this._initializeResult); | |
| } | |
| this._goalManager?.startTracking(); | |
| return res; |
packages/sdk/browser/src/LDClient.ts
Outdated
| /** | ||
| * Optional identify options to use for the identify operation. {@link LDIdentifyOptions} | ||
| */ | ||
| identifyOptions?: LDIdentifyOptions; |
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.
So, bootstrap would be in start instead of in the options.
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.
So I wasn't sure if bootstrap data is something users will typically tie with their context? If not then 100% should be in start function.
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.
Added the bootstrap option in start, I kept the additional bootstrap option in case it is more natural for users to tie a certain set of identify options to a context
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.
Bug: Compat layer calls identify before start, breaking initialization
The compat layer's _initIdentify method calls this._client.identify() directly without calling start() first. However, the new identifyResult implementation now requires _startPromise to be set before identify can be called, otherwise it returns { status: 'error', error: new Error('Identify called before start') }. Since the compat layer never calls start(), all initialization attempts through the compat layer will fail immediately. The unit tests don't catch this because they mock identify to return success without exercising the real BrowserClient code. The compat layer needs to call start() with the bootstrap/hash options instead of calling identify() directly.
packages/sdk/browser/src/compat/LDClientCompatImpl.ts#L47-L62
js-core/packages/sdk/browser/src/compat/LDClientCompatImpl.ts
Lines 47 to 62 in 437b864
| this.logger = this._client.logger; | |
| this._initIdentify(context, bootstrap, hash); | |
| } | |
| private async _initIdentify( | |
| context: LDContext, | |
| bootstrap?: LDFlagSet, | |
| hash?: string, | |
| ): Promise<void> { | |
| try { | |
| const result = await this._client.identify(context, { | |
| noTimeout: true, | |
| bootstrap, | |
| hash, | |
| sheddable: false, | |
| }); |
packages/sdk/browser/src/BrowserClient.ts#L233-L239
js-core/packages/sdk/browser/src/BrowserClient.ts
Lines 233 to 239 in 437b864
| ): Promise<LDIdentifyResult> { | |
| if (!this._startPromise) { | |
| this.logger.error( | |
| 'Client must be started before it can identify a context, did you forget to call start()?', | |
| ); | |
| return { status: 'error', error: new Error('Identify called before start') }; | |
| } |
437b864 to
ad18342
Compare
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.
Bug: Compat layer fails because identify requires start first
The compat layer's _initIdentify method calls this._client.identify() without first calling start(). However, the new identifyResult implementation in BrowserClient now checks if _startPromise is set and returns an error status if not. Since the compat layer never calls start(), every initialization via the compat path will fail with "Identify called before start" error. The compat layer needs to call start() instead of identify() for its initial identification, or the identifyResult check needs to allow calls from the compat path.
packages/sdk/browser/src/compat/LDClientCompatImpl.ts#L56-L62
js-core/packages/sdk/browser/src/compat/LDClientCompatImpl.ts
Lines 56 to 62 in ad18342
| try { | |
| const result = await this._client.identify(context, { | |
| noTimeout: true, | |
| bootstrap, | |
| hash, | |
| sheddable: false, | |
| }); |
packages/sdk/browser/src/BrowserClient.ts#L233-L238
js-core/packages/sdk/browser/src/BrowserClient.ts
Lines 233 to 238 in ad18342
| ): Promise<LDIdentifyResult> { | |
| if (!this._startPromise) { | |
| this.logger.error( | |
| 'Client must be started before it can identify a context, did you forget to call start()?', | |
| ); | |
| return { status: 'error', error: new Error('Identify called before start') }; |
| bootstrap, | ||
| hash, | ||
| }, | ||
| }); |
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.
Bug: Missing sheddable:false allows initial identify to be discarded
The compat layer's initial identify no longer passes sheddable: false. The old code explicitly set sheddable: false when calling identify(), but the new start() call doesn't include it. Since identifyResult() defaults sheddable to true when undefined, the initial identify can now be discarded if a user immediately calls identify() on the returned client. The comment on line 69 even states 'shed' cannot happen with sheddable: false, but this invariant no longer holds.
3f446dd to
b5172cd
Compare
- Introduced `start` method in `BrowserClient` to handle client initialization with context. - Replaced direct calls to `identify` with `setInitialContext` and `start` for better context management. - Updated example app to reflect changes in client initialization and context handling. - Added tests to ensure proper functionality of new initialization flow.
- Implemented a test to verify that multiple calls to the `start` method of `BrowserClient` return the same promise and resolve to the same result. - Ensured that only one identify call is made during the initialization process, confirming the promise caching behavior. - Fix contract tests to use the new initialization method
- require `start` to be called before additional `identify`
b5172cd to
97e9856
Compare
- Added logic to use bootstrap data from start options if not provided in identify options. - Updated LDStartOptions interface to include an optional bootstrap property for identify operations.
To align the api to more of what it does.
| // start the client, then immediately kick off an identify operation | ||
| // in order to preserve the behavior of the previous SDK. | ||
| this._client.start(); | ||
| this._initIdentify(context, bootstrap, hash); |
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.
Bug: Compat layer triggers duplicate non-sheddable identify operations
The compat layer calls start() on line 51, which internally calls identifyResult with sheddable: false for the initial context. Then it immediately calls _initIdentify() on line 52, which calls identify() also with sheddable: false for the same context. Since both operations are non-sheddable, neither can be discarded, resulting in two full network requests to LaunchDarkly for the same context. This is a regression from the previous SDK behavior where only one identify operation occurred.
Additional Locations (1)
🤖 I have created a release *beep* *boop* --- <details><summary>browser-telemetry: 1.0.15</summary> ## [1.0.15](browser-telemetry-v1.0.14...browser-telemetry-v1.0.15) (2025-12-18) ### Dependencies * The following workspace dependencies were updated * devDependencies * @launchdarkly/js-client-sdk bumped from 0.10.0 to 0.11.0 </details> <details><summary>jest: 0.1.15</summary> ## [0.1.15](jest-v0.1.14...jest-v0.1.15) (2025-12-18) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/react-native-client-sdk bumped from ~10.12.2 to ~10.12.3 </details> <details><summary>js-client-sdk: 0.11.0</summary> ## [0.11.0](js-client-sdk-v0.10.0...js-client-sdk-v0.11.0) (2025-12-18) ### ⚠ BREAKING CHANGES * align browser v4 intialization flow to specs ([#1040](#1040)) ### Features * adding support for debug override plugins ([#1033](#1033)) ([17f5e7d](17f5e7d)) * allow clients to evaluate bootstrapped flags when not ready ([#1036](#1036)) ([9b4542a](9b4542a)) * implement `waitForInitialization` for browser sdk 4.x ([#1028](#1028)) ([156532a](156532a)) ### Code Refactoring * align browser v4 intialization flow to specs ([#1040](#1040)) ([eff6a55](eff6a55)) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-client-sdk-common bumped from 1.15.2 to 1.16.0 </details> <details><summary>js-client-sdk-common: 1.16.0</summary> ## [1.16.0](js-client-sdk-common-v1.15.2...js-client-sdk-common-v1.16.0) (2025-12-18) ### Features * adding support for debug override plugins ([#1033](#1033)) ([17f5e7d](17f5e7d)) * allow clients to evaluate bootstrapped flags when not ready ([#1036](#1036)) ([9b4542a](9b4542a)) * implement `waitForInitialization` for browser sdk 4.x ([#1028](#1028)) ([156532a](156532a)) </details> <details><summary>react-native-client-sdk: 10.12.3</summary> ## [10.12.3](react-native-client-sdk-v10.12.2...react-native-client-sdk-v10.12.3) (2025-12-18) ### Dependencies * The following workspace dependencies were updated * dependencies * @launchdarkly/js-client-sdk-common bumped from 1.15.2 to 1.16.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Release browser SDK 0.11.0 (breaking init flow, new features) with related bumps to common 1.16.0, React Native 10.12.3, telemetry 1.0.15, and jest 0.1.15. > > - **SDK Releases** > - **Browser (`@launchdarkly/js-client-sdk` 0.11.0)**: > - Breaking: align v4 initialization flow to specs. > - Features: debug override plugins, evaluate bootstrapped flags when not ready, `waitForInitialization`. > - Deps: `@launchdarkly/js-client-sdk-common` → `1.16.0`. > - Version metadata updated in `packages/sdk/browser/src/platform/BrowserInfo.ts`. > - **Common (`@launchdarkly/js-client-sdk-common` 1.16.0)**: > - Features: debug override plugins, bootstrapped flag evaluation pre-ready, `waitForInitialization` support. > - **React Native (`@launchdarkly/react-native-client-sdk` 10.12.3)** > - Deps: `@launchdarkly/js-client-sdk-common` → `1.16.0`. > - Version metadata updated in `packages/sdk/react-native/src/platform/PlatformInfo.ts`. > - **Telemetry (`@launchdarkly/browser-telemetry` 1.0.15)** > - DevDeps: `@launchdarkly/js-client-sdk` → `0.11.0`. > - **Tooling (`@launchdarkly/jest` 0.1.15)** > - Deps: `@launchdarkly/react-native-client-sdk` → `~10.12.3`. > - **Manifest** > - `.release-please-manifest.json` updated to new versions across affected packages. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 8978f79. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Requirements
Related issues
sdk-1667
Describe the solution you've provided
startmethod inBrowserClientto handle client initialization.setInitialContextandstartfor better context management.Additional context
Note
Adds an explicit client start() flow and new createClient API (with required initial context), updates initialization/identify behavior, and refreshes tests, examples, and compat layer accordingly.
LDClient.start(options?)returningLDWaitForInitializationResult; cache promise; supportbootstrapviastart/identifyOptions.initialize(...)withcreateClient(clientSideId, context, options);makeClientnow requires an initialLDContext.identifybeforestart(returns{ status: 'error' }and logs); initialidentifyis non-sheddable.waitForInitializationreturns{ status: 'complete' | 'failed' | 'timeout' }(note: some tests updated fromerror→failed).LDWaitForInitializationOptions.timeoutoptional; addLDStartOptions._initialContext,_startPromise,_initializedPromise; preset flags frombootstrapbefore identify./compat):initialize(envKey, context, options)now builds viamakeClient(envKey, context, ...); immediatelystart()then performidentifyto mirror old behavior; re-export unchanged surface minusstart/setInitialContext.createClient(..., context)andclient.start()+waitForInitialization().start()and required initial context; add cases for start idempotency, shedding order, bootstrap pre-eval, URL filtering, waitForInitialization timing/failure.Written by Cursor Bugbot for commit 846ae61. This will update automatically on new commits. Configure here.