Skip to content

Commit

Permalink
Replace splits::checkCache method with storage::validateCache method
Browse files Browse the repository at this point in the history
  • Loading branch information
EmilianoSanchez committed Dec 18, 2024
1 parent 0cb3a1f commit d392cc8
Show file tree
Hide file tree
Showing 10 changed files with 19 additions and 49 deletions.
8 changes: 0 additions & 8 deletions src/storages/AbstractSplitsCacheAsync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,6 @@ export abstract class AbstractSplitsCacheAsync implements ISplitsCacheAsync {
return Promise.resolve(true);
}

/**
* Check if the splits information is already stored in cache.
* Noop, just keeping the interface. This is used by client-side implementations only.
*/
checkCache(): Promise<boolean> {
return Promise.resolve(false);
}

/**
* Kill `name` split and set `defaultTreatment` and `changeNumber`.
* Used for SPLIT_KILL push notifications.
Expand Down
8 changes: 0 additions & 8 deletions src/storages/AbstractSplitsCacheSync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,6 @@ export abstract class AbstractSplitsCacheSync implements ISplitsCacheSync {

abstract clear(): void

/**
* Check if the splits information is already stored in cache. This data can be preloaded.
* It is used as condition to emit SDK_SPLITS_CACHE_LOADED, and then SDK_READY_FROM_CACHE.
*/
checkCache(): boolean {
return false;
}

/**
* Kill `name` split and set `defaultTreatment` and `changeNumber`.
* Used for SPLIT_KILL push notifications.
Expand Down
11 changes: 1 addition & 10 deletions src/storages/inLocalStorage/SplitsCacheInLocal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,15 +212,6 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
}
}

/**
* Check if the splits information is already stored in browser LocalStorage.
* In this function we could add more code to check if the data is valid.
* @override
*/
checkCache(): boolean {
return this.getChangeNumber() > -1;
}

/**
* Clean Splits cache if its `lastUpdated` timestamp is older than the given `expirationTimestamp`,
*
Expand All @@ -245,7 +236,7 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
this.updateNewFilter = true;

// if there is cache, clear it
if (this.checkCache()) this.clear();
if (this.getChangeNumber() > -1) this.clear();

} catch (e) {
this.log.error(LOG_PREFIX + e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,10 @@ test('SPLIT CACHE / LocalStorage', () => {
expect(cache.getSplit('lol1')).toEqual(null);
expect(cache.getSplit('lol2')).toEqual(somethingElse);

expect(cache.checkCache()).toBe(false); // checkCache should return false until localstorage has data.

expect(cache.getChangeNumber() === -1).toBe(true);

expect(cache.checkCache()).toBe(false); // checkCache should return false until localstorage has data.

cache.setChangeNumber(123);

expect(cache.checkCache()).toBe(true); // checkCache should return true once localstorage has data.

expect(cache.getChangeNumber() === 123).toBe(true);

});
Expand Down
5 changes: 5 additions & 0 deletions src/storages/inLocalStorage/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ export function InLocalStorage(options: InLocalStorageOptions = {}): IStorageSyn
telemetry: shouldRecordTelemetry(params) ? new TelemetryCacheInMemory(splits, segments) : undefined,
uniqueKeys: impressionsMode === NONE ? new UniqueKeysCacheInMemoryCS() : undefined,

// @TODO implement
validateCache() {
return splits.getChangeNumber() > -1;
},

destroy() { },

// When using shared instantiation with MEMORY we reuse everything but segments (they are customer per key).
Expand Down
5 changes: 1 addition & 4 deletions src/storages/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,6 @@ export interface ISplitsCacheBase {
// only for Client-Side. Returns true if the storage is not synchronized yet (getChangeNumber() === -1) or contains a FF using segments or large segments
usesSegments(): MaybeThenable<boolean>,
clear(): MaybeThenable<boolean | void>,
// should never reject or throw an exception. Instead return false by default, to avoid emitting SDK_READY_FROM_CACHE.
checkCache(): MaybeThenable<boolean>,
killLocally(name: string, defaultTreatment: string, changeNumber: number): MaybeThenable<boolean>,
getNamesByFlagSets(flagSets: string[]): MaybeThenable<Set<string>[]>
}
Expand All @@ -209,7 +207,6 @@ export interface ISplitsCacheSync extends ISplitsCacheBase {
trafficTypeExists(trafficType: string): boolean,
usesSegments(): boolean,
clear(): void,
checkCache(): boolean,
killLocally(name: string, defaultTreatment: string, changeNumber: number): boolean,
getNamesByFlagSets(flagSets: string[]): Set<string>[]
}
Expand All @@ -226,7 +223,6 @@ export interface ISplitsCacheAsync extends ISplitsCacheBase {
trafficTypeExists(trafficType: string): Promise<boolean>,
usesSegments(): Promise<boolean>,
clear(): Promise<boolean | void>,
checkCache(): Promise<boolean>,
killLocally(name: string, defaultTreatment: string, changeNumber: number): Promise<boolean>,
getNamesByFlagSets(flagSets: string[]): Promise<Set<string>[]>
}
Expand Down Expand Up @@ -457,6 +453,7 @@ export interface IStorageSync extends IStorageBase<
IUniqueKeysCacheSync
> {
// Defined in client-side
validateCache?: () => boolean, // @TODO support async
largeSegments?: ISegmentsCacheSync,
}

Expand Down
8 changes: 4 additions & 4 deletions src/sync/__tests__/syncManagerOnline.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ test('syncManagerOnline should start or not the submitter depending on user cons

const syncManager = syncManagerOnlineFactory()({
settings, // @ts-ignore
storage: { splits: { checkCache: () => false } },
storage: {},
});
const submitterManager = syncManager.submitterManager!;

Expand Down Expand Up @@ -100,7 +100,7 @@ test('syncManagerOnline should syncAll a single time when sync is disabled', ()
// Test pushManager for main client
const syncManager = syncManagerOnlineFactory(() => pollingManagerMock, pushManagerFactoryMock)({
settings, // @ts-ignore
storage: { splits: { checkCache: () => false } },
storage: { validateCache: () => false },
});

expect(pushManagerFactoryMock).not.toBeCalled();
Expand Down Expand Up @@ -169,7 +169,7 @@ test('syncManagerOnline should syncAll a single time when sync is disabled', ()
// pushManager instantiation control test
const testSyncManager = syncManagerOnlineFactory(() => pollingManagerMock, pushManagerFactoryMock)({
settings, // @ts-ignore
storage: { splits: { checkCache: () => false } },
storage: { validateCache: () => false },
});

expect(pushManagerFactoryMock).toBeCalled();
Expand All @@ -186,7 +186,7 @@ test('syncManagerOnline should syncAll a single time when sync is disabled', ()
test('syncManagerOnline should emit SDK_SPLITS_CACHE_LOADED if validateCache returns true', async () => {
const params = {
settings: fullSettings,
storage: { splits: { checkCache: () => true } },
storage: { validateCache: () => true },
readiness: { splits: { emit: jest.fn() } }
}; // @ts-ignore
const syncManager = syncManagerOnlineFactory()(params);
Expand Down
9 changes: 5 additions & 4 deletions src/sync/offline/syncTasks/fromObjectSyncTask.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { forOwn } from '../../../utils/lang';
import { IReadinessManager } from '../../../readiness/types';
import { ISplitsCacheSync } from '../../../storages/types';
import { ISplitsCacheSync, IStorageSync } from '../../../storages/types';
import { ISplitsParser } from '../splitsParser/types';
import { ISplit, ISplitPartial } from '../../../dtos/types';
import { syncTaskFactory } from '../../syncTask';
Expand All @@ -15,7 +15,7 @@ import { SYNC_OFFLINE_DATA, ERROR_SYNC_OFFLINE_LOADING } from '../../../logger/c
*/
export function fromObjectUpdaterFactory(
splitsParser: ISplitsParser,
storage: { splits: ISplitsCacheSync },
storage: Pick<IStorageSync, 'splits' | 'validateCache'>,
readiness: IReadinessManager,
settings: ISettings,
): () => Promise<boolean> {
Expand Down Expand Up @@ -60,9 +60,10 @@ export function fromObjectUpdaterFactory(

if (startingUp) {
startingUp = false;
Promise.resolve(splitsCache.checkCache()).then(cacheReady => {
const isCacheLoaded = storage.validateCache ? storage.validateCache() : false;
Promise.resolve().then(() => {
// Emits SDK_READY_FROM_CACHE
if (cacheReady) readiness.splits.emit(SDK_SPLITS_CACHE_LOADED);
if (isCacheLoaded) readiness.splits.emit(SDK_SPLITS_CACHE_LOADED);
// Emits SDK_READY
readiness.segments.emit(SDK_SEGMENTS_ARRIVED);
});
Expand Down
6 changes: 2 additions & 4 deletions src/sync/syncManagerOnline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,10 +89,8 @@ export function syncManagerOnlineFactory(
running = true;

if (startFirstTime) {
const isCacheLoaded = storage.splits.checkCache();
Promise.resolve().then(() => {
if (isCacheLoaded) readiness.splits.emit(SDK_SPLITS_CACHE_LOADED);
});
const isCacheLoaded = storage.validateCache ? storage.validateCache() : false;
if (isCacheLoaded) Promise.resolve().then(() => { readiness.splits.emit(SDK_SPLITS_CACHE_LOADED); });
}

// start syncing splits and segments
Expand Down
2 changes: 1 addition & 1 deletion src/utils/settingsValidation/storage/storageCS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { IStorageFactoryParams, IStorageSync } from '../../../storages/types';

export function __InLocalStorageMockFactory(params: IStorageFactoryParams): IStorageSync {
const result = InMemoryStorageCSFactory(params);
result.splits.checkCache = () => true; // to emit SDK_READY_FROM_CACHE
result.validateCache = () => true; // to emit SDK_READY_FROM_CACHE
return result;
}
__InLocalStorageMockFactory.type = STORAGE_MEMORY;
Expand Down

0 comments on commit d392cc8

Please sign in to comment.