Simplify fetching logic (#176)

- Remove retry-logic that was too complicated. We will no longer cache
negative responses at all.
- Increase expire time from 7 to 30 days.
- Don't block on LiveSatisfaction initialization

Author: roncohen

packages/browser-sdk/src/client.ts

@@ -73,6 +73,7 @@ export class BucketClient {
   private httpClient: HttpClient;
 
   private liveSatisfaction: LiveSatisfaction | undefined;
+  private liveSatisfactionInit: Promise<void> | undefined;
   private featuresClient: FeaturesClient;
 
   constructor(
@@ -135,11 +136,16 @@ export class BucketClient {
    * Must be called before calling other SDK methods.
    */
   async initialize() {
-    const inits = [this.featuresClient.initialize()];
     if (this.liveSatisfaction) {
-      inits.push(this.liveSatisfaction.initialize());
+      // do not block on live satisfaction initialization
+      this.liveSatisfactionInit = this.liveSatisfaction
+        .initialize()
+        .catch((e) => {
+          this.logger.error("error initializing live satisfaction", e);
+        });
     }
-    await Promise.all(inits);
+
+    await this.featuresClient.initialize();
 
     this.logger.debug(
       `initialized with key "${this.publishableKey}" and options`,
@@ -312,8 +318,10 @@ export class BucketClient {
     return this.featuresClient.getFeatures();
   }
 
-  stop() {
+  async stop() {
     if (this.liveSatisfaction) {
+      // ensure fully initialized before stopping
+      await this.liveSatisfactionInit;
       this.liveSatisfaction.stop();
     }
   }

packages/browser-sdk/src/feature/featureCache.ts

@@ -8,9 +8,7 @@ interface StorageItem {
 interface cacheEntry {
   expireAt: number;
   staleAt: number;
-  success: boolean; // we also want to cache failures to avoid the UI waiting and spamming the API
-  features: APIFeaturesResponse | undefined;
-  attemptCount: number;
+  features: APIFeaturesResponse;
 }
 
 // Parse and validate an API feature response
@@ -41,10 +39,8 @@ export function parseAPIFeaturesResponse(
 }
 
 export interface CacheResult {
-  features: APIFeaturesResponse | undefined;
+  features: APIFeaturesResponse;
   stale: boolean;
-  success: boolean;
-  attemptCount: number;
 }
 
 export class FeatureCache {
@@ -69,13 +65,9 @@ export class FeatureCache {
   set(
     key: string,
     {
-      success,
       features,
-      attemptCount,
     }: {
-      success: boolean;
-      features?: APIFeaturesResponse;
-      attemptCount: number;
+      features: APIFeaturesResponse;
     },
   ) {
     let cacheData: CacheData = {};
@@ -93,8 +85,6 @@ export class FeatureCache {
       expireAt: Date.now() + this.expireTimeMs,
       staleAt: Date.now() + this.staleTimeMs,
       features,
-      success,
-      attemptCount,
     } satisfies cacheEntry;
 
     cacheData = Object.fromEntries(
@@ -118,9 +108,7 @@ export class FeatureCache {
         ) {
           return {
             features: cachedResponse[key].features,
-            success: cachedResponse[key].success,
             stale: cachedResponse[key].staleAt < Date.now(),
-            attemptCount: cachedResponse[key].attemptCount,
           };
         }
       }
@@ -145,8 +133,6 @@ function validateCacheData(cacheDataInput: any) {
     if (
       typeof cacheEntry.expireAt !== "number" ||
       typeof cacheEntry.staleAt !== "number" ||
-      typeof cacheEntry.success !== "boolean" ||
-      typeof cacheEntry.attemptCount !== "number" ||
       (cacheEntry.features && !parseAPIFeaturesResponse(cacheEntry.features))
     ) {
       return;
@@ -155,9 +141,7 @@ function validateCacheData(cacheDataInput: any) {
     cacheData[key] = {
       expireAt: cacheEntry.expireAt,
       staleAt: cacheEntry.staleAt,
-      success: cacheEntry.success,
       features: cacheEntry.features,
-      attemptCount: cacheEntry.attemptCount,
     };
   }
   return cacheData;

packages/browser-sdk/src/feature/features.ts

@@ -25,21 +25,18 @@ export type FeaturesOptions = {
   fallbackFeatures?: string[];
   timeoutMs?: number;
   staleWhileRevalidate?: boolean;
-  failureRetryAttempts?: number | false;
 };
 
 type Config = {
   fallbackFeatures: string[];
   timeoutMs: number;
   staleWhileRevalidate: boolean;
-  failureRetryAttempts: number | false;
 };
 
 export const DEFAULT_FEATURES_CONFIG: Config = {
   fallbackFeatures: [],
   timeoutMs: 5000,
   staleWhileRevalidate: false,
-  failureRetryAttempts: false,
 };
 
 // Deep merge two objects.
@@ -108,7 +105,7 @@ export function clearFeatureCache() {
 }
 
 export const FEATURES_STALE_MS = 60000; // turn stale after 60 seconds, optionally reevaluate in the background
-export const FEATURES_EXPIRE_MS = 7 * 24 * 60 * 60 * 1000; // expire entirely after 7 days
+export const FEATURES_EXPIRE_MS = 30 * 24 * 60 * 60 * 1000; // expire entirely after 30 days
 
 const localStorageCacheKey = `__bucket_features`;
 
@@ -147,6 +144,10 @@ export class FeaturesClient {
 
   async initialize() {
     const features = (await this.maybeFetchFeatures()) || {};
+    this.setFeatures(features);
+  }
+
+  private setFeatures(features: APIFeaturesResponse) {
     const proxiedFeatures = maskedProxy(features, (fs, key) => {
       this.sendCheckEvent({
         key,
@@ -177,40 +178,60 @@ export class FeaturesClient {
   }
 
   private async maybeFetchFeatures(): Promise<APIFeaturesResponse | undefined> {
-    const cachedItem = this.cache.get(this.fetchParams().toString());
-
-    // if there's no cached item OR the cached item is a failure and we haven't retried
-    // too many times yet - fetch now
-    if (
-      !cachedItem ||
-      (!cachedItem.success &&
-        (this.config.failureRetryAttempts === false ||
-          cachedItem.attemptCount < this.config.failureRetryAttempts))
-    ) {
-      return await this.fetchFeatures();
-    }
+    const cacheKey = this.fetchParams().toString();
+    const cachedItem = this.cache.get(cacheKey);
+
+    if (cachedItem) {
+      if (!cachedItem.stale) return cachedItem.features;
 
-    // cachedItem is a success or a failed attempt that we've retried too many times
-    if (cachedItem.stale) {
       // serve successful stale cache if `staleWhileRevalidate` is enabled
-      if (this.config.staleWhileRevalidate && cachedItem.success) {
-        // re-fetch in the background, return last successful value
-        this.fetchFeatures().catch(() => {
-          // we don't care about the result, we just want to re-fetch
-        });
+      if (this.config.staleWhileRevalidate) {
+        // re-fetch in the background, but immediately return last successful value
+        this.fetchFeatures()
+          .then((features) => {
+            if (!features) return;
+
+            this.cache.set(cacheKey, {
+              features,
+            });
+            this.setFeatures(features);
+          })
+          .catch(() => {
+            // we don't care about the result, we just want to re-fetch
+          });
         return cachedItem.features;
       }
+    }
+
+    // if there's no cached item or there is a stale one but `staleWhileRevalidate` is disabled
+    // try fetching a new one
+    const fetchedFeatures = await this.fetchFeatures();
+
+    if (fetchedFeatures) {
+      this.cache.set(cacheKey, {
+        features: fetchedFeatures,
+      });
+
+      return fetchedFeatures;
+    }
 
-      return await this.fetchFeatures();
+    if (cachedItem) {
+      // fetch failed, return stale cache
+      return cachedItem.features;
     }
 
-    // serve cached items if not stale and not expired
-    return cachedItem.features;
+    // fetch failed, nothing cached => return fallbacks
+    return this.config.fallbackFeatures.reduce((acc, key) => {
+      acc[key] = {
+        key,
+        isEnabled: true,
+      };
+      return acc;
+    }, {} as APIFeaturesResponse);
   }
 
-  public async fetchFeatures(): Promise<APIFeaturesResponse> {
+  public async fetchFeatures(): Promise<APIFeaturesResponse | undefined> {
     const params = this.fetchParams();
-    const cacheKey = params.toString();
     try {
       const res = await this.httpClient.get({
         path: "/features/enabled",
@@ -238,41 +259,10 @@ export class FeaturesClient {
         throw new Error("unable to validate response");
       }
 
-      this.cache.set(cacheKey, {
-        success: true,
-        features: typeRes.features,
-        attemptCount: 0,
-      });
-
       return typeRes.features;
     } catch (e) {
       this.logger.error("error fetching features: ", e);
-
-      const current = this.cache.get(cacheKey);
-      if (current) {
-        // if there is a previous failure cached, increase the attempt count
-        this.cache.set(cacheKey, {
-          success: current.success,
-          features: current.features,
-          attemptCount: current.attemptCount + 1,
-        });
-      } else {
-        // otherwise cache if the request failed and there is no previous version to extend
-        // to avoid having the UI wait and spam the API
-        this.cache.set(cacheKey, {
-          success: false,
-          features: undefined,
-          attemptCount: 1,
-        });
-      }
-
-      return this.config.fallbackFeatures.reduce((acc, key) => {
-        acc[key] = {
-          key,
-          isEnabled: true,
-        };
-        return acc;
-      }, {} as APIFeaturesResponse);
+      return;
     }
   }
 

packages/browser-sdk/src/feature/maskedProxy.ts

@@ -4,6 +4,9 @@ export default function maskedProxy<T extends object, K extends keyof T, O>(
 ) {
   return new Proxy(obj, {
     get(target: T, prop) {
+      if (typeof prop === "symbol") {
+        return target[prop as K];
+      }
       return valueFunc(target, prop as K);
     },
     set(_target, prop, _value) {

packages/browser-sdk/src/feedback/feedback.ts

@@ -229,6 +229,7 @@ export class LiveSatisfaction {
       this.logger.error("feedback prompting already initialized");
       return;
     }
+    this.initialized = true;
 
     const channel = await this.getChannel();
     if (!channel) return;

packages/browser-sdk/test/featureCache.test.ts

@@ -48,31 +48,17 @@ describe("cache", () => {
   test("caches items", async () => {
     const { cache } = newCache();
 
-    cache.set("key", { success: true, features, attemptCount: 1 });
+    cache.set("key", { features });
     expect(cache.get("key")).toEqual({
       stale: false,
-      success: true,
       features,
-      attemptCount: 1,
-    } satisfies CacheResult);
-  });
-
-  test("caches unsuccessful items", async () => {
-    const { cache } = newCache();
-
-    cache.set("key", { success: false, features, attemptCount: 1 });
-    expect(cache.get("key")).toEqual({
-      stale: false,
-      success: false,
-      features,
-      attemptCount: 1,
     } satisfies CacheResult);
   });
 
   test("sets stale", async () => {
     const { cache } = newCache();
 
-    cache.set("key", { success: true, features, attemptCount: 1 });
+    cache.set("key", { features });
 
     vitest.advanceTimersByTime(TEST_STALE_MS + 1);
 
@@ -84,17 +70,13 @@ describe("cache", () => {
     const { cache, cacheItem } = newCache();
 
     cache.set("first key", {
-      success: true,
       features,
-      attemptCount: 1,
     });
     expect(cacheItem[0]).not.toBeNull();
     vitest.advanceTimersByTime(TEST_EXPIRE_MS + 1);
 
     cache.set("other key", {
-      success: true,
       features,
-      attemptCount: 1,
     });
 
     const item = cache.get("key");