Merge pull request #378 from splitio/cache_expiration_move_validateCache_call

[Cache expiration] Integrate with synchronization start flow

Author: EmilianoSanchez

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<boolean> {
-    return Promise.resolve(false);
-  }
-
   /**
    * Kill `name` split and set `defaultTreatment` and `changeNumber`.
    * Used for SPLIT_KILL push notifications.

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.

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);

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);
 
 });

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).

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<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>[]>
 }
@@ -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>[]
 }
@@ -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>[]>
 }
@@ -457,6 +453,7 @@ export interface IStorageSync extends IStorageBase<
   IUniqueKeysCacheSync
 > {
   // Defined in client-side
+  validateCache?: () => boolean, // @TODO support async
   largeSegments?: ISegmentsCacheSync,
 }
 

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: {},
+  });
   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: { validateCache: () => 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: { validateCache: () => 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: { validateCache: () => 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();
+});

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 { 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<IStorageSync, 'splits' | 'validateCache'>,
   readiness: IReadinessManager,
   settings: ISettings,
 ): () => Promise<boolean> {
@@ -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);
           });
@@ -80,7 +81,7 @@ export function fromObjectUpdaterFactory(
  */
 export function fromObjectSyncTaskFactory(
   splitsParser: ISplitsParser,
-  storage: { splits: ISplitsCacheSync },
+  storage: Pick<IStorageSync, 'splits' | 'validateCache'>,
   readiness: IReadinessManager,
   settings: ISettings
 ): ISyncTask<[], boolean> {

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<boolean> {
       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

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,11 @@ export function syncManagerOnlineFactory(
       start() {
         running = true;
 
+        if (startFirstTime) {
+          const isCacheLoaded = storage.validateCache ? storage.validateCache() : false;
+          if (isCacheLoaded) Promise.resolve().then(() => { readiness.splits.emit(SDK_SPLITS_CACHE_LOADED); });
+        }
+
         // start syncing splits and segments
         if (pollingManager) {
 
@@ -96,7 +102,6 @@ export function syncManagerOnlineFactory(
               // Doesn't call `syncAll` when the syncManager is resuming
               if (startFirstTime) {
                 pollingManager.syncAll();
-                startFirstTime = false;
               }
               pushManager.start();
             } else {
@@ -105,13 +110,14 @@ export function syncManagerOnlineFactory(
           } else {
             if (startFirstTime) {
               pollingManager.syncAll();
-              startFirstTime = false;
             }
           }
         }
 
         // start periodic data recording (events, impressions, telemetry).
         submitterManager.start(!isConsentGranted(settings));
+
+        startFirstTime = false;
       },
 
       /**