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: prevent fetching toggles when they are up to date #202

Closed
wants to merge 43 commits into from
Closed
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
7d231b0
to remove: change main file
jeremiewtd Apr 30, 2024
2fdf1e0
save last update key after fetch
jeremiewtd Apr 30, 2024
4f42346
refactor to handle 304 status
jeremiewtd May 2, 2024
ce45d89
unit test last update flag
jeremiewtd May 2, 2024
bab5000
handling isUpToDate
jeremiewtd May 2, 2024
a181bd5
update test
jeremiewtd May 2, 2024
c9a1677
fix existing test not working
Florent-Wanteeed May 3, 2024
3683bed
Store lastRefreshTimestamp to prevent async
Florent-Wanteeed May 3, 2024
195c438
fix toggleStorageTTL unit
Florent-Wanteeed May 3, 2024
ebcea49
refactor: rename option
jeremiewtd May 3, 2024
2f46c6b
unit test togglesStorage initial fetch
jeremiewtd May 3, 2024
ca8ebb9
refactor: emit ready method
jeremiewtd May 3, 2024
9463475
unit test togglesStorage ready
jeremiewtd May 3, 2024
9f6a10c
unit test ready not sent with bootstrap option
jeremiewtd May 3, 2024
cee9f51
unit test fetch when expired
jeremiewtd May 3, 2024
95f126c
documentation
jeremiewtd May 3, 2024
5ace0d9
improve readme
jeremiewtd May 6, 2024
c2f34e2
restore package.json
jeremiewtd May 6, 2024
cf12da6
revert public method
jeremiewtd May 6, 2024
63d22bf
restore package
jeremiewtd May 6, 2024
8c921af
readme
jeremiewtd May 6, 2024
c225d21
fix: linting
jeremiewtd May 6, 2024
54d5ab5
Merge remote-tracking branch 'origin/main' into feature/prevent-fetch…
jeremiewtd May 7, 2024
ad853c2
fix: lint
jeremiewtd May 7, 2024
44aa7f1
Merge branch 'main' into feature/prevent-fetch-on-load
jeremiewtd May 10, 2024
2d264a6
refactor: update timestamp key
jeremiewtd May 10, 2024
1556900
test: failing test that cover the bug
jeremiewtd May 10, 2024
bff8282
fix: can be up to date even with no flags
jeremiewtd May 10, 2024
b595afc
refactor: create initialFetchToggles to be used on start only
jeremiewtd May 10, 2024
0fc23d1
doc: update readme
jeremiewtd May 10, 2024
5bb8c20
fix: lint
jeremiewtd May 16, 2024
5266140
experimental and hash
Florent-Wanteeed Jun 10, 2024
7712d92
retour PR
Florent-Wanteeed Jun 12, 2024
4e7a656
Merge branch 'main' into feature/prevent-fetch
Florent-Wanteeed Jun 12, 2024
7684319
rollback previous test optim
Florent-Wanteeed Jun 12, 2024
9ecd585
Object Hash Value
Florent-Wanteeed Jun 26, 2024
8aec66c
Merge branch 'main' into feature/prevent-fetch
Florent-Wanteeed Jun 26, 2024
3c20789
system time goes back into the past
Florent-Wanteeed Jul 2, 2024
c0078f8
refacto
Florent-Wanteeed Jul 2, 2024
9e07183
Met a jour le flag fetchedFromServer quand on est a jour
Florent-Wanteeed Jul 2, 2024
04cd36e
retour
Florent-Wanteeed Jul 9, 2024
2fae8e0
retour PR
Florent-Wanteeed Jul 10, 2024
0a5d88c
Merge pull request #2 from wanteeed/feature/prevent-fetch
Florent-Wanteeed Jul 10, 2024
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
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ The Unleash SDK takes the following options:
| customHeaders | no| `{}` | Additional headers to use when making HTTP requests to the Unleash proxy. In case of name collisions with the default headers, the `customHeaders` value will be used if it is not `null` or `undefined`. `customHeaders` values that are `null` or `undefined` will be ignored. |
| impressionDataAll | no| `false` | Allows you to trigger "impression" events for **all** `getToggle` and `getVariant` invocations. This is particularly useful for "disabled" feature toggles that are not visible to frontend SDKs. |
| environment | no | `default` | Sets the `environment` option of the [Unleash context](https://docs.getunleash.io/reference/unleash-context). This does **not** affect the SDK's [Unleash environment](https://docs.getunleash.io/reference/environments). |
| togglesStorageTTL | no | `0` | How long (Time To Live), in seconds, the toggles in storage are considered valid and should not be fetched again. If set to 0 will disable expiration checking and will be considered always expired. |
ivarconr marked this conversation as resolved.
Show resolved Hide resolved

### Listen for updates via the EventEmitter

Expand Down
170 changes: 169 additions & 1 deletion src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
IConfig,
IMutableContext,
IToggle,
InMemoryStorageProvider,
UnleashClient,
} from './index';
import { getTypeSafeRequest, getTypeSafeRequestUrl } from './test';
Expand Down Expand Up @@ -553,6 +554,77 @@ test('Should publish ready when initial fetch completed', (done) => {
});
});

describe('handling last update flag storage', () => {
let storageProvider: IStorageProvider;
let saveSpy: jest.SpyInstance;
class Store implements IStorageProvider {
public async save() {
return Promise.resolve();
}

public async get() {
return Promise.resolve([]);
}
}

beforeEach(() => {
storageProvider = new Store();
saveSpy = jest.spyOn(storageProvider, 'save');
});

test('Should store last update flag when fetch is successful', async () => {
const startTime = Date.now();
fetchMock.mockResponseOnce(JSON.stringify(data));

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider,
};

const client = new UnleashClient(config);
await client.start();
expect(saveSpy).toHaveBeenCalledWith('lastUpdate', expect.any(Number));
expect(saveSpy.mock.lastCall?.at(1)).toBeGreaterThanOrEqual(startTime);
});

test('Should store last update flag when fetch is successful with 304 status', async () => {
const startTime = Date.now();
fetchMock.mockResponseOnce(JSON.stringify(data), { status: 304 });

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider,
};

const client = new UnleashClient(config);
await client.start();
expect(saveSpy).toHaveBeenCalledWith('lastUpdate', expect.any(Number));
expect(saveSpy.mock.lastCall?.at(1)).toBeGreaterThanOrEqual(startTime);
});

test('Should not store last update flag when fetch is not successful', async () => {
fetchMock.mockResponseOnce('', { status: 500 });

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider,
};

const client = new UnleashClient(config);
await client.start();
expect(saveSpy).not.toHaveBeenCalledWith(
'lastUpdate',
expect.any(Number)
);
});
});

test('Should publish error when initial init fails', (done) => {
const givenError = 'Error';

Expand Down Expand Up @@ -1483,15 +1555,16 @@ test('Should emit impression events on getVariant calls when impressionData is f
});

test('Should publish ready only when the first fetch was successful', async () => {
expect.assertions(3);
Copy link
Member

Choose a reason for hiding this comment

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

its a bit confusing to see updates to existing tests that seems unrelated to the PR. Can you please elaborate on why you needed to modify this scenario?

Copy link
Member

Choose a reason for hiding this comment

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

(the change looks reasonable tbh, but still would love to see your comments on them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an early version of our implementation, we needed to update this test and actually we realized that the test was not working as it should so we fixed it, but we can remove that if you think that should not be part of this pull request.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for a clarification. I would prefer to have it as a separate PR if you do not mind. We can keep it here as well

fetchMock.mockResponse(JSON.stringify(data));
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
refreshInterval: 1,
disableMetrics: true,
};
const client = new UnleashClient(config);
await client.start();

let readyCount = 0;

Expand All @@ -1503,6 +1576,8 @@ test('Should publish ready only when the first fetch was successful', async () =
expect(readyCount).toEqual(1);
});

await client.start();

jest.advanceTimersByTime(1001);
jest.advanceTimersByTime(1001);

Expand Down Expand Up @@ -1715,3 +1790,96 @@ describe('READY event emission', () => {
expect(client.emit).toHaveBeenCalledWith(EVENTS.READY);
});
});

describe('handling togglesStorageTTL > 0', () => {
let storage: IStorageProvider;
let fakeNow: number;
beforeEach(async () => {
fakeNow = new Date('2024-01-01').getTime();
jest.useFakeTimers();
jest.setSystemTime(fakeNow);
storage = new InMemoryStorageProvider();

fetchMock.mockResponseOnce(JSON.stringify(data)).mockResponseOnce(
JSON.stringify({
toggles: [
{
name: 'simpleToggle',
enabled: false,
impressionData: true,
},
],
})
);

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
};
// performing an initial fetch to populate the toggles and lastUpdate timestamp
const client = new UnleashClient(config);
await client.start();
client.stop();

expect(fetchMock).toHaveBeenCalledTimes(1);
fetchMock.mockClear();
});

afterEach(() => {
jest.useRealTimers();
});

test('Should not perform an initial fetch when toggles are up to date', async () => {
jest.setSystemTime(fakeNow + 59000);
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);
await client.start();
const isEnabled = client.isEnabled('simpleToggle');
expect(isEnabled).toBe(true);
client.stop();
expect(fetchMock).toHaveBeenCalledTimes(0);
});

test('Should perform an initial fetch when toggles are expired', async () => {
jest.setSystemTime(fakeNow + 61000);

const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);
await client.start();
const isEnabled = client.isEnabled('simpleToggle');
expect(isEnabled).toBe(false);
client.stop();
expect(fetchMock).toHaveBeenCalledTimes(1);
});

test('Should send ready event when toggles are up to date', async () => {
const config: IConfig = {
url: 'http://localhost/test',
clientKey: '12',
appName: 'web',
storageProvider: storage,
togglesStorageTTL: 60,
};
const client = new UnleashClient(config);

const readySpy = jest.fn();
client.on(EVENTS.READY, readySpy);
client.on(EVENTS.INIT, () => readySpy.mockClear());
await client.start();
expect(readySpy).toHaveBeenCalledTimes(1);
});
});
66 changes: 52 additions & 14 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ interface IConfig extends IStaticContext {
customHeaders?: Record<string, string>;
impressionDataAll?: boolean;
usePOSTrequests?: boolean;
togglesStorageTTL?: number;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of adding it directly to the config definition, If we could have an optional "experimental" container we could make it clear that this option might go away or be changed in the future.

experimental?: {
  togglesStorageTTL?: number;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok we are working on it and adding the hashed context. Do you have any proposal for how you would like this to be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello

Concerning the hashed context, we started with the use of this type of library : “object-hash”.
Is this OK for you ?

unleash-proxy-client” lib size => 5.5 KB
object-hash” lib size => 10 KB

Which is a significant increase in percentage terms.

Last lib update: 02/2022 (v3.0.0),

Otherwise we can simply use a JSON.stringify (more verbose but which does not depend on anything)

Copy link
Member

Choose a reason for hiding this comment

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

We do not want to take on a library of this size for this.

I think the JSON.stringify is a good start, and as this is an experimental feature we can use that to learn the performance impact here. In the future we can consider to take advantage of the built in hasing capabilities, supported by most modern browsers (https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/digest)

}

interface IVariant {
Expand Down Expand Up @@ -92,6 +93,7 @@ const defaultVariant: IVariant = {
feature_enabled: false,
};
const storeKey = 'repo';
const lastUpdateKey = 'lastUpdate';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const lastUpdateKey = 'lastUpdate';
const lastUpdateKey = 'repoTimestamp';

Copy link
Member

Choose a reason for hiding this comment

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

I think the naming could be slightly better. Would love more opinions on the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were not very inspired for this name 😃, I think a combination of the two proposals could be the best?

Suggested change
const lastUpdateKey = 'lastUpdate';
const lastUpdateKey = 'repoLastUpdateTimestamp';


type SdkState = 'initializing' | 'healthy' | 'error';

Expand Down Expand Up @@ -145,6 +147,8 @@ export class UnleashClient extends TinyEmitter {
private usePOSTrequests = false;
private started = false;
private sdkState: SdkState;
private togglesStorageTTL: number;
private lastRefreshTimestamp: number;

constructor({
storageProvider,
Expand All @@ -165,6 +169,7 @@ export class UnleashClient extends TinyEmitter {
customHeaders = {},
impressionDataAll = false,
usePOSTrequests = false,
togglesStorageTTL = 0,
}: IConfig) {
super();
// Validations
Expand Down Expand Up @@ -193,6 +198,9 @@ export class UnleashClient extends TinyEmitter {
this.context = { appName, environment, ...context };
this.usePOSTrequests = usePOSTrequests;
this.sdkState = 'initializing';
this.togglesStorageTTL = togglesStorageTTL * 1000;
this.lastRefreshTimestamp = 0;

this.ready = new Promise((resolve) => {
this.init()
.then(resolve)
Expand Down Expand Up @@ -345,6 +353,7 @@ export class UnleashClient extends TinyEmitter {
this.context = { sessionId, ...this.context };

this.toggles = (await this.storage.get(storeKey)) || [];
this.lastRefreshTimestamp = await this.storage.get(lastUpdateKey);

if (
this.bootstrap &&
Expand Down Expand Up @@ -423,8 +432,29 @@ export class UnleashClient extends TinyEmitter {
await this.storage.save(storeKey, toggles);
}

private isUpToDate() {
if (this.togglesStorageTTL <= 0 || this.toggles.length === 0) {
jeremiewtd marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
const timestamp = Date.now();

return !!(
this.lastRefreshTimestamp &&
timestamp - this.lastRefreshTimestamp <= this.togglesStorageTTL
);
}

private async updateLastRefresh() {
this.lastRefreshTimestamp = Date.now();
this.storage.save(lastUpdateKey, this.lastRefreshTimestamp);
}

private async fetchToggles() {
if (this.fetch) {
if (this.isUpToDate()) {
this.emitReady();
return;
Copy link
Member

@ivarconr ivarconr May 7, 2024

Choose a reason for hiding this comment

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

This method have been carefully designed to not use "early returns". Early return in combination with try/catch control flow increases the complexity. I am not comfortable about breaking the current pattern to support avoiding the initial fetch use case.

I think we instead can have a specific initialFetchMethod, that only calls fetchToggles if the cached data is not considered recent.

Copy link
Member

Choose a reason for hiding this comment

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

By running this on each fetch we have another potential bug in the future: What if the stored timestamp somehow ends up far in to the future (we are dealing with untrusted browsers) we will never try to fetch updates at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're right, the initialFetchMethod is a better idea than this, we will rework it.

Come to think of it, this code is actually buggy... because if we update the context and the isUpToDate function says "yes", the flags will not be updated, which is not intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by "untrusted browsers", what could be the behavior on them?

Copy link
Member

Choose a reason for hiding this comment

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

Anything in user-land cannot be trusted. Users might suddenly change their local time etc, which forces us to be a bit restrictive on what we trust. E.g. if the stored data is 2 hours in the future we should probably disregard the cached data and do a fetch.

}
if (this.abortController) {
this.abortController.abort();
}
Expand Down Expand Up @@ -457,20 +487,7 @@ export class UnleashClient extends TinyEmitter {
this.emit(EVENTS.RECOVERED);
}

if (response.ok && response.status !== 304) {
this.etag = response.headers.get('ETag') || '';
const data = await response.json();
await this.storeToggles(data.toggles);

if (this.sdkState !== 'healthy') {
this.sdkState = 'healthy';
}

if (!this.readyEventEmitted) {
this.emit(EVENTS.READY);
this.readyEventEmitted = true;
}
} else if (!response.ok && response.status !== 304) {
if (!response.ok && response.status !== 304) {
console.error(
'Unleash: Fetching feature toggles did not have an ok response'
);
Expand All @@ -479,7 +496,21 @@ export class UnleashClient extends TinyEmitter {
type: 'HttpError',
code: response.status,
});
return;
}
if (response.status !== 304) {
this.etag = response.headers.get('ETag') || '';
const data = await response.json();
await this.storeToggles(data.toggles);

if (this.sdkState !== 'healthy') {
this.sdkState = 'healthy';
}

this.emitReady();
}

this.updateLastRefresh();
Copy link
Member

Choose a reason for hiding this comment

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

This should happen when we get a positive response from the server (2xx or 304).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes this is the case no?

} catch (e) {
console.error('Unleash: unable to fetch feature toggles', e);
this.sdkState = 'error';
Expand All @@ -489,6 +520,13 @@ export class UnleashClient extends TinyEmitter {
}
}
}

private emitReady() {
jeremiewtd marked this conversation as resolved.
Show resolved Hide resolved
if (!this.readyEventEmitted) {
this.emit(EVENTS.READY);
this.readyEventEmitted = true;
}
}
}

// export storage providers from root module
Expand Down
Loading