Skip to content

Commit

Permalink
fix: Minor fixes from docs pr review. (#363)
Browse files Browse the repository at this point in the history
* Corrected close to return a Promise. 
* Removed redundant EventName `identifying`.
* Add missing type for getConnectionMode.
  • Loading branch information
yusinto authored Feb 7, 2024
1 parent 7e111b4 commit 4768bf7
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 35 deletions.
47 changes: 19 additions & 28 deletions packages/shared/sdk-client/src/LDClientImpl.storage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,10 @@ describe('sdk-client storage', () => {
expect(basicPlatform.storage.get).toHaveBeenCalledWith('org:Testy Pizza');

// 'change' should not have been emitted
expect(emitter.emit).toHaveBeenCalledTimes(3);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'identifying', context);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenNthCalledWith(
3,
2,
'error',
context,
expect.objectContaining({ message: 'test-error' }),
Expand Down Expand Up @@ -135,20 +134,15 @@ describe('sdk-client storage', () => {
);

// 'change' should not have been emitted
expect(emitter.emit).toHaveBeenCalledTimes(3);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(
1,
'identifying',
expect.objectContaining(toMulti(context)),
);
expect(emitter.emit).toHaveBeenNthCalledWith(
2,
'change',
expect.objectContaining(toMulti(context)),
defaultFlagKeys,
);
expect(emitter.emit).toHaveBeenNthCalledWith(
3,
2,
'error',
expect.objectContaining(toMulti(context)),
expect.objectContaining({ message: 'test-error' }),
Expand All @@ -175,8 +169,6 @@ describe('sdk-client storage', () => {
ldc.identify(context).then(noop);
await jest.runAllTimersAsync();

expect(emitter.emit).toHaveBeenCalledTimes(1);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'identifying', context);
expect(basicPlatform.storage.set).toHaveBeenNthCalledWith(
1,
'org:Testy Pizza',
Expand Down Expand Up @@ -207,9 +199,8 @@ describe('sdk-client storage', () => {
'org:Testy Pizza',
JSON.stringify(putResponse),
);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'identifying', context);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
});

test('syncing storage when a flag is added', async () => {
Expand All @@ -229,7 +220,7 @@ describe('sdk-client storage', () => {
'org:Testy Pizza',
JSON.stringify(putResponse),
);
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['another-dev-test-flag']);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['another-dev-test-flag']);
});

test('syncing storage when a flag is updated', async () => {
Expand All @@ -239,7 +230,7 @@ describe('sdk-client storage', () => {
const allFlags = await identifyGetAllFlags(false, putResponse);

expect(allFlags).toMatchObject({ 'dev-test-flag': false });
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
});

test('syncing storage on multiple flag operations', async () => {
Expand All @@ -254,7 +245,7 @@ describe('sdk-client storage', () => {

expect(allFlags).toMatchObject({ 'dev-test-flag': false, 'another-dev-test-flag': true });
expect(allFlags).not.toHaveProperty('moonshot-demo');
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, [
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, [
'moonshot-demo',
'dev-test-flag',
'another-dev-test-flag',
Expand All @@ -275,8 +266,8 @@ describe('sdk-client storage', () => {
'org:Testy Pizza',
JSON.stringify(defaultPutResponse),
);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, defaultFlagKeys);
expect(emitter.emit).toHaveBeenCalledTimes(1);
expect(emitter.emit).toHaveBeenNthCalledWith(1, 'change', context, defaultFlagKeys);

// this is defaultPutResponse
expect(allFlags).toEqual({
Expand Down Expand Up @@ -306,7 +297,7 @@ describe('sdk-client storage', () => {

// both previous and current are true but inExperiment has changed
// so a change event should be emitted
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
});

test('patch should emit change event', async () => {
Expand All @@ -324,8 +315,8 @@ describe('sdk-client storage', () => {
expect.stringContaining(JSON.stringify(patchResponse)),
);
expect(flagsInStorage['dev-test-flag'].version).toEqual(patchResponse.version);
expect(emitter.emit).toHaveBeenCalledTimes(3);
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
});

test('patch should add new flags', async () => {
Expand All @@ -341,8 +332,8 @@ describe('sdk-client storage', () => {
expect.stringContaining(JSON.stringify(patchResponse)),
);
expect(flagsInStorage).toHaveProperty('another-dev-test-flag');
expect(emitter.emit).toHaveBeenCalledTimes(3);
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['another-dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['another-dev-test-flag']);
});

test('patch should ignore older version', async () => {
Expand Down Expand Up @@ -395,8 +386,8 @@ describe('sdk-client storage', () => {
expect.stringContaining('dev-test-flag'),
);
expect(flagsInStorage['dev-test-flag']).toMatchObject({ ...deleteResponse, deleted: true });
expect(emitter.emit).toHaveBeenCalledTimes(3);
expect(emitter.emit).toHaveBeenNthCalledWith(3, 'change', context, ['dev-test-flag']);
expect(emitter.emit).toHaveBeenCalledTimes(2);
expect(emitter.emit).toHaveBeenNthCalledWith(2, 'change', context, ['dev-test-flag']);
});

test('delete should not delete equal version', async () => {
Expand Down
1 change: 0 additions & 1 deletion packages/shared/sdk-client/src/LDClientImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,6 @@ export default class LDClientImpl implements LDClient {

const { identifyPromise, identifyResolve } = this.createPromiseWithListeners();
this.logger.debug(`Identifying ${JSON.stringify(context)}`);
this.emitter.emit('identifying', context);

const flagsStorage = await this.getFlagsFromStorage(checkedContext.canonicalKey);
if (flagsStorage) {
Expand Down
8 changes: 6 additions & 2 deletions packages/shared/sdk-client/src/api/LDClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export interface LDClient {
* events. After the client is closed, all calls to {@link variation} will return default values,
* and it will not make any requests to LaunchDarkly.
*/
close(): void;
close(): Promise<void>;

/**
* Flushes all pending analytics events.
Expand All @@ -80,6 +80,11 @@ export interface LDClient {
*/
flush(): Promise<{ error?: Error; result: boolean }>;

/**
* Gets the SDK connection mode.
*/
getConnectionMode(): ConnectionMode;

/**
* Returns the client's current context.
*
Expand Down Expand Up @@ -196,7 +201,6 @@ export interface LDClient {
*
* The following event names (keys) are used by the client:
*
* - `"identifying"`: The client starts to fetch feature flags.
* - `"error"`: General event for any kind of error condition during client operation.
* The callback parameter is an Error object. If you do not listen for "error"
* events, then the errors will be logged with `console.log()`.
Expand Down
4 changes: 1 addition & 3 deletions packages/shared/sdk-client/src/api/LDEmitter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,11 @@ describe('LDEmitter', () => {
test('eventNames', () => {
const errorHandler1 = jest.fn();
const changeHandler = jest.fn();
const readyHandler = jest.fn();

emitter.on('identifying', readyHandler);
emitter.on('error', errorHandler1);
emitter.on('change', changeHandler);

expect(emitter.eventNames()).toEqual(['identifying', 'error', 'change']);
expect(emitter.eventNames()).toEqual(['error', 'change']);
});

test('listener count', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/sdk-client/src/api/LDEmitter.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export type EventName = 'identifying' | 'error' | 'change';
export type EventName = 'error' | 'change';

type CustomEventListeners = {
original: Function;
Expand Down

0 comments on commit 4768bf7

Please sign in to comment.