Skip to content
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

feat: Allow waiting for the network response on identify. #548

Merged
merged 5 commits into from
Aug 19, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 53 additions & 11 deletions packages/shared/sdk-client/src/LDClientImpl.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,6 @@ import * as mockResponseJson from './evaluation/mockResponse.json';
import LDClientImpl from './LDClientImpl';
import { Flags } from './types';

let mockPlatform: ReturnType<typeof createBasicPlatform>;
let logger: ReturnType<typeof createLogger>;

beforeEach(() => {
mockPlatform = createBasicPlatform();
logger = createLogger();
});

jest.mock('@launchdarkly/js-sdk-common', () => {
const actual = jest.requireActual('@launchdarkly/js-sdk-common');
const actualMock = jest.requireActual('@launchdarkly/private-js-mocks');
Expand Down Expand Up @@ -49,11 +41,15 @@ const autoEnv = {
os: { name: 'An OS', version: '1.0.1', family: 'orange' },
},
};
let ldc: LDClientImpl;
let defaultPutResponse: Flags;

describe('sdk-client object', () => {
let ldc: LDClientImpl;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decreasing the scope of these to the fixture.

let defaultPutResponse: Flags;
let mockPlatform: ReturnType<typeof createBasicPlatform>;
let logger: ReturnType<typeof createLogger>;

beforeEach(() => {
mockPlatform = createBasicPlatform();
logger = createLogger();
defaultPutResponse = clone<Flags>(mockResponseJson);
setupMockStreamingProcessor(false, defaultPutResponse);
mockPlatform.crypto.randomUUID.mockReturnValue('random1');
Expand Down Expand Up @@ -97,6 +93,8 @@ describe('sdk-client object', () => {
defaultPutResponse['dev-test-flag'].value = false;
const carContext: LDContext = { kind: 'car', key: 'test-car' };

mockPlatform.crypto.randomUUID.mockReturnValue('random1');

await ldc.identify(carContext);
const c = ldc.getContext();
const all = ldc.allFlags();
Expand Down Expand Up @@ -161,6 +159,8 @@ describe('sdk-client object', () => {
defaultPutResponse['dev-test-flag'].value = false;
const carContext: LDContext = { kind: 'car', anonymous: true, key: '' };

mockPlatform.crypto.randomUUID.mockReturnValue('random1');

await ldc.identify(carContext);
const c = ldc.getContext();
const all = ldc.allFlags();
Expand Down Expand Up @@ -207,4 +207,46 @@ describe('sdk-client object', () => {
expect(emitter.listenerCount('change')).toEqual(1);
expect(emitter.listenerCount('error')).toEqual(1);
});

test('can complete identification using storage', async () => {
const data: Record<string, string> = {};
mockPlatform.storage.get.mockImplementation((key) => data[key]);
mockPlatform.storage.set.mockImplementation((key: string, value: string) => {
data[key] = value;
});
mockPlatform.storage.clear.mockImplementation((key: string) => {
delete data[key];
});

// First identify should populate storage.
await ldc.identify(context);

expect(logger.debug).not.toHaveBeenCalledWith('Identify completing with cached flags');

// Second identify should use storage.
await ldc.identify(context);

expect(logger.debug).toHaveBeenCalledWith('Identify completing with cached flags');
});

test('does not complete identify using storage when instructed to wait for the network response', async () => {
const data: Record<string, string> = {};
mockPlatform.storage.get.mockImplementation((key) => data[key]);
mockPlatform.storage.set.mockImplementation((key: string, value: string) => {
data[key] = value;
});
mockPlatform.storage.clear.mockImplementation((key: string) => {
delete data[key];
});

// First identify should populate storage.
await ldc.identify(context);

expect(logger.debug).not.toHaveBeenCalledWith('Identify completing with cached flags');

// Second identify would use storage, but we instruct it not to.
await ldc.identify(context, { waitForNetworkResults: true, timeout: 5 });

expect(logger.debug).not.toHaveBeenCalledWith('Identify completing with cached flags');
});
});
18 changes: 14 additions & 4 deletions packages/shared/sdk-client/src/LDClientImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,6 @@ export default class LDClientImpl implements LDClient {
},
{},
);

await this.flagManager.init(context, descriptors).then(identifyResolve());
},
});
Expand Down Expand Up @@ -320,6 +319,9 @@ export default class LDClientImpl implements LDClient {
* 3. A network error is encountered during initialization.
*/
async identify(pristineContext: LDContext, identifyOptions?: LDIdentifyOptions): Promise<void> {
// In offline mode we do not support waiting for results.
const waitForNetworkResults = !!identifyOptions?.waitForNetworkResults && !this.isOffline();

if (identifyOptions?.timeout) {
this.identifyTimeout = identifyOptions.timeout;
}
Expand Down Expand Up @@ -354,15 +356,23 @@ export default class LDClientImpl implements LDClient {
this.logger.debug(`Identifying ${JSON.stringify(this.checkedContext)}`);

const loadedFromCache = await this.flagManager.loadCached(this.checkedContext);
if (loadedFromCache) {
if (loadedFromCache && !waitForNetworkResults) {
this.logger.debug('Identify completing with cached flags');
identifyResolve();
}
if (loadedFromCache && waitForNetworkResults) {
this.logger.debug(
'Identify - Flags loaded from cache, but identify was requested with "waitForNetworkResults"',
);
}

if (this.isOffline()) {
if (loadedFromCache) {
this.logger.debug('Offline identify using storage flags.');
this.logger.debug('Offline identify - using cached flags.');
} else {
this.logger.debug('Offline identify no storage. Defaults will be used.');
this.logger.debug(
'Offline identify - no cached flags, using defaults or already loaded flags.',
Copy link
Contributor

@tanderson-ld tanderson-ld Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion for clarity. I had to think a bit to realize what this was conveying.

Suggested change
'Offline identify - no cached flags, using defaults or already loaded flags.',
'Identify - Currently offline with no cached flags for provided context. Maintaining current flags. This may result in default flag evaluations if no flags have been loaded before.'

Is the trailing comma intentional?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our linting requires training coma for parameters that are not on one line. The linter also forces this line to be wrapped as such.

);
identifyResolve();
}
} else {
Expand Down
12 changes: 12 additions & 0 deletions packages/shared/sdk-client/src/api/LDIdentifyOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,16 @@ export interface LDIdentifyOptions {
* Defaults to 5 seconds.
*/
timeout: number;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kinyoklion timeout should be optional then

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately timeout was already implemented as non-optional. That said it defaults to 5, so setting it to 5 is equal to not providing it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will consider it.

There are advantages to having it non-optional and some SDKs require it with every call identify. Which makes it more obvious that the operation can timeout. We document it, but people are still often surprised that it times out.


/**
* When true indicates that the SDK will attempt to wait for values from
* LaunchDarkly instead of depending on cached values. The cached values will
* still be loaded, but the promise returned by the identify function will not
* resolve as a result of those cached values being loaded. Generally this
* option should NOT be used and instead flag changes should be listened to.
* It the client is set to offline mode, then this option is ignored.
*
* Defaults to false.
*/
waitForNetworkResults?: boolean;
}