Revert "Add onReadyFromCacheCb to storage factory params for code cleanup"

This reverts commit 38092dc.

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, SDK_SPLITS_CACHE_LOADED } from '../readiness/constants';
+import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED } from '../readiness/constants';
 import { objectAssign } from '../utils/lang/objectAssign';
 import { strategyDebugFactory } from '../trackers/strategy/strategyDebug';
 import { strategyOptimizedFactory } from '../trackers/strategy/strategyOptimized';
@@ -52,9 +52,6 @@ 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,6 +28,14 @@ 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,6 +48,14 @@ 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,6 +217,15 @@ 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`,
    *
@@ -241,7 +250,7 @@ export class SplitsCacheInLocal extends AbstractSplitsCacheSync {
         this.updateNewFilter = true;
 
         // if there is cache, clear it
-        if (this.getChangeNumber() > -1) this.clear();
+        if (this.checkCache()) this.clear();
 
       } catch (e) {
         this.log.error(LOG_PREFIX + e);

src/storages/inLocalStorage/__tests__/SplitsCacheInLocal.spec.ts

@@ -31,10 +31,16 @@ 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, LOCALHOST_MODE, NONE, STORAGE_LOCALSTORAGE } from '../../utils/constants';
+import { DEBUG, 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 { onReadyFromCacheCb, settings, settings: { log, scheduler: { impressionsQueueSize, eventsQueueSize, }, sync: { impressionsMode, __splitFiltersValidation } } } = params;
+    const { 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;
@@ -55,12 +55,6 @@ export function InLocalStorage(options: InLocalStorageOptions = {}): IStorageSyn
       telemetry: shouldRecordTelemetry(params) ? new TelemetryCacheInMemory(splits, segments) : undefined,
       uniqueKeys: impressionsMode === NONE ? new UniqueKeysCacheInMemoryCS() : undefined,
 
-      init() {
-        if (settings.mode === LOCALHOST_MODE || splits.getChangeNumber() > -1) {
-          Promise.resolve().then(onReadyFromCacheCb);
-        }
-      },
-
       destroy() {
         this.splits = new SplitsCacheInMemory(__splitFiltersValidation);
         this.segments = new MySegmentsCacheInMemory();

src/storages/types.ts

@@ -208,6 +208,8 @@ 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>[]>
 }
@@ -224,6 +226,7 @@ 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>[]
 }
@@ -240,6 +243,7 @@ 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>[]>
 }
@@ -501,7 +505,6 @@ 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 } from '../../../readiness/constants';
+import { SDK_SPLITS_ARRIVED, SDK_SEGMENTS_ARRIVED, SDK_SPLITS_CACHE_LOADED } from '../../../readiness/constants';
 import { SYNC_OFFLINE_DATA, ERROR_SYNC_OFFLINE_LOADING } from '../../../logger/constants';
 
 /**
@@ -60,8 +60,12 @@ export function fromObjectUpdaterFactory(
 
         if (startingUp) {
           startingUp = false;
-          // Emits SDK_READY
-          readiness.segments.emit(SDK_SEGMENTS_ARRIVED);
+          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);
+          });
         }
         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 } from '../../../readiness/constants';
+import { SDK_SPLITS_ARRIVED, SDK_SPLITS_CACHE_LOADED } 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,8 +153,7 @@ export function splitChangesUpdaterFactory(
      */
     function _splitChangesUpdater(since: number, retry = 0): Promise<boolean> {
       log.debug(SYNC_SPLITS_FETCH, [since]);
-
-      return Promise.resolve(splitUpdateNotification ?
+      const fetcherPromise = Promise.resolve(splitUpdateNotification ?
         { splits: [splitUpdateNotification.payload], till: splitUpdateNotification.changeNumber } :
         splitChangesFetcher(since, noCache, till, _promiseDecorator)
       )
@@ -201,6 +200,15 @@ 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 } from '../storageCS';
+import { validateStorageCS, __InLocalStorageMockFactory } from '../storageCS';
 import { InMemoryStorageCSFactory } from '../../../../storages/inMemory/InMemoryStorageCS';
 import { loggerMock as log } from '../../../../logger/__tests__/sdkLogger.mock';
 
@@ -32,6 +32,11 @@ 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,6 +3,14 @@ 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
@@ -22,6 +30,11 @@ 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');