fix: make hooks in client sdk only return void (#671)

<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR

Fixes return type of hooks in client SDK. 

Maybe we should make this more clear in the spec @toddbaert.

### Related Issues
Fixes #630 

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->

---------

Signed-off-by: Lukas Reining <[email protected]>
Co-authored-by: Todd Baert <[email protected]>

Author: lukas-reining

packages/client/src/client/open-feature-client.ts

@@ -6,7 +6,6 @@ import {
   EventHandler,
   FlagValue,
   FlagValueType,
-  Hook,
   HookContext,
   JsonValue,
   Logger,
@@ -22,6 +21,7 @@ import { OpenFeature } from '../open-feature';
 import { Provider } from '../provider';
 import { InternalEventEmitter } from '../events/internal/internal-event-emitter';
 import { Client } from './client';
+import { Hook } from '../hooks';
 
 type OpenFeatureClientOptions = {
   name?: string;
@@ -38,7 +38,7 @@ export class OpenFeatureClient implements Client {
     private readonly providerAccessor: () => Provider,
     private readonly emitterAccessor: () => InternalEventEmitter,
     private readonly globalLogger: () => Logger,
-    private readonly options: OpenFeatureClientOptions
+    private readonly options: OpenFeatureClientOptions,
   ) {}
 
   get metadata(): ClientMetadata {
@@ -76,12 +76,12 @@ export class OpenFeatureClient implements Client {
     return this;
   }
 
-  addHooks(...hooks: Hook<FlagValue>[]): this {
+  addHooks(...hooks: Hook[]): this {
     this._hooks = [...this._hooks, ...hooks];
     return this;
   }
 
-  getHooks(): Hook<FlagValue>[] {
+  getHooks(): Hook[] {
     return this._hooks;
   }
 
@@ -97,7 +97,7 @@ export class OpenFeatureClient implements Client {
   getBooleanDetails(
     flagKey: string,
     defaultValue: boolean,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<boolean> {
     return this.evaluate<boolean>(flagKey, this._provider.resolveBooleanEvaluation, defaultValue, 'boolean', options);
   }
@@ -109,15 +109,15 @@ export class OpenFeatureClient implements Client {
   getStringDetails<T extends string = string>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<T> {
     return this.evaluate<T>(
       flagKey,
       // this isolates providers from our restricted string generic argument.
       this._provider.resolveStringEvaluation as () => EvaluationDetails<T>,
       defaultValue,
       'string',
-      options
+      options,
     );
   }
 
@@ -128,30 +128,30 @@ export class OpenFeatureClient implements Client {
   getNumberDetails<T extends number = number>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<T> {
     return this.evaluate<T>(
       flagKey,
       // this isolates providers from our restricted number generic argument.
       this._provider.resolveNumberEvaluation as () => EvaluationDetails<T>,
       defaultValue,
       'number',
-      options
+      options,
     );
   }
 
   getObjectValue<T extends JsonValue = JsonValue>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): T {
     return this.getObjectDetails(flagKey, defaultValue, options).value;
   }
 
   getObjectDetails<T extends JsonValue = JsonValue>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<T> {
     return this.evaluate<T>(flagKey, this._provider.resolveObjectEvaluation, defaultValue, 'object', options);
   }
@@ -161,7 +161,7 @@ export class OpenFeatureClient implements Client {
     resolver: (flagKey: string, defaultValue: T, context: EvaluationContext, logger: Logger) => ResolutionDetails<T>,
     defaultValue: T,
     flagType: FlagValueType,
-    options: FlagEvaluationOptions = {}
+    options: FlagEvaluationOptions = {},
   ): EvaluationDetails<T> {
     // merge global, client, and evaluation context
 
@@ -224,26 +224,19 @@ export class OpenFeatureClient implements Client {
   }
 
   private beforeHooks(hooks: Hook[], hookContext: HookContext, options: FlagEvaluationOptions) {
+    Object.freeze(hookContext);
+    Object.freeze(hookContext.context);
+
     for (const hook of hooks) {
-      // freeze the hookContext
-      Object.freeze(hookContext);
-
-      // use Object.assign to avoid modification of frozen hookContext
-      Object.assign(hookContext.context, {
-        ...hookContext.context,
-        ...hook?.before?.(hookContext, Object.freeze(options.hookHints)),
-      });
+      hook?.before?.(hookContext, Object.freeze(options.hookHints));
     }
-
-    // after before hooks, freeze the EvaluationContext.
-    return Object.freeze(hookContext.context);
   }
 
   private afterHooks(
     hooks: Hook[],
     hookContext: HookContext,
     evaluationDetails: EvaluationDetails<FlagValue>,
-    options: FlagEvaluationOptions
+    options: FlagEvaluationOptions,
   ) {
     // run "after" hooks sequentially
     for (const hook of hooks) {

packages/client/src/evaluation/evaluation.ts

@@ -1,7 +1,7 @@
-import { EvaluationDetails, Hook, HookHints, JsonValue } from '@openfeature/core';
+import { EvaluationDetails, BaseHook, HookHints, JsonValue } from '@openfeature/core';
 
 export interface FlagEvaluationOptions {
-  hooks?: Hook[];
+  hooks?: BaseHook[];
   hookHints?: HookHints;
 }
 
@@ -25,7 +25,7 @@ export interface Features {
   getBooleanDetails(
     flagKey: string,
     defaultValue: boolean,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<boolean>;
 
   /**
@@ -53,7 +53,7 @@ export interface Features {
   getStringDetails<T extends string = string>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<T>;
 
   /**
@@ -81,7 +81,7 @@ export interface Features {
   getNumberDetails<T extends number = number>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<T>;
 
   /**
@@ -107,12 +107,12 @@ export interface Features {
   getObjectDetails(
     flagKey: string,
     defaultValue: JsonValue,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<JsonValue>;
 
   getObjectDetails<T extends JsonValue = JsonValue>(
     flagKey: string,
     defaultValue: T,
-    options?: FlagEvaluationOptions
+    options?: FlagEvaluationOptions,
   ): EvaluationDetails<T>;
 }

packages/client/src/hooks/hook.ts

@@ -0,0 +1,3 @@
+import { BaseHook, FlagValue } from '@openfeature/core';
+
+export type Hook = BaseHook<FlagValue, void, void>;

packages/client/src/hooks/index.ts

@@ -0,0 +1 @@
+export * from './hook';

packages/client/src/index.ts

@@ -3,4 +3,5 @@ export * from './provider';
 export * from './evaluation';
 export * from './open-feature';
 export * from './events';
+export * from './hooks';
 export * from '@openfeature/core';

packages/client/src/open-feature.ts

@@ -2,6 +2,7 @@ import { EvaluationContext, ManageContext, OpenFeatureCommonAPI } from '@openfea
 import { Client, OpenFeatureClient } from './client';
 import { NOOP_PROVIDER, Provider } from './provider';
 import { OpenFeatureEventEmitter } from './events';
+import { Hook } from './hooks';
 
 // use a symbol as a key for the global singleton
 const GLOBAL_OPENFEATURE_API_KEY = Symbol.for('@openfeature/web-sdk/api');
@@ -11,7 +12,7 @@ type OpenFeatureGlobal = {
 };
 const _globalThis = globalThis as OpenFeatureGlobal;
 
-export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> implements ManageContext<Promise<void>> {
+export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> implements ManageContext<Promise<void>> {
   protected _events = new OpenFeatureEventEmitter();
   protected _defaultProvider: Provider = NOOP_PROVIDER;
   protected _createEventEmitter = () => new OpenFeatureEventEmitter();
@@ -49,7 +50,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> implements Ma
         } catch (err) {
           this._logger?.error(`Error running context change handler of provider ${provider.metadata.name}:`, err);
         }
-      })
+      }),
     );
   }
 
@@ -75,7 +76,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider> implements Ma
       () => this.getProviderForClient(name),
       () => this.buildAndCacheEventEmitterForClient(name),
       () => this._logger,
-      { name, version }
+      { name, version },
     );
   }
 

packages/client/src/provider/provider.ts

@@ -1,4 +1,5 @@
-import { CommonProvider, EvaluationContext, Hook, JsonValue, Logger, ResolutionDetails } from '@openfeature/core';
+import { CommonProvider, EvaluationContext, JsonValue, Logger, ResolutionDetails } from '@openfeature/core';
+import { Hook } from '../hooks';
 
 /**
  * Interface that providers must implement to resolve flag values for their particular
@@ -30,7 +31,7 @@ export interface Provider extends CommonProvider {
     flagKey: string,
     defaultValue: boolean,
     context: EvaluationContext,
-    logger: Logger
+    logger: Logger,
   ): ResolutionDetails<boolean>;
 
   /**
@@ -40,7 +41,7 @@ export interface Provider extends CommonProvider {
     flagKey: string,
     defaultValue: string,
     context: EvaluationContext,
-    logger: Logger
+    logger: Logger,
   ): ResolutionDetails<string>;
 
   /**
@@ -50,7 +51,7 @@ export interface Provider extends CommonProvider {
     flagKey: string,
     defaultValue: number,
     context: EvaluationContext,
-    logger: Logger
+    logger: Logger,
   ): ResolutionDetails<number>;
 
   /**
@@ -60,6 +61,6 @@ export interface Provider extends CommonProvider {
     flagKey: string,
     defaultValue: T,
     context: EvaluationContext,
-    logger: Logger
+    logger: Logger,
   ): ResolutionDetails<T>;
 }

packages/client/test/hooks.spec.ts

@@ -4,9 +4,9 @@ import {
   Client,
   FlagValueType,
   EvaluationContext,
-  Hook,
   GeneralError,
   OpenFeature,
+  Hook,
 } from '../src';
 
 const BOOLEAN_VALUE = true;
@@ -107,27 +107,6 @@ describe('Hooks', () => {
   });
 
   describe('Requirement 4.1.4', () => {
-    describe('before', () => {
-      it('evaluationContext must be mutable', (done) => {
-        client.getBooleanValue(FLAG_KEY, false, {
-          hooks: [
-            {
-              before: (hookContext) => {
-                try {
-                  // evaluation context is mutable in before, so this should work.
-                  hookContext.context.newBeforeProp = 'new!';
-                  expect(hookContext.context.newBeforeProp).toBeTruthy();
-                  done();
-                } catch (err) {
-                  done(err);
-                }
-              },
-            },
-          ],
-        });
-      });
-    });
-
     describe('after', () => {
       it('evaluationContext must be immutable', (done) => {
         client.getBooleanValue(FLAG_KEY, false, {
@@ -151,58 +130,6 @@ describe('Hooks', () => {
     });
   });
 
-  describe('4.3.2', () => {
-    it('"before" must run before flag resolution', async () => {
-      await client.getBooleanValue(FLAG_KEY, false, {
-        hooks: [
-          {
-            before: () => {
-              // add a prop to the context.
-              return { beforeRan: true };
-            },
-          },
-        ],
-      });
-
-      expect(MOCK_PROVIDER.resolveBooleanEvaluation).toHaveBeenCalledWith(
-        expect.anything(),
-        expect.anything(),
-        // ensure property was added by the time the flag resolution occurred.
-        expect.objectContaining({
-          beforeRan: true,
-        }),
-        expect.anything()
-      );
-    });
-  });
-
-  describe('Requirement 4.3.3', () => {
-    it('EvaluationContext must be passed to next "before" hook', (done) => {
-      client.getBooleanValue(FLAG_KEY, false, {
-        hooks: [
-          {
-            before: () => {
-              // add a prop to the context.
-              return { beforeRan: true };
-            },
-          },
-          {
-            before: (hookContext) => {
-              // ensure added prop exists in next hook
-              try {
-                expect(hookContext.context.beforeRan).toBeTruthy();
-                done();
-              } catch (err) {
-                done(err);
-              }
-              return { beforeRan: true };
-            },
-          },
-        ],
-      });
-    });
-  });
-
   describe('Requirement 4.3.5', () => {
     it('"after" must run after flag evaluation', (done) => {
       client.getBooleanValue(FLAG_KEY, false, {