Add onReadyFromCacheCb to storage factory params for code cleanup

Author: EmilianoSanchez

src/sdkFactory/index.ts

@@ -7,7 +7,7 @@ import { IBasicClient, SplitIO } from '../types';
 import { validateAndTrackApiKey } from '../utils/inputValidation/apiKey';
 import { createLoggerAPI } from '../logger/sdkLogger';
 import { NEW_FACTORY, RETRIEVE_MANAGER } from '../logger/constants';
-import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED } from '../readiness/constants';
+import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED, SDK_SPLITS_CACHE_LOADED } from '../readiness/constants';
 import { objectAssign } from '../utils/lang/objectAssign';
 import { strategyDebugFactory } from '../trackers/strategy/strategyDebug';
 import { strategyOptimizedFactory } from '../trackers/strategy/strategyOptimized';
@@ -46,6 +46,9 @@ export function sdkFactory(params: ISdkFactoryParams): SplitIO.ICsSDK | SplitIO.
       readiness.splits.emit(SDK_SPLITS_ARRIVED);
       readiness.segments.emit(SDK_SEGMENTS_ARRIVED);
     },
+    onReadyFromCacheCb: () => {
+      readiness.splits.emit(SDK_SPLITS_CACHE_LOADED);
+    }
   });
   // @TODO add support for dataloader: `if (params.dataLoader) params.dataLoader(storage);`
   const clients: Record<string, IBasicClient> = {};

src/storages/AbstractSplitsCacheAsync.ts

@@ -28,14 +28,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

@@ -48,14 +48,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

@@ -217,15 +217,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`,
    *
@@ -250,7 +241,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

@@ -31,16 +31,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

@@ -12,7 +12,7 @@ import { SplitsCacheInMemory } from '../inMemory/SplitsCacheInMemory';
 import { DEFAULT_CACHE_EXPIRATION_IN_MILLIS } from '../../utils/constants/browser';
 import { InMemoryStorageCSFactory } from '../inMemory/InMemoryStorageCS';
 import { LOG_PREFIX } from './constants';
-import { DEBUG, NONE, STORAGE_LOCALSTORAGE } from '../../utils/constants';
+import { DEBUG, LOCALHOST_MODE, NONE, STORAGE_LOCALSTORAGE } from '../../utils/constants';
 import { shouldRecordTelemetry, TelemetryCacheInMemory } from '../inMemory/TelemetryCacheInMemory';
 import { UniqueKeysCacheInMemoryCS } from '../inMemory/UniqueKeysCacheInMemoryCS';
 import { getMatching } from '../../utils/key';
@@ -36,7 +36,7 @@ export function InLocalStorage(options: InLocalStorageOptions = {}): IStorageSyn
       return InMemoryStorageCSFactory(params);
     }
 
-    const { settings, settings: { log, scheduler: { impressionsQueueSize, eventsQueueSize, }, sync: { impressionsMode, __splitFiltersValidation } } } = params;
+    const { onReadyFromCacheCb, settings, settings: { log, scheduler: { impressionsQueueSize, eventsQueueSize, }, sync: { impressionsMode, __splitFiltersValidation } } } = params;
     const matchingKey = getMatching(settings.core.key);
     const keys = new KeyBuilderCS(prefix, matchingKey);
     const expirationTimestamp = Date.now() - DEFAULT_CACHE_EXPIRATION_IN_MILLIS;
@@ -45,6 +45,10 @@ export function InLocalStorage(options: InLocalStorageOptions = {}): IStorageSyn
     const segments = new MySegmentsCacheInLocal(log, keys);
     const largeSegments = new MySegmentsCacheInLocal(log, myLargeSegmentsKeyBuilder(prefix, matchingKey));
 
+    if (settings.mode === LOCALHOST_MODE || splits.getChangeNumber() > -1) {
+      Promise.resolve().then(onReadyFromCacheCb);
+    }
+
     return {
       splits,
       segments,

src/storages/types.ts

@@ -208,8 +208,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<ISet<string>[]>
 }
@@ -226,7 +224,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[]): ISet<string>[]
 }
@@ -243,7 +240,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<ISet<string>[]>
 }
@@ -504,6 +500,7 @@ export interface IStorageFactoryParams {
    * It is meant for emitting SDK_READY event in consumer mode, and waiting before using the storage in the synchronizer.
    */
   onReadyCb: (error?: any) => void,
+  onReadyFromCacheCb: (error?: any) => void,
 }
 
 export type StorageType = 'MEMORY' | 'LOCALSTORAGE' | 'REDIS' | 'PLUGGABLE';

src/sync/offline/syncTasks/fromObjectSyncTask.ts

@@ -7,7 +7,7 @@ import { syncTaskFactory } from '../../syncTask';
 import { ISyncTask } from '../../types';
 import { ISettings } from '../../../types';
 import { CONTROL } from '../../../utils/constants';
-import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED, SDK_SPLITS_CACHE_LOADED } from '../../../readiness/constants';
+import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED } from '../../../readiness/constants';
 import { SYNC_OFFLINE_DATA, ERROR_SYNC_OFFLINE_LOADING } from '../../../logger/constants';
 
 /**
@@ -60,12 +60,8 @@ export function fromObjectUpdaterFactory(
 
         if (startingUp) {
           startingUp = false;
-          Promise.resolve(splitsCache.checkCache()).then(cacheReady => {
-            // Emits SDK_READY_FROM_CACHE
-            if (cacheReady) readiness.splits.emit(SDK_SPLITS_CACHE_LOADED);
-            // Emits SDK_READY
-            readiness.segments.emit(SDK_SEGMENTS_ARRIVED);
-          });
+          // Emits SDK_READY
+          readiness.segments.emit(SDK_SEGMENTS_ARRIVED);
         }
         return true;
       });

src/sync/polling/updaters/splitChangesUpdater.ts

@@ -4,7 +4,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,8 @@ 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 +201,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/utils/settingsValidation/storage/__tests__/storageCS.spec.ts

@@ -1,4 +1,4 @@
-import { validateStorageCS, __InLocalStorageMockFactory } from '../storageCS';
+import { validateStorageCS } from '../storageCS';
 import { InMemoryStorageCSFactory } from '../../../../storages/inMemory/InMemoryStorageCS';
 import { loggerMock as log } from '../../../../logger/__tests__/sdkLogger.mock';
 
@@ -32,11 +32,6 @@ describe('storage validator for pluggable storage (client-side)', () => {
     expect(log.error).not.toBeCalled();
   });
 
-  test('fallbacks to mock InLocalStorage storage if the storage is InLocalStorage and the mode localhost', () => {
-    expect(validateStorageCS({ log, mode: 'localhost', storage: mockInLocalStorageFactory })).toBe(__InLocalStorageMockFactory);
-    expect(log.error).not.toBeCalled();
-  });
-
   test('throws error if the provided storage factory is not compatible with the mode', () => {
     expect(() => { validateStorageCS({ log, mode: 'consumer', storage: mockInLocalStorageFactory }); }).toThrow('A PluggableStorage instance is required on consumer mode');
     expect(() => { validateStorageCS({ log, mode: 'consumer_partial', storage: mockInLocalStorageFactory }); }).toThrow('A PluggableStorage instance is required on consumer mode');

src/utils/settingsValidation/storage/storageCS.ts

@@ -3,14 +3,6 @@ import { ISettings, SDKMode } from '../../../types';
 import { ILogger } from '../../../logger/types';
 import { ERROR_STORAGE_INVALID } from '../../../logger/constants';
 import { LOCALHOST_MODE, STANDALONE_MODE, STORAGE_PLUGGABLE, STORAGE_LOCALSTORAGE, STORAGE_MEMORY } from '../../../utils/constants';
-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
-  return result;
-}
-__InLocalStorageMockFactory.type = STORAGE_MEMORY;
 
 /**
  * This function validates `settings.storage` object
@@ -30,11 +22,6 @@ export function validateStorageCS(settings: { log: ILogger, storage?: any, mode:
     log.error(ERROR_STORAGE_INVALID);
   }
 
-  // In localhost mode with InLocalStorage, fallback to a mock InLocalStorage to emit SDK_READY_FROM_CACHE
-  if (mode === LOCALHOST_MODE && storage.type === STORAGE_LOCALSTORAGE) {
-    return __InLocalStorageMockFactory;
-  }
-
   if ([LOCALHOST_MODE, STANDALONE_MODE].indexOf(mode) === -1) {
     // Consumer modes require an async storage
     if (storage.type !== STORAGE_PLUGGABLE) throw new Error('A PluggableStorage instance is required on consumer mode');