Skip to content

Commit

Permalink
Update context waits for start (#130)
Browse files Browse the repository at this point in the history
  • Loading branch information
kwasniew committed Dec 16, 2022
1 parent 6c7a458 commit 0ca4603
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 4 deletions.
25 changes: 23 additions & 2 deletions src/index.test.ts
Expand Up @@ -48,7 +48,7 @@ test('Should perform an initial fetch as POST', async () => {
};
const client = new UnleashClient(config);
await client.start();

const request = getTypeSafeRequest(fetchMock, 0);
const body = JSON.parse(request.body as string);

Expand All @@ -65,7 +65,7 @@ test('Should perform an initial fetch as GET', async () => {
};
const client = new UnleashClient(config);
await client.start();

const request = getTypeSafeRequest(fetchMock, 0);

expect(request.method).toBe('GET');
Expand Down Expand Up @@ -852,6 +852,27 @@ test('Should update context fields on request', async () => {
expect(url.searchParams.get('environment')).toEqual('prod');
});

test('Updating context should wait on asynchronous start', async () => {
fetchMock.mockResponses(
[JSON.stringify(data), { status: 200 }],
[JSON.stringify(data), { status: 200 }]
);
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
environment: 'prod',
};
const client = new UnleashClient(config);

client.start();
await client.updateContext({
userId: '123'
});

expect(fetchMock).toBeCalledTimes(2);
});

test('Should not replace sessionId when updating context', async () => {
fetchMock.mockResponses(
[JSON.stringify(data), { status: 200 }],
Expand Down
16 changes: 14 additions & 2 deletions src/index.ts
Expand Up @@ -210,7 +210,7 @@ export class UnleashClient extends TinyEmitter {
const toggle = this.toggles.find((t) => t.name === toggleName);
const enabled = toggle?.enabled || false;
const variant = toggle ? toggle.variant : defaultVariant;

this.metrics.count(toggleName, true);
if (toggle?.impressionData || this.impressionDataAll) {
const event = this.eventsHandler.createImpressionEvent(
Expand Down Expand Up @@ -240,9 +240,21 @@ export class UnleashClient extends TinyEmitter {
sessionId: this.context.sessionId,
};
this.context = { ...staticContext, ...context };

if (this.timerRef) {
await this.fetchToggles();
}
else {

This comment has been minimized.

Copy link
@YayBurritos

YayBurritos Jan 20, 2023

This change seems to require us to now call start() prior to calling updateContext(). Our application (using package version 2.1.0) currently: 1) updates the content first (by calling updateContext()); and then 2) calls start() so that the newly-update context is used when the toggles are first retrieved. This approach works just fine prior this particular update in 2.4.1.

Since both updateContext() and start() are async, we are awaiting each. The introduction of the await new Promise(...) in this function is causing our application to basically hang since this Promise can't be resolved (there's nothing to cause the EVENTS.READY event to be emitted in that scenario).

If the plan is to stick with this fix going forward, can the documentation be updated to indicate that this is a potentially-breaking change and provide guidelines on the proper order in calling these functions - particularly when you're initializing the UnleashClient???

Thanks!

await new Promise<void>(resolve => {
const listener = () => {
this.fetchToggles().then(() => {
this.off(EVENTS.READY, listener);
resolve();
});
};
this.once(EVENTS.READY, listener);
});
}
}

public getContext() {
Expand Down Expand Up @@ -344,7 +356,7 @@ export class UnleashClient extends TinyEmitter {
const url = isPOST ? this.url : urlWithContextAsQuery(this.url, this.context);
const method = isPOST ? 'POST' : 'GET';
const body = isPOST ? JSON.stringify({context: this.context}) : undefined;

const response = await this.fetch(url.toString(), {
method,
cache: 'no-cache',
Expand Down

0 comments on commit 0ca4603

Please sign in to comment.