From 0cb3a1f51c938c4cb51fb397b4f10364996eb76a Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 18 Dec 2024 00:20:10 -0300 Subject: [PATCH 1/3] Move validateCache call from splitChangesUpdater to syncManagerOnline --- src/sync/__tests__/syncManagerOnline.spec.ts | 32 ++++++++++++++++--- .../polling/updaters/splitChangesUpdater.ts | 13 ++------ src/sync/syncManagerOnline.ts | 14 ++++++-- 3 files changed, 41 insertions(+), 18 deletions(-) diff --git a/src/sync/__tests__/syncManagerOnline.spec.ts b/src/sync/__tests__/syncManagerOnline.spec.ts index 7fda853b..11346c98 100644 --- a/src/sync/__tests__/syncManagerOnline.spec.ts +++ b/src/sync/__tests__/syncManagerOnline.spec.ts @@ -2,6 +2,7 @@ import { fullSettings } from '../../utils/settingsValidation/__tests__/settings. import { syncTaskFactory } from './syncTask.mock'; import { syncManagerOnlineFactory } from '../syncManagerOnline'; import { IReadinessManager } from '../../readiness/types'; +import { SDK_SPLITS_CACHE_LOADED } from '../../readiness/constants'; jest.mock('../submitters/submitterManager', () => { return { @@ -45,8 +46,10 @@ const pushManagerFactoryMock = jest.fn(() => pushManagerMock); test('syncManagerOnline should start or not the submitter depending on user consent status', () => { const settings = { ...fullSettings }; - // @ts-ignore - const syncManager = syncManagerOnlineFactory()({ settings }); + const syncManager = syncManagerOnlineFactory()({ + settings, // @ts-ignore + storage: { splits: { checkCache: () => false } }, + }); const submitterManager = syncManager.submitterManager!; syncManager.start(); @@ -95,7 +98,10 @@ test('syncManagerOnline should syncAll a single time when sync is disabled', () // @ts-ignore // Test pushManager for main client - const syncManager = syncManagerOnlineFactory(() => pollingManagerMock, pushManagerFactoryMock)({ settings }); + const syncManager = syncManagerOnlineFactory(() => pollingManagerMock, pushManagerFactoryMock)({ + settings, // @ts-ignore + storage: { splits: { checkCache: () => false } }, + }); expect(pushManagerFactoryMock).not.toBeCalled(); @@ -161,7 +167,10 @@ test('syncManagerOnline should syncAll a single time when sync is disabled', () settings.sync.enabled = true; // @ts-ignore // pushManager instantiation control test - const testSyncManager = syncManagerOnlineFactory(() => pollingManagerMock, pushManagerFactoryMock)({ settings }); + const testSyncManager = syncManagerOnlineFactory(() => pollingManagerMock, pushManagerFactoryMock)({ + settings, // @ts-ignore + storage: { splits: { checkCache: () => false } }, + }); expect(pushManagerFactoryMock).toBeCalled(); @@ -173,3 +182,18 @@ test('syncManagerOnline should syncAll a single time when sync is disabled', () testSyncManager.stop(); }); + +test('syncManagerOnline should emit SDK_SPLITS_CACHE_LOADED if validateCache returns true', async () => { + const params = { + settings: fullSettings, + storage: { splits: { checkCache: () => true } }, + readiness: { splits: { emit: jest.fn() } } + }; // @ts-ignore + const syncManager = syncManagerOnlineFactory()(params); + + await syncManager.start(); + + expect(params.readiness.splits.emit).toBeCalledWith(SDK_SPLITS_CACHE_LOADED); + + syncManager.stop(); +}); diff --git a/src/sync/polling/updaters/splitChangesUpdater.ts b/src/sync/polling/updaters/splitChangesUpdater.ts index e8153987..e447227d 100644 --- a/src/sync/polling/updaters/splitChangesUpdater.ts +++ b/src/sync/polling/updaters/splitChangesUpdater.ts @@ -3,7 +3,7 @@ import { ISplitChangesFetcher } from '../fetchers/types'; import { ISplit, ISplitChangesResponse, ISplitFiltersValidation } from '../../../dtos/types'; import { ISplitsEventEmitter } from '../../../readiness/types'; import { timeout } from '../../../utils/promise/timeout'; -import { SDK_SPLITS_ARRIVED, SDK_SPLITS_CACHE_LOADED } from '../../../readiness/constants'; +import { SDK_SPLITS_ARRIVED } from '../../../readiness/constants'; import { ILogger } from '../../../logger/types'; import { SYNC_SPLITS_FETCH, SYNC_SPLITS_NEW, SYNC_SPLITS_REMOVED, SYNC_SPLITS_SEGMENTS, SYNC_SPLITS_FETCH_FAILS, SYNC_SPLITS_FETCH_RETRY } from '../../../logger/constants'; import { startsWith } from '../../../utils/lang'; @@ -153,7 +153,7 @@ export function splitChangesUpdaterFactory( */ function _splitChangesUpdater(since: number, retry = 0): Promise { log.debug(SYNC_SPLITS_FETCH, [since]); - const fetcherPromise = Promise.resolve(splitUpdateNotification ? + return Promise.resolve(splitUpdateNotification ? { splits: [splitUpdateNotification.payload], till: splitUpdateNotification.changeNumber } : splitChangesFetcher(since, noCache, till, _promiseDecorator) ) @@ -200,15 +200,6 @@ export function splitChangesUpdaterFactory( } return false; }); - - // After triggering the requests, if we have cached splits information let's notify that to emit SDK_READY_FROM_CACHE. - // Wrapping in a promise since checkCache can be async. - if (splitsEventEmitter && startingUp) { - Promise.resolve(splits.checkCache()).then(isCacheReady => { - if (isCacheReady) splitsEventEmitter.emit(SDK_SPLITS_CACHE_LOADED); - }); - } - return fetcherPromise; } let sincePromise = Promise.resolve(splits.getChangeNumber()); // `getChangeNumber` never rejects or throws error diff --git a/src/sync/syncManagerOnline.ts b/src/sync/syncManagerOnline.ts index 5410c17f..777fe41a 100644 --- a/src/sync/syncManagerOnline.ts +++ b/src/sync/syncManagerOnline.ts @@ -9,6 +9,7 @@ import { SYNC_START_POLLING, SYNC_CONTINUE_POLLING, SYNC_STOP_POLLING } from '.. import { isConsentGranted } from '../consent'; import { POLLING, STREAMING, SYNC_MODE_UPDATE } from '../utils/constants'; import { ISdkFactoryContextSync } from '../sdkFactory/types'; +import { SDK_SPLITS_CACHE_LOADED } from '../readiness/constants'; /** * Online SyncManager factory. @@ -28,7 +29,7 @@ export function syncManagerOnlineFactory( */ return function (params: ISdkFactoryContextSync): ISyncManagerCS { - const { settings, settings: { log, streamingEnabled, sync: { enabled: syncEnabled } }, telemetryTracker } = params; + const { settings, settings: { log, streamingEnabled, sync: { enabled: syncEnabled } }, telemetryTracker, storage, readiness } = params; /** Polling Manager */ const pollingManager = pollingManagerFactory && pollingManagerFactory(params); @@ -87,6 +88,13 @@ export function syncManagerOnlineFactory( start() { running = true; + if (startFirstTime) { + const isCacheLoaded = storage.splits.checkCache(); + Promise.resolve().then(() => { + if (isCacheLoaded) readiness.splits.emit(SDK_SPLITS_CACHE_LOADED); + }); + } + // start syncing splits and segments if (pollingManager) { @@ -96,7 +104,6 @@ export function syncManagerOnlineFactory( // Doesn't call `syncAll` when the syncManager is resuming if (startFirstTime) { pollingManager.syncAll(); - startFirstTime = false; } pushManager.start(); } else { @@ -105,13 +112,14 @@ export function syncManagerOnlineFactory( } else { if (startFirstTime) { pollingManager.syncAll(); - startFirstTime = false; } } } // start periodic data recording (events, impressions, telemetry). submitterManager.start(!isConsentGranted(settings)); + + startFirstTime = false; }, /** From d392cc88cc4ee1c22aefa20eb77ec85629d744f7 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 18 Dec 2024 00:43:02 -0300 Subject: [PATCH 2/3] Replace splits::checkCache method with storage::validateCache method --- src/storages/AbstractSplitsCacheAsync.ts | 8 -------- src/storages/AbstractSplitsCacheSync.ts | 8 -------- src/storages/inLocalStorage/SplitsCacheInLocal.ts | 11 +---------- .../__tests__/SplitsCacheInLocal.spec.ts | 6 ------ src/storages/inLocalStorage/index.ts | 5 +++++ src/storages/types.ts | 5 +---- src/sync/__tests__/syncManagerOnline.spec.ts | 8 ++++---- src/sync/offline/syncTasks/fromObjectSyncTask.ts | 9 +++++---- src/sync/syncManagerOnline.ts | 6 ++---- src/utils/settingsValidation/storage/storageCS.ts | 2 +- 10 files changed, 19 insertions(+), 49 deletions(-) diff --git a/src/storages/AbstractSplitsCacheAsync.ts b/src/storages/AbstractSplitsCacheAsync.ts index dcf059ed..56664b22 100644 --- a/src/storages/AbstractSplitsCacheAsync.ts +++ b/src/storages/AbstractSplitsCacheAsync.ts @@ -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 { - return Promise.resolve(false); - } - /** * Kill `name` split and set `defaultTreatment` and `changeNumber`. * Used for SPLIT_KILL push notifications. diff --git a/src/storages/AbstractSplitsCacheSync.ts b/src/storages/AbstractSplitsCacheSync.ts index f82ebbd6..09c131ba 100644 --- a/src/storages/AbstractSplitsCacheSync.ts +++ b/src/storages/AbstractSplitsCacheSync.ts @@ -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. diff --git a/src/storages/inLocalStorage/SplitsCacheInLocal.ts b/src/storages/inLocalStorage/SplitsCacheInLocal.ts index 93eb6f32..92057d4a 100644 --- a/src/storages/inLocalStorage/SplitsCacheInLocal.ts +++ b/src/storages/inLocalStorage/SplitsCacheInLocal.ts @@ -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`, * @@ -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); diff --git a/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts b/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts index 4d8ec076..17b2584d 100644 --- a/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts +++ b/src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts @@ -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); }); diff --git a/src/storages/inLocalStorage/index.ts b/src/storages/inLocalStorage/index.ts index 871be592..1d16cd62 100644 --- a/src/storages/inLocalStorage/index.ts +++ b/src/storages/inLocalStorage/index.ts @@ -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). diff --git a/src/storages/types.ts b/src/storages/types.ts index 5d7b40b6..8c63f452 100644 --- a/src/storages/types.ts +++ b/src/storages/types.ts @@ -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, clear(): MaybeThenable, - // should never reject or throw an exception. Instead return false by default, to avoid emitting SDK_READY_FROM_CACHE. - checkCache(): MaybeThenable, killLocally(name: string, defaultTreatment: string, changeNumber: number): MaybeThenable, getNamesByFlagSets(flagSets: string[]): MaybeThenable[]> } @@ -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[] } @@ -226,7 +223,6 @@ export interface ISplitsCacheAsync extends ISplitsCacheBase { trafficTypeExists(trafficType: string): Promise, usesSegments(): Promise, clear(): Promise, - checkCache(): Promise, killLocally(name: string, defaultTreatment: string, changeNumber: number): Promise, getNamesByFlagSets(flagSets: string[]): Promise[]> } @@ -457,6 +453,7 @@ export interface IStorageSync extends IStorageBase< IUniqueKeysCacheSync > { // Defined in client-side + validateCache?: () => boolean, // @TODO support async largeSegments?: ISegmentsCacheSync, } diff --git a/src/sync/__tests__/syncManagerOnline.spec.ts b/src/sync/__tests__/syncManagerOnline.spec.ts index 11346c98..c7dba96e 100644 --- a/src/sync/__tests__/syncManagerOnline.spec.ts +++ b/src/sync/__tests__/syncManagerOnline.spec.ts @@ -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!; @@ -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(); @@ -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(); @@ -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); diff --git a/src/sync/offline/syncTasks/fromObjectSyncTask.ts b/src/sync/offline/syncTasks/fromObjectSyncTask.ts index 84805110..1b3f97f8 100644 --- a/src/sync/offline/syncTasks/fromObjectSyncTask.ts +++ b/src/sync/offline/syncTasks/fromObjectSyncTask.ts @@ -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'; @@ -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, readiness: IReadinessManager, settings: ISettings, ): () => Promise { @@ -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); }); diff --git a/src/sync/syncManagerOnline.ts b/src/sync/syncManagerOnline.ts index 777fe41a..aed32493 100644 --- a/src/sync/syncManagerOnline.ts +++ b/src/sync/syncManagerOnline.ts @@ -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 diff --git a/src/utils/settingsValidation/storage/storageCS.ts b/src/utils/settingsValidation/storage/storageCS.ts index f7b531fc..7d58af3d 100644 --- a/src/utils/settingsValidation/storage/storageCS.ts +++ b/src/utils/settingsValidation/storage/storageCS.ts @@ -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; From 679f841c0389da146b8ccdeaf094f3b44c35d875 Mon Sep 17 00:00:00 2001 From: Emiliano Sanchez Date: Wed, 18 Dec 2024 15:17:45 -0300 Subject: [PATCH 3/3] Polishing --- src/sync/offline/syncTasks/fromObjectSyncTask.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sync/offline/syncTasks/fromObjectSyncTask.ts b/src/sync/offline/syncTasks/fromObjectSyncTask.ts index 1b3f97f8..d7184fcc 100644 --- a/src/sync/offline/syncTasks/fromObjectSyncTask.ts +++ b/src/sync/offline/syncTasks/fromObjectSyncTask.ts @@ -1,6 +1,6 @@ import { forOwn } from '../../../utils/lang'; import { IReadinessManager } from '../../../readiness/types'; -import { ISplitsCacheSync, IStorageSync } from '../../../storages/types'; +import { IStorageSync } from '../../../storages/types'; import { ISplitsParser } from '../splitsParser/types'; import { ISplit, ISplitPartial } from '../../../dtos/types'; import { syncTaskFactory } from '../../syncTask'; @@ -81,7 +81,7 @@ export function fromObjectUpdaterFactory( */ export function fromObjectSyncTaskFactory( splitsParser: ISplitsParser, - storage: { splits: ISplitsCacheSync }, + storage: Pick, readiness: IReadinessManager, settings: ISettings ): ISyncTask<[], boolean> {