fixup: pr feedback

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

Author: toddbaert

libs/hooks/debounce/README.md

@@ -20,7 +20,7 @@ In the example below, we wrap a logging hook.
 This debounces all its stages, so it only logs a maximum of once a minute for each flag key, no matter how many times that flag is evaluated.
 
 ```ts
-const debounceHook = new DebounceHook<string>(loggingHook, {
+const debounceHook = new DebounceHook(loggingHook, {
   debounceTime: 60_000,             // how long to wait before the hook can fire again
   maxCacheItems: 100,               // max amount of items to keep in the cache; if exceeded, the oldest item is dropped
 });
@@ -34,13 +34,16 @@ client.addHooks(debounceHook);
 
 The hook maintains a simple expiring cache with a fixed max size and keeps a record of recent evaluations based on an optional key-generation function (cacheKeySupplier).
 Be default, the key-generation function is purely based on the flag key.
-Particularly in server use-cases, you may want to take the targetingKey or other contextual information into account in your debouncing:
+Particularly in server use-cases, you may want to take the targetingKey or other contextual information into account in your debouncing.
+Below we see an example using this, as well as other advanced options:
 
 ```ts
 const debounceHook = new DebounceHook<string>(loggingHook, {
   cacheKeySupplier: (flagKey, context) => flagKey + context.targetingKey, // cache on a combination of user and flag key
-  debounceTime: 60_000,            
-  maxCacheItems: 1000,              
+  debounceTime: 60_000, // debounce for 60 seconds (per user due to our cacheKeySupplier above)
+  maxCacheItems: 1000,  // cache a maximum of 1000 records
+  cacheErrors: false,   // don't debounce errors; always re-run if the last run threw 
+  logger: console,      // optional logger
 });
 ```
 

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

@@ -18,7 +18,7 @@ describe('DebounceHook', () => {
       finally: jest.fn(),
     };
 
-    const hook = new DebounceHook<string>(innerHook, {
+    const hook = new DebounceHook(innerHook, {
       debounceTime: 60_000,
       maxCacheItems: 100,
     });
@@ -76,7 +76,7 @@ describe('DebounceHook', () => {
         after: jest.fn(),
       };
 
-      const hook = new DebounceHook<boolean>(innerHook, {
+      const hook = new DebounceHook(innerHook, {
         debounceTime: 60_000,
         maxCacheItems: 100,
       });
@@ -106,7 +106,7 @@ describe('DebounceHook', () => {
         before: jest.fn(),
       };
 
-      const hook = new DebounceHook<string>(innerHook, {
+      const hook = new DebounceHook(innerHook, {
         debounceTime: 60_000,
         maxCacheItems: 1,
       });
@@ -156,7 +156,7 @@ describe('DebounceHook', () => {
       };
       const hints = {};
 
-      const hook = new DebounceHook<number>(innerHook, {
+      const hook = new DebounceHook(innerHook, {
         cacheKeySupplier: (_, context) => context.targetingKey, // we are caching purely based on the targetingKey in the context, so we will only ever cache one entry
         debounceTime: 60_000,
         maxCacheItems: 100,
@@ -190,7 +190,7 @@ describe('DebounceHook', () => {
       const context = {};
 
       // this hook caches error invocations
-      const hook = new DebounceHook<string[]>(innerErrorHook, {
+      const hook = new DebounceHook(innerErrorHook, {
         maxCacheItems: 100,
         debounceTime: 60_000,
         cacheErrors,
@@ -208,7 +208,7 @@ describe('DebounceHook', () => {
       it('should have type compatibility with API, client, evaluation', () => {
         const webSdkHook = {} as WebSdkHook;
 
-        const hook = new DebounceHook<number>(webSdkHook, {
+        const hook = new DebounceHook(webSdkHook, {
           debounceTime: 60_000,
           maxCacheItems: 100,
         });
@@ -230,7 +230,7 @@ describe('DebounceHook', () => {
           finally: jest.fn(),
         };
 
-        const hook = new DebounceHook<string>(innerWebSdkHook, {
+        const hook = new DebounceHook(innerWebSdkHook, {
           debounceTime: 60_000,
           maxCacheItems: 100,
         });
@@ -261,7 +261,7 @@ describe('DebounceHook', () => {
       it('should have type compatibility with API, client, evaluation', () => {
         const serverHook = {} as ServerSdkHook;
 
-        const hook = new DebounceHook<number>(serverHook, {
+        const hook = new DebounceHook(serverHook, {
           debounceTime: 60_000,
           maxCacheItems: 100,
         });
@@ -285,7 +285,7 @@ describe('DebounceHook', () => {
           finally: jest.fn(),
         };
 
-        const hook = new DebounceHook<number>(innerServerSdkHook, {
+        const hook = new DebounceHook(innerServerSdkHook, {
           debounceTime: 60_000,
           maxCacheItems: 100,
         });
@@ -331,9 +331,11 @@ describe('DebounceHook', () => {
           }),
         };
 
-        const hook = new DebounceHook<number>(innerServerSdkHook, {
+        const hook = new DebounceHook(innerServerSdkHook, {
           debounceTime: 60_000,
           maxCacheItems: 100,
+          cacheErrors: false,
+          logger: console,
         });
 
         const evaluationDetails: EvaluationDetails<number> = {

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

@@ -81,15 +81,7 @@ export type Options = {
  * 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
-    BaseHook<
-      T,
-      Record<string, unknown>,
-      Promise<EvaluationContext | void> | EvaluationContext | void,
-      Promise<void> | void
-    >
-{
+export class DebounceHook implements BaseHook {
   private readonly cache: FixedSizeExpiringCache<HookStagesEntry>;
   private readonly cacheErrors: boolean;
   private readonly cacheKeySupplier: Options['cacheKeySupplier'];
@@ -112,31 +104,31 @@ export class DebounceHook<T extends FlagValue = FlagValue>
     });
   }
 
-  before(hookContext: HookContext<T>, hookHints?: HookHints) {
+  before(hookContext: HookContext, hookHints?: HookHints) {
     return this.maybeSkipAndCache(
       'before',
       () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.before?.(hookContext, hookHints),
     );
   }
 
-  after(hookContext: HookContext<T>, evaluationDetails: EvaluationDetails<T>, hookHints?: HookHints) {
+  after(hookContext: HookContext, evaluationDetails: EvaluationDetails<FlagValue>, hookHints?: HookHints) {
     return this.maybeSkipAndCache(
       'after',
       () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.after?.(hookContext, evaluationDetails, hookHints),
     );
   }
 
-  error(hookContext: HookContext<T>, err: unknown, hookHints?: HookHints) {
+  error(hookContext: HookContext, err: unknown, hookHints?: HookHints) {
     return this.maybeSkipAndCache(
       'error',
       () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
       () => this.innerHook?.error?.(hookContext, err, hookHints),
     );
   }
 
-  finally(hookContext: HookContext<T>, evaluationDetails: EvaluationDetails<T>, hookHints?: HookHints) {
+  finally(hookContext: HookContext, evaluationDetails: EvaluationDetails<FlagValue>, hookHints?: HookHints) {
     return this.maybeSkipAndCache(
       'finally',
       () => this.cacheKeySupplier?.(hookContext.flagKey, hookContext.context),
@@ -150,10 +142,10 @@ export class DebounceHook<T extends FlagValue = FlagValue>
     hookCallback: () => T,
   ): T | void {
     // the cache key is a concatenation of the result of calling keyGenCallback and the stage
-    let dynamicKey: string | null | undefined;
+    let cacheKey: string | null | undefined;
 
     try {
-      dynamicKey = keyGenCallback();
+      cacheKey = keyGenCallback();
     } catch (e) {
       // if the keyGenCallback throws, we log and run the hook stage
       this.options.logger?.error(
@@ -163,11 +155,10 @@ export class DebounceHook<T extends FlagValue = FlagValue>
     }
 
     // if the keyGenCallback returns nothing, we don't do any caching
-    if (!dynamicKey) {
+    if (!cacheKey) {
       return hookCallback.call(this.innerHook);
     }
 
-    const cacheKey = `${dynamicKey}::cache-key`;
     const got = this.cache.get(cacheKey);
 
     if (got) {
@@ -222,6 +213,7 @@ export class DebounceHook<T extends FlagValue = FlagValue>
     cached: HookStagesEntry | undefined,
     maybeContext: EvaluationContext | void,
   ): void {
+    // cache the context if we have one, otherwise just a true to indicate we ran this stage
     this.cache.set(key, { ...cached, [stage]: maybeContext || true });
   }