Place 'setChangeNumber' call as last operation inside storage 'update' methods, to signal transaction commit

Author: EmilianoSanchez

src/storages/AbstractMySegmentsCacheSync.ts

@@ -49,12 +49,10 @@ export abstract class AbstractMySegmentsCacheSync implements ISegmentsCacheSync
    * For client-side synchronizer: it resets or updates the cache.
    */
   resetSegments(segmentsData: MySegmentsData | IMySegmentsResponse): boolean {
-    this.setChangeNumber(segmentsData.cn);
-
     const { added, removed } = segmentsData as MySegmentsData;
+    let isDiff = false;
 
     if (added && removed) {
-      let isDiff = false;
 
       added.forEach(segment => {
         isDiff = this.addSegment(segment) || isDiff;
@@ -63,32 +61,40 @@ export abstract class AbstractMySegmentsCacheSync implements ISegmentsCacheSync
       removed.forEach(segment => {
         isDiff = this.removeSegment(segment) || isDiff;
       });
+    } else {
 
-      return isDiff;
-    }
+      const names = ((segmentsData as IMySegmentsResponse).k || []).map(s => s.n).sort();
+      const storedSegmentKeys = this.getRegisteredSegments().sort();
 
-    const names = ((segmentsData as IMySegmentsResponse).k || []).map(s => s.n).sort();
-    const storedSegmentKeys = this.getRegisteredSegments().sort();
+      // Extreme fast => everything is empty
+      if (!names.length && !storedSegmentKeys.length) {
+        isDiff = false;
+      } else {
 
-    // Extreme fast => everything is empty
-    if (!names.length && !storedSegmentKeys.length) return false;
+        let index = 0;
 
-    let index = 0;
+        while (index < names.length && index < storedSegmentKeys.length && names[index] === storedSegmentKeys[index]) index++;
 
-    while (index < names.length && index < storedSegmentKeys.length && names[index] === storedSegmentKeys[index]) index++;
+        // Quick path => no changes
+        if (index === names.length && index === storedSegmentKeys.length) {
+          isDiff = false;
+        } else {
 
-    // Quick path => no changes
-    if (index === names.length && index === storedSegmentKeys.length) return false;
+          // Slowest path => add and/or remove segments
+          for (let removeIndex = index; removeIndex < storedSegmentKeys.length; removeIndex++) {
+            this.removeSegment(storedSegmentKeys[removeIndex]);
+          }
 
-    // Slowest path => add and/or remove segments
-    for (let removeIndex = index; removeIndex < storedSegmentKeys.length; removeIndex++) {
-      this.removeSegment(storedSegmentKeys[removeIndex]);
-    }
+          for (let addIndex = index; addIndex < names.length; addIndex++) {
+            this.addSegment(names[addIndex]);
+          }
 
-    for (let addIndex = index; addIndex < names.length; addIndex++) {
-      this.addSegment(names[addIndex]);
+          isDiff = true;
+        }
+      }
     }
 
-    return true;
+    this.setChangeNumber(segmentsData.cn);
+    return isDiff;
   }
 }

src/storages/AbstractSplitsCacheSync.ts

@@ -14,9 +14,10 @@ export abstract class AbstractSplitsCacheSync implements ISplitsCacheSync {
   protected abstract setChangeNumber(changeNumber: number): boolean | void
 
   update(toAdd: ISplit[], toRemove: ISplit[], changeNumber: number): boolean {
+    let updated = toAdd.map(addedFF => this.addSplit(addedFF)).some(result => result);
+    updated = toRemove.map(removedFF => this.removeSplit(removedFF.name)).some(result => result) || updated;
     this.setChangeNumber(changeNumber);
-    const updated = toAdd.map(addedFF => this.addSplit(addedFF)).some(result => result);
-    return toRemove.map(removedFF => this.removeSplit(removedFF.name)).some(result => result) || updated;
+    return updated;
   }
 
   abstract getSplit(name: string): ISplit | null

src/storages/inLocalStorage/RBSegmentsCacheInLocal.ts

@@ -27,9 +27,10 @@ export class RBSegmentsCacheInLocal implements IRBSegmentsCacheSync {
   }
 
   update(toAdd: IRBSegment[], toRemove: IRBSegment[], changeNumber: number): boolean {
+    let updated = toAdd.map(toAdd => this.add(toAdd)).some(result => result);
+    updated = toRemove.map(toRemove => this.remove(toRemove.name)).some(result => result) || updated;
     this.setChangeNumber(changeNumber);
-    const updated = toAdd.map(toAdd => this.add(toAdd)).some(result => result);
-    return toRemove.map(toRemove => this.remove(toRemove.name)).some(result => result) || updated;
+    return updated;
   }
 
   private setChangeNumber(changeNumber: number) {

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

@@ -29,7 +29,7 @@ describe('validateCache', () => {
   });
 
   test('if there is no cache, it should return false', async () => {
-    expect(await validateCache({ storage: localStorage }, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
+    expect(await validateCache({}, localStorage, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
 
     expect(logSpy).not.toHaveBeenCalled();
 
@@ -47,7 +47,7 @@ describe('validateCache', () => {
     localStorage.setItem(keys.buildSplitsTillKey(), '1');
     localStorage.setItem(keys.buildHashKey(), FULL_SETTINGS_HASH);
 
-    expect(await validateCache({ storage: localStorage }, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(true);
+    expect(await validateCache({}, localStorage, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(true);
 
     expect(logSpy).not.toHaveBeenCalled();
 
@@ -66,7 +66,7 @@ describe('validateCache', () => {
     localStorage.setItem(keys.buildHashKey(), FULL_SETTINGS_HASH);
     localStorage.setItem(keys.buildLastUpdatedKey(), Date.now() - 1000 * 60 * 60 * 24 * 2 + ''); // 2 days ago
 
-    expect(await validateCache({ storage: localStorage, expirationDays: 1 }, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
+    expect(await validateCache({ expirationDays: 1 }, localStorage, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
 
     expect(logSpy).toHaveBeenCalledWith('storage:localstorage: Cache expired more than 1 days ago. Cleaning up cache');
 
@@ -83,7 +83,7 @@ describe('validateCache', () => {
     localStorage.setItem(keys.buildSplitsTillKey(), '1');
     localStorage.setItem(keys.buildHashKey(), FULL_SETTINGS_HASH);
 
-    expect(await validateCache({ storage: localStorage }, { ...fullSettings, core: { ...fullSettings.core, authorizationKey: 'another-sdk-key' } }, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
+    expect(await validateCache({}, localStorage, { ...fullSettings, core: { ...fullSettings.core, authorizationKey: 'another-sdk-key' } }, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
 
     expect(logSpy).toHaveBeenCalledWith('storage:localstorage: SDK key, flags filter criteria, or flags spec version has changed. Cleaning up cache');
 
@@ -101,7 +101,7 @@ describe('validateCache', () => {
     localStorage.setItem(keys.buildSplitsTillKey(), '1');
     localStorage.setItem(keys.buildHashKey(), FULL_SETTINGS_HASH);
 
-    expect(await validateCache({ storage: localStorage, clearOnInit: true }, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
+    expect(await validateCache({ clearOnInit: true }, localStorage, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
 
     expect(logSpy).toHaveBeenCalledWith('storage:localstorage: clearOnInit was set and cache was not cleared in the last 24 hours. Cleaning up cache');
 
@@ -117,13 +117,13 @@ describe('validateCache', () => {
     // If cache is cleared, it should not clear again until a day has passed
     logSpy.mockClear();
     localStorage.setItem(keys.buildSplitsTillKey(), '1');
-    expect(await validateCache({ storage: localStorage, clearOnInit: true }, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(true);
+    expect(await validateCache({ clearOnInit: true }, localStorage, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(true);
     expect(logSpy).not.toHaveBeenCalled();
     expect(localStorage.getItem(keys.buildLastClear())).toBe(lastClear); // Last clear should not have changed
 
     // If a day has passed, it should clear again
     localStorage.setItem(keys.buildLastClear(), (Date.now() - 1000 * 60 * 60 * 24 - 1) + '');
-    expect(await validateCache({ storage: localStorage, clearOnInit: true }, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
+    expect(await validateCache({ clearOnInit: true }, localStorage, fullSettings, keys, splits, rbSegments, segments, largeSegments)).toBe(false);
     expect(logSpy).toHaveBeenCalledWith('storage:localstorage: clearOnInit was set and cache was not cleared in the last 24 hours. Cleaning up cache');
     expect(splits.clear).toHaveBeenCalledTimes(2);
     expect(rbSegments.clear).toHaveBeenCalledTimes(2);

src/storages/inLocalStorage/index.ts

@@ -63,7 +63,7 @@ export function InLocalStorage(options: SplitIO.InLocalStorageOptions = {}): ISt
       uniqueKeys: new UniqueKeysCacheInMemoryCS(),
 
       validateCache() {
-        return validateCachePromise || (validateCachePromise = validateCache({ ...options, storage }, settings, keys, splits, rbSegments, segments, largeSegments));
+        return validateCachePromise || (validateCachePromise = validateCache(options, storage, settings, keys, splits, rbSegments, segments, largeSegments));
       },
 
       destroy() {

src/storages/inLocalStorage/validateCache.ts

@@ -16,9 +16,8 @@ const MILLIS_IN_A_DAY = 86400000;
  *
  * @returns `true` if cache should be cleared, `false` otherwise
  */
-function validateExpiration(options: SplitIO.InLocalStorageOptions & { storage: SplitIO.Storage }, settings: ISettings, keys: KeyBuilderCS, currentTimestamp: number, isThereCache: boolean) {
+function validateExpiration(options: SplitIO.InLocalStorageOptions, storage: SplitIO.Storage, settings: ISettings, keys: KeyBuilderCS, currentTimestamp: number, isThereCache: boolean) {
   const { log } = settings;
-  const { storage } = options;
 
   // Check expiration
   const lastUpdatedTimestamp = parseInt(storage.getItem(keys.buildLastUpdatedKey()) as string, 10);
@@ -68,28 +67,29 @@ function validateExpiration(options: SplitIO.InLocalStorageOptions & { storage:
  *
  * @returns `true` if cache is ready to be used, `false` otherwise (cache was cleared or there is no cache)
  */
-export function validateCache(options: SplitIO.InLocalStorageOptions & { storage: SplitIO.Storage }, settings: ISettings, keys: KeyBuilderCS, splits: SplitsCacheInLocal, rbSegments: RBSegmentsCacheInLocal, segments: MySegmentsCacheInLocal, largeSegments: MySegmentsCacheInLocal): Promise<boolean> {
-  return new Promise<boolean>((resolve) => {
+export function validateCache(options: SplitIO.InLocalStorageOptions, storage: SplitIO.Storage, settings: ISettings, keys: KeyBuilderCS, splits: SplitsCacheInLocal, rbSegments: RBSegmentsCacheInLocal, segments: MySegmentsCacheInLocal, largeSegments: MySegmentsCacheInLocal): Promise<boolean> {
+
+  return Promise.resolve().then(() => {
     const currentTimestamp = Date.now();
     const isThereCache = splits.getChangeNumber() > -1;
 
-    if (validateExpiration(options, settings, keys, currentTimestamp, isThereCache)) {
+    if (validateExpiration(options, storage, settings, keys, currentTimestamp, isThereCache)) {
       splits.clear();
       rbSegments.clear();
       segments.clear();
       largeSegments.clear();
 
       // Update last clear timestamp
       try {
-        options.storage.setItem(keys.buildLastClear(), currentTimestamp + '');
+        storage.setItem(keys.buildLastClear(), currentTimestamp + '');
       } catch (e) {
         settings.log.error(LOG_PREFIX + e);
       }
 
-      resolve(false);
+      return false;
     }
 
     // Check if ready from cache
-    resolve(isThereCache);
+    return isThereCache;
   });
 }

types/splitio.d.ts

@@ -1012,7 +1012,7 @@ declare namespace SplitIO {
      */
     clearOnInit?: boolean;
     /**
-     * Optional storage API to use. If not provided, the SDK will use the default localStorage Web API.
+     * Optional storage API to persist rollout plan related data. If not provided, the SDK will use the default localStorage Web API.
      *
      * @defaultValue `window.localStorage`
      */