feat!: Only support identify with result. (#1000)

Originally the SDK API had an `identify` method which didn't return any
result. We added an `identifyResult` which is a safer API.

We have decided that we shouldn't support the less-safe API and instead
should have a single identify method. So the regular identify method
will now have the behavior of `identifyResult`.

This moves the browser client to use a PIMPL implementation to decouple
its interface of the inheritance hierarchy. Which allows for identify to
have a completely different signature than the base implementation. It
also allows us to retain identify in RN until we are ready to do a major
version.

For the PIMPL implementation I decided to use a factory instead of a
class. So tests needed updated to use that factory.

I also re-organized plugin registration so the better encapsulate PIMPL
implementation would be passed to register.

The "compat" adapter needed to be updated to use the new API. For now it
supports using the old interface, but we may want to consider removing
it if we determine it isn't safe.


<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> Replaces the BrowserClient class with a makeClient factory using a
PIMPL impl and changes identify to return LDIdentifyResult (replacing
identifyResult); updates plugin registration, compat, initialization,
and tests.
> 
> - **API**
> - `identify` now returns `LDIdentifyResult` (with statuses like
`completed/shed/timeout/error`) and accepts browser-specific options;
`identifyResult` is removed from the public interface.
> - Introduce factory `makeClient` wrapping an internal
`BrowserClientImpl` (PIMPL); `initialize` now uses `makeClient`.
> - `LDClient` type updated to expose the new `identify` signature and
omit the old one.
> - **Plugins**
> - Refactor plugin registration to register against the public PIMPL
client; hook retrieval unchanged; environment metadata still forwarded.
> - **Compat**
> - `LDClientCompatImpl` updated to use `makeClient` and new `identify`
result handling; passes `sheddable: false`, propagates `bootstrap/hash`,
and maps failures/timeouts.
> - **Tests**
> - Migrate tests to `makeClient`; adjust expectations for `identify`
return values and shedding behavior; verify plugin registration and
environment metadata.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
4df7885. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Author: kinyoklion

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

@@ -6,7 +6,7 @@ import {
   LDLogger,
 } from '@launchdarkly/js-client-sdk-common';
 
-import { BrowserClient } from '../src/BrowserClient';
+import { makeClient } from '../src/BrowserClient';
 import { LDPlugin } from '../src/LDPlugin';
 import { BrowserOptions } from '../src/options';
 import { makeBasicPlatform } from './BrowserClient.mocks';
@@ -41,7 +41,7 @@ it('registers plugins and executes hooks during initialization', async () => {
 
   const platform = makeBasicPlatform();
 
-  const client = new BrowserClient(
+  const client = makeClient(
     'client-side-id',
     AutoEnvAttributes.Disabled,
     {
@@ -132,7 +132,7 @@ it('registers multiple plugins and executes all hooks', async () => {
 
   const platform = makeBasicPlatform();
 
-  const client = new BrowserClient(
+  const client = makeClient(
     'client-side-id',
     AutoEnvAttributes.Disabled,
     {
@@ -198,8 +198,7 @@ it('passes correct environmentMetadata to plugin getHooks and register functions
 
   const platform = makeBasicPlatform(options);
 
-  // eslint-disable-next-line no-new
-  new BrowserClient(
+  makeClient(
     'client-side-id',
     AutoEnvAttributes.Disabled,
     {
@@ -280,8 +279,7 @@ it('passes correct environmentMetadata without optional fields', async () => {
 
   const platform = makeBasicPlatform();
 
-  // eslint-disable-next-line no-new
-  new BrowserClient(
+  makeClient(
     'client-side-id',
     AutoEnvAttributes.Disabled,
     {

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

@@ -4,7 +4,7 @@ import {
   LDSingleKindContext,
 } from '@launchdarkly/js-client-sdk-common';
 
-import { BrowserClient } from '../src/BrowserClient';
+import { makeClient } from '../src/BrowserClient';
 import { makeBasicPlatform } from './BrowserClient.mocks';
 import { goodBootstrapDataWithReasons } from './testBootstrapData';
 
@@ -27,7 +27,7 @@ describe('given a mock platform for a BrowserClient', () => {
   });
 
   it('includes urls in custom events', async () => {
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -56,7 +56,7 @@ describe('given a mock platform for a BrowserClient', () => {
   });
 
   it('can filter URLs in custom events', async () => {
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -90,7 +90,7 @@ describe('given a mock platform for a BrowserClient', () => {
   });
 
   it('can filter URLs in click events', async () => {
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -133,7 +133,7 @@ describe('given a mock platform for a BrowserClient', () => {
   });
 
   it('can filter URLs in pageview events', async () => {
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -163,7 +163,7 @@ describe('given a mock platform for a BrowserClient', () => {
   });
 
   it('can use bootstrap data', async () => {
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -189,17 +189,17 @@ describe('given a mock platform for a BrowserClient', () => {
     });
   });
 
-  it('can shed intermediate identifyResult calls', async () => {
-    const client = new BrowserClient(
+  it('can shed intermediate identify calls', async () => {
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       { streaming: false, logger, diagnosticOptOut: true, sendEvents: false, fetchGoals: false },
       platform,
     );
 
-    const promise1 = client.identifyResult({ key: 'user-key-1', kind: 'user' });
-    const promise2 = client.identifyResult({ key: 'user-key-2', kind: 'user' });
-    const promise3 = client.identifyResult({ key: 'user-key-3', kind: 'user' });
+    const promise1 = client.identify({ key: 'user-key-1', kind: 'user' });
+    const promise2 = client.identify({ key: 'user-key-2', kind: 'user' });
+    const promise3 = client.identify({ key: 'user-key-3', kind: 'user' });
 
     const [result1, result2, result3] = await Promise.all([promise1, promise2, promise3]);
 
@@ -212,7 +212,7 @@ describe('given a mock platform for a BrowserClient', () => {
 
   it('calls beforeIdentify in order', async () => {
     const order: string[] = [];
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -250,7 +250,7 @@ describe('given a mock platform for a BrowserClient', () => {
 
   it('completes identify calls in order', async () => {
     const order: string[] = [];
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -292,7 +292,7 @@ describe('given a mock platform for a BrowserClient', () => {
 
   it('completes awaited identify calls in order without shedding', async () => {
     const order: string[] = [];
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       {
@@ -323,9 +323,9 @@ describe('given a mock platform for a BrowserClient', () => {
       platform,
     );
 
-    const result1 = await client.identifyResult({ key: 'user-key-1', kind: 'user' });
-    const result2 = await client.identifyResult({ key: 'user-key-2', kind: 'user' });
-    const result3 = await client.identifyResult({ key: 'user-key-3', kind: 'user' });
+    const result1 = await client.identify({ key: 'user-key-1', kind: 'user' });
+    const result2 = await client.identify({ key: 'user-key-2', kind: 'user' });
+    const result3 = await client.identify({ key: 'user-key-3', kind: 'user' });
 
     expect(result1.status).toEqual('completed');
     expect(result2.status).toEqual('completed');
@@ -335,8 +335,8 @@ describe('given a mock platform for a BrowserClient', () => {
     expect(order).toEqual(['user-key-1', 'user-key-2', 'user-key-3']);
   });
 
-  it('can shed intermediate identify calls', async () => {
-    const client = new BrowserClient(
+  it('can shed intermediate identify calls without waiting for results', async () => {
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       { streaming: false, logger, diagnosticOptOut: true, sendEvents: false, fetchGoals: false },
@@ -354,25 +354,16 @@ describe('given a mock platform for a BrowserClient', () => {
   });
 
   it('it does not shed non-shedable identify calls', async () => {
-    const client = new BrowserClient(
+    const client = makeClient(
       'client-side-id',
       AutoEnvAttributes.Disabled,
       { streaming: false, logger, diagnosticOptOut: true, sendEvents: false, fetchGoals: false },
       platform,
     );
 
-    const promise1 = client.identifyResult(
-      { key: 'user-key-1', kind: 'user' },
-      { sheddable: false },
-    );
-    const promise2 = client.identifyResult(
-      { key: 'user-key-2', kind: 'user' },
-      { sheddable: false },
-    );
-    const promise3 = client.identifyResult(
-      { key: 'user-key-3', kind: 'user' },
-      { sheddable: false },
-    );
+    const promise1 = client.identify({ key: 'user-key-1', kind: 'user' }, { sheddable: false });
+    const promise2 = client.identify({ key: 'user-key-2', kind: 'user' }, { sheddable: false });
+    const promise3 = client.identify({ key: 'user-key-3', kind: 'user' }, { sheddable: false });
 
     const [result1, result2, result3] = await Promise.all([promise1, promise2, promise3]);
 

packages/sdk/browser/__tests__/compat/LDClientCompatImpl.test.ts

@@ -2,20 +2,21 @@ import { jest } from '@jest/globals';
 
 import { LDContext, LDFlagSet } from '@launchdarkly/js-client-sdk-common';
 
-import { BrowserClient } from '../../src/BrowserClient';
+// Import after mocking
+import { makeClient } from '../../src/BrowserClient';
 import LDClientCompatImpl from '../../src/compat/LDClientCompatImpl';
 import { LDOptions } from '../../src/compat/LDCompatOptions';
+import { LDClient } from '../../src/LDClient';
 
 // @ts-ignore
-const mockBrowserClient: jest.MockedObject<BrowserClient> = {
+const mockBrowserClient: jest.MockedObject<LDClient> = {
   identify: jest.fn(),
   allFlags: jest.fn(),
   close: jest.fn(),
   flush: jest.fn(),
   setStreaming: jest.fn(),
   on: jest.fn(),
   off: jest.fn(),
-  sdkKey: 'test-sdk-key',
   variation: jest.fn(),
   variationDetail: jest.fn(),
   boolVariation: jest.fn(),
@@ -39,36 +40,37 @@ const mockBrowserClient: jest.MockedObject<BrowserClient> = {
 
 jest.mock('../../src/BrowserClient', () => ({
   __esModule: true,
-  BrowserClient: jest.fn(() => mockBrowserClient),
+  makeClient: jest.fn(),
 }));
 
+const mockMakeClient = makeClient as jest.MockedFunction<typeof makeClient>;
+
 afterEach(() => {
-  jest.resetAllMocks();
+  jest.clearAllMocks();
 });
 
 beforeEach(() => {
-  // TypesScript doesn't understand that the BrowserClient is a mock.
-  // @ts-ignore
-  BrowserClient.mockImplementation(() => mockBrowserClient);
+  // Restore the mock implementation after clearing
+  mockMakeClient.mockReturnValue(mockBrowserClient);
 });
 
 describe('given a LDClientCompatImpl client with mocked browser client that is immediately ready', () => {
   let client: LDClientCompatImpl;
 
   beforeEach(() => {
     jest.useFakeTimers();
-    mockBrowserClient.identify.mockImplementation(() => Promise.resolve());
+    mockBrowserClient.identify.mockImplementation(() => Promise.resolve({ status: 'completed' }));
     client = new LDClientCompatImpl('env-key', { kind: 'user', key: 'user-key' });
   });
 
   it('should resolve waitForInitialization when the client is already initialized', async () => {
     jest.advanceTimersToNextTimer();
-    mockBrowserClient.identify.mockResolvedValue(undefined);
+    mockBrowserClient.identify.mockResolvedValue({ status: 'completed' });
 
     await expect(client.waitForInitialization()).resolves.toBeUndefined();
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 });
@@ -81,7 +83,7 @@ describe('given a LDClientCompatImpl client with mocked browser client that init
     mockBrowserClient.identify.mockImplementation(
       () =>
         new Promise((r) => {
-          setTimeout(r, 100);
+          setTimeout(() => r({ status: 'completed' }), 100);
         }),
     );
     client = new LDClientCompatImpl('env-key', { kind: 'user', key: 'user-key' });
@@ -92,12 +94,15 @@ describe('given a LDClientCompatImpl client with mocked browser client that init
     const context: LDContext = { kind: 'user', key: 'new-user' };
     const mockFlags: LDFlagSet = { flag1: true, flag2: false };
 
-    mockBrowserClient.identify.mockResolvedValue();
+    mockBrowserClient.identify.mockResolvedValue({ status: 'completed' });
     mockBrowserClient.allFlags.mockReturnValue(mockFlags);
 
     const result = await client.identify(context);
 
-    expect(mockBrowserClient.identify).toHaveBeenCalledWith(context, { hash: undefined });
+    expect(mockBrowserClient.identify).toHaveBeenCalledWith(context, {
+      hash: undefined,
+      sheddable: false,
+    });
     expect(result).toEqual(mockFlags);
   });
 
@@ -107,7 +112,7 @@ describe('given a LDClientCompatImpl client with mocked browser client that init
     const mockFlags: LDFlagSet = { flag1: true, flag2: false };
 
     mockBrowserClient.allFlags.mockReturnValue(mockFlags);
-    mockBrowserClient.identify.mockImplementation(() => Promise.resolve());
+    mockBrowserClient.identify.mockImplementation(() => Promise.resolve({ status: 'completed' }));
     // Starting advancing the timers for the nest call. The wrapped promises
     // do not resolve sychronously.
     jest.advanceTimersToNextTimerAsync();
@@ -161,35 +166,35 @@ describe('given a LDClientCompatImpl client with mocked browser client that init
 
   it('should resolve waitForInitialization when the client is initialized', async () => {
     jest.advanceTimersToNextTimer();
-    mockBrowserClient.identify.mockResolvedValue(undefined);
+    mockBrowserClient.identify.mockResolvedValue({ status: 'completed' });
 
     await expect(client.waitForInitialization()).resolves.toBeUndefined();
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 
   it('should resolve second waitForInitialization immediately', async () => {
     jest.advanceTimersToNextTimer();
-    mockBrowserClient.identify.mockResolvedValue(undefined);
+    mockBrowserClient.identify.mockResolvedValue({ status: 'completed' });
 
     await expect(client.waitForInitialization()).resolves.toBeUndefined();
     await expect(client.waitForInitialization()).resolves.toBeUndefined();
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 
   it('should resolve waitUntilReady immediately if the client is already initialized', async () => {
     jest.advanceTimersToNextTimer();
-    mockBrowserClient.identify.mockResolvedValue(undefined);
+    mockBrowserClient.identify.mockResolvedValue({ status: 'completed' });
 
     await expect(client.waitUntilReady()).resolves.toBeUndefined();
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 
@@ -427,6 +432,7 @@ it('forwards bootstrap and hash to BrowserClient identify call', async () => {
     bootstrap: bootstrapData,
     hash: 'someHash',
     noTimeout: true,
+    sheddable: false,
   });
 });
 
@@ -451,7 +457,7 @@ describe('given a LDClientCompatImpl client with mocked browser client which fai
 
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 
@@ -464,7 +470,7 @@ describe('given a LDClientCompatImpl client with mocked browser client which fai
 
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 
@@ -475,7 +481,7 @@ describe('given a LDClientCompatImpl client with mocked browser client which fai
 
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });
 
@@ -487,7 +493,7 @@ describe('given a LDClientCompatImpl client with mocked browser client which fai
 
     expect(mockBrowserClient.identify).toHaveBeenCalledWith(
       { kind: 'user', key: 'user-key' },
-      { bootstrap: undefined, hash: undefined, noTimeout: true },
+      { bootstrap: undefined, hash: undefined, noTimeout: true, sheddable: false },
     );
   });