use single supplier so hook is debounced in entirety

Signed-off-by: Todd Baert <[email protected]>

Author: toddbaert

libs/hooks/debounce/src/lib/debounce-hook.spec.ts

@@ -14,13 +14,7 @@ describe('DebounceHook', () => {
       finally: jest.fn(),
     };
 
-    const supplier = (flagKey: string) => flagKey;
-
     const hook = new DebounceHook<string>(innerHook, {
-      beforeCacheKeySupplier: supplier,
-      afterCacheKeySupplier: supplier,
-      errorCacheKeySupplier: supplier,
-      finallyCacheKeySupplier: supplier,
       debounceTime: 60_000,
       maxCacheItems: 100,
     });
@@ -71,6 +65,31 @@ describe('DebounceHook', () => {
         hints,
       );
     });
+
+    it('stages should be cached independently', () => {
+      const innerHook: Hook = {
+        before: jest.fn(),
+        after: jest.fn(),
+      };
+
+      const hook = new DebounceHook<boolean>(innerHook, {
+        debounceTime: 60_000,
+        maxCacheItems: 100,
+      });
+
+      const flagKey = 'my-flag';
+
+      hook.before({ flagKey } as HookContext, {});
+      hook.after({ flagKey } as HookContext, {
+        flagKey,
+        flagMetadata: {},
+        value: true,
+      });
+
+      // both should run
+      expect(innerHook.before).toHaveBeenCalledTimes(1);
+      expect(innerHook.after).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe('options', () => {
@@ -84,7 +103,6 @@ describe('DebounceHook', () => {
       };
 
       const hook = new DebounceHook<string>(innerHook, {
-        beforeCacheKeySupplier: (flagKey: string) => flagKey,
         debounceTime: 60_000,
         maxCacheItems: 1,
       });
@@ -105,7 +123,6 @@ describe('DebounceHook', () => {
       const flagKey = 'some-flag';
 
       const hook = new DebounceHook<string>(innerHook, {
-        beforeCacheKeySupplier: (flagKey: string) => flagKey,
         debounceTime: 500,
         maxCacheItems: 1,
       });
@@ -122,40 +139,28 @@ describe('DebounceHook', () => {
       expect(innerHook.before).toHaveBeenCalledTimes(2);
     });
 
-    it('noop if supplier not defined', () => {
+    it('use custom supplier', () => {
       const innerHook: Hook = {
         before: jest.fn(),
         after: jest.fn(),
         error: jest.fn(),
         finally: jest.fn(),
       };
 
-      const flagKey = 'some-flag';
       const context = {};
       const hints = {};
 
-      // no suppliers defined, so we no-op (do no caching)
       const hook = new DebounceHook<string>(innerHook, {
+        cacheKeySupplier: () => 'a-silly-const-key', // a constant key means all invocations are cached; just to test that the custom supplier is used
         debounceTime: 60_000,
         maxCacheItems: 100,
       });
 
-      const evaluationDetails: EvaluationDetails<string> = {
-        value: 'testValue',
-      } as EvaluationDetails<string>;
-
-      for (let i = 0; i < 3; i++) {
-        hook.before({ flagKey, context } as HookContext, hints);
-        hook.after({ flagKey, context } as HookContext, evaluationDetails, hints);
-        hook.error({ flagKey, context } as HookContext, hints);
-        hook.finally({ flagKey, context } as HookContext, evaluationDetails, hints);
-      }
+      hook.before({ flagKey: 'flag1', context } as HookContext, hints);
+      hook.before({ flagKey: 'flag2', context } as HookContext, hints);
 
-      // every invocation should have run since we have only maxCacheItems: 1
-      expect(innerHook.before).toHaveBeenCalledTimes(3);
-      expect(innerHook.after).toHaveBeenCalledTimes(3);
-      expect(innerHook.error).toHaveBeenCalledTimes(3);
-      expect(innerHook.finally).toHaveBeenCalledTimes(3);
+      // since we used a constant key, the second invocation should have been cached even though the flagKey was different
+      expect(innerHook.before).toHaveBeenCalledTimes(1);
     });
 
     it.each([
@@ -180,7 +185,6 @@ describe('DebounceHook', () => {
 
       // this hook caches error invocations
       const hook = new DebounceHook<string>(innerErrorHook, {
-        beforeCacheKeySupplier: (flagKey: string) => flagKey,
         maxCacheItems: 100,
         debounceTime: 60_000,
         cacheErrors,

libs/hooks/debounce/src/lib/debounce-hook.ts

@@ -1,17 +1,24 @@
-import type {
-  EvaluationContext,
-  EvaluationDetails,
-  FlagValue,
-  Hook,
-  HookContext,
-  HookHints,
+import type { Logger } from '@openfeature/web-sdk';
+import {
+  ErrorCode,
+  OpenFeatureError,
+  type EvaluationContext,
+  type EvaluationDetails,
+  type FlagValue,
+  type Hook,
+  type HookContext,
+  type HookHints,
 } from '@openfeature/web-sdk';
 import { FixedSizeExpiringCache } from './utils/fixed-size-expiring-cache';
 
+const DEFAULT_CACHE_KEY_SUPPLIER = (flagKey: string) => flagKey;
+type StageResult = true | CachedError;
+type HookStagesEntry = { before?: StageResult; after?: StageResult; error?: StageResult; finally?: StageResult };
+
 /**
  * An error cached from a previous hook invocation.
  */
-export class CachedError extends Error {
+export class CachedError extends OpenFeatureError {
   private _innerError: unknown;
 
   constructor(innerError: unknown) {
@@ -27,60 +34,27 @@ export class CachedError extends Error {
   get innerError() {
     return this._innerError;
   }
+
+  get code() {
+    if (this._innerError instanceof OpenFeatureError) {
+      return this._innerError.code;
+    }
+    return ErrorCode.GENERAL;
+  }
 }
 
-export type Options<T extends FlagValue = FlagValue> = {
+export type Options = {
   /**
-   * Function to generate the cache key for the before stage of the wrapped hook.
+   * Function to generate the cache key for the wrapped hook.
    * If the cache key is found in the cache, the hook stage will not run.
-   * If not defined, the DebounceHook will no-op for this stage (inner hook will always run for this stage).
+   * By default, the flag key is used as the cache key.
    *
    * @param flagKey the flag key
    * @param context the evaluation context
    * @returns cache key for this stage
+   * @default (flagKey) => flagKey
    */
-  beforeCacheKeySupplier?: (flagKey: string, context: EvaluationContext) => string | null | undefined;
-  /**
-   * Function to generate the cache key for the after stage of the wrapped hook.
-   * If the cache key is found in the cache, the hook stage will not run.
-   * If not defined, the DebounceHook will no-op for this stage (inner hook will always run for this stage).
-   *
-   * @param flagKey the flag key
-   * @param context the evaluation context
-   * @param details the evaluation details
-   * @returns cache key for this stage
-   */
-  afterCacheKeySupplier?: (
-    flagKey: string,
-    context: EvaluationContext,
-    details: EvaluationDetails<T>,
-  ) => string | null | undefined;
-  /**
-   * Function to generate the cache key for the error stage of the wrapped hook.
-   * If the cache key is found in the cache, the hook stage will not run.
-   * If not defined, the DebounceHook will no-op for this stage (inner hook will always run for this stage).
-   *
-   * @param flagKey the flag key
-   * @param context the evaluation context
-   * @param err the Error
-   * @returns cache key for this stage
-   */
-  errorCacheKeySupplier?: (flagKey: string, context: EvaluationContext, err: unknown) => string | null | undefined;
-  /**
-   * Function to generate the cache key for the error stage of the wrapped hook.
-   * If the cache key is found in the cache, the hook stage will not run.
-   * If not defined, the DebounceHook will no-op for this stage (inner hook will always run for this stage).
-   *
-   * @param flagKey the flag key
-   * @param context the evaluation context
-   * @param details the evaluation details
-   * @returns cache key for this stage
-   */
-  finallyCacheKeySupplier?: (
-    flagKey: string,
-    context: EvaluationContext,
-    details: EvaluationDetails<T>,
-  ) => string | null | undefined;
+  cacheKeySupplier?: (flagKey: string, context: EvaluationContext) => string | null | undefined;
   /**
    * Whether or not to debounce and cache the errors thrown by hook stages.
    * If false (default) stages that throw will not be debounced and their errors not cached.
@@ -95,24 +69,29 @@ export type Options<T extends FlagValue = FlagValue> = {
    * Max number of items to be kept in cache before the oldest entry falls out.
    */
   maxCacheItems: number;
+  /**
+   * Optional logger.
+   */
+  logger?: Logger;
 };
 
 /**
  * A hook that wraps another hook and debounces its execution based on the provided options.
- * Each stage of the hook (before, after, error, finally) is debounced independently.
- * If a stage is called with a cache key that has been seen within the debounce time, the inner hook's stage will not run.
+ * The cacheKeySupplier is used to generate a cache key for the hook, which is used to determine if the hook should be executed or skipped.
  * If no cache key supplier is provided for a stage, that stage will always run.
  */
 export class DebounceHook<T extends FlagValue = FlagValue> implements Hook {
-  private readonly cache: FixedSizeExpiringCache<true | CachedError>;
+  private readonly cache: FixedSizeExpiringCache<HookStagesEntry>;
   private readonly cacheErrors: boolean;
+  private readonly cacheKeySupplier: Options['cacheKeySupplier'];
 
   public constructor(
     private readonly innerHook: Hook,
-    private readonly options: Options<T>,
+    private readonly options: Options,
   ) {
     this.cacheErrors = options.cacheErrors ?? false;
-    this.cache = new FixedSizeExpiringCache<true | CachedError>({
+    this.cacheKeySupplier = options.cacheKeySupplier ?? DEFAULT_CACHE_KEY_SUPPLIER;
+    this.cache = new FixedSizeExpiringCache<HookStagesEntry>({
       maxItems: options.maxCacheItems,
       ttlMs: options.debounceTime,
     });
@@ -121,31 +100,31 @@ export class DebounceHook<T extends FlagValue = FlagValue> implements Hook {
   before(hookContext: HookContext, hookHints?: HookHints) {
     this.maybeSkipAndCache(
       'before',
-      () => this.options?.beforeCacheKeySupplier?.(hookContext.flagKey, hookContext.context),
+      () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.before?.(hookContext, hookHints),
     );
   }
 
   after(hookContext: HookContext, evaluationDetails: EvaluationDetails<T>, hookHints?: HookHints) {
     this.maybeSkipAndCache(
       'after',
-      () => this.options?.afterCacheKeySupplier?.(hookContext.flagKey, hookContext.context, evaluationDetails),
+      () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.after?.(hookContext, evaluationDetails, hookHints),
     );
   }
 
   error(hookContext: HookContext, err: unknown, hookHints?: HookHints) {
     this.maybeSkipAndCache(
       'error',
-      () => this.options?.errorCacheKeySupplier?.(hookContext.flagKey, hookContext.context, err),
+      () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.error?.(hookContext, err, hookHints),
     );
   }
 
   finally(hookContext: HookContext, evaluationDetails: EvaluationDetails<T>, hookHints?: HookHints) {
     this.maybeSkipAndCache(
       'finally',
-      () => this.options?.finallyCacheKeySupplier?.(hookContext.flagKey, hookContext.context, evaluationDetails),
+      () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.finally?.(hookContext, evaluationDetails, hookHints),
     );
   }
@@ -156,34 +135,49 @@ export class DebounceHook<T extends FlagValue = FlagValue> implements Hook {
     hookCallback: () => void,
   ) {
     // the cache key is a concatenation of the result of calling keyGenCallback and the stage
-    const dynamicKey = keyGenCallback();
+    let dynamicKey: string | null | undefined;
+
+    try {
+      dynamicKey = keyGenCallback();
+    } catch (e) {
+      // if the keyGenCallback throws, we log and run the hook stage
+      this.options.logger?.error(
+        `DebounceHook: cacheKeySupplier threw an error, running inner hook stage "${stage}" without debouncing.`,
+        e,
+      );
+    }
 
-    // if the keyGenCallback returns nothing, we don't do any caching 
+    // if the keyGenCallback returns nothing, we don't do any caching
     if (!dynamicKey) {
       hookCallback.call(this.innerHook);
+      return;
     }
-    
+
     const cacheKeySuffix = stage;
     const cacheKey = `${dynamicKey}::${cacheKeySuffix}`;
     const got = this.cache.get(cacheKey);
 
     if (got) {
+      const cachedStageResult = got[stage];
       // throw cached errors
-      if (got instanceof CachedError) {
+      if (cachedStageResult instanceof CachedError) {
         throw got;
       }
-      return;
-    } 
-    
+      if (cachedStageResult === true) {
+        // already ran this stage for this key and is still in the debounce period
+        return;
+      }
+    }
+
     try {
       hookCallback.call(this.innerHook);
-      this.cache.set(cacheKey, true);
+      this.cache.set(cacheKey, { ...got, [stage]: true });
     } catch (error: unknown) {
       if (this.cacheErrors) {
         // cache error
-        this.cache.set(cacheKey, new CachedError(error));
+        this.cache.set(cacheKey, { ...got, [stage]: new CachedError(error) });
       }
       throw error;
-    }    
+    }
   }
 }

libs/hooks/debounce/src/lib/utils/fixed-size-expiring-cache.ts

@@ -45,7 +45,7 @@ export class FixedSizeExpiringCache<T> {
    * @param key key for the entry
    * @returns value or key or undefined
    */
-  get(key: string): T | void {
+  get(key: string): T | undefined {
     if (key) {
       const entry = this.cacheMap.get(key);
       if (entry) {
@@ -56,6 +56,7 @@ export class FixedSizeExpiringCache<T> {
         }
       }
     }
+    return undefined;
   }
 
   /**