Repository: open-feature/js-sdk
Pull request #753 (2024-01-11 02:16:48)

fix: some handlers fail to run
fixed an issue where some handlers would fail to run if the associated provider did not have a onContextChange method


Patch:
diff --git a/packages/client/src/client/open-feature-client.ts b/packages/client/src/client/open-feature-client.ts
index 84fdc79c..dc86f0a5 100644
--- a/packages/client/src/client/open-feature-client.ts
+++ b/packages/client/src/client/open-feature-client.ts
@@ -179,7 +179,7 @@ export class OpenFeatureClient implements Client {
     const allHooksReversed = [...allHooks].reverse();
 
     const context = {
-      ...OpenFeature.getContext(),
+      ...OpenFeature.getContext(this?.options?.name),
     };
 
     // this reference cannot change during the course of evaluation
diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts
index 1aa50243..77e3889d 100644
--- a/packages/client/src/open-feature.ts
+++ b/packages/client/src/open-feature.ts
@@ -17,6 +17,11 @@ const GLOBAL_OPENFEATURE_API_KEY = Symbol.for('@openfeature/web-sdk/api');
 type OpenFeatureGlobal = {
   [GLOBAL_OPENFEATURE_API_KEY]?: OpenFeatureAPI;
 };
+type NameProviderRecord = {
+  name?: string;
+  provider: Provider;
+}
+
 const _globalThis = globalThis as OpenFeatureGlobal;
 
 export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> implements ManageContext<Promise<void>> {
@@ -81,16 +86,23 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
       const oldContext = this._context;
       this._context = context;
 
-      const providersWithoutContextOverride = Array.from(this._clientProviders.entries())
+      // collect all providers that are using the default context (not mapped to a name)
+      const defaultContextNameProviders: NameProviderRecord[] = Array.from(this._clientProviders.entries())
         .filter(([name]) => !this._namedProviderContext.has(name))
-        .reduce<Provider[]>((acc, [, provider]) => {
-          acc.push(provider);
+        .reduce<NameProviderRecord[]>((acc, [name, provider]) => {
+          acc.push({ name, provider });
           return acc;
         }, []);
 
-      const allProviders = [this._defaultProvider, ...providersWithoutContextOverride];
+      const allProviders: NameProviderRecord[] = [
+        // add in the default (no name)
+        { name: undefined, provider: this._defaultProvider },
+        ...defaultContextNameProviders,
+      ];
       await Promise.all(
-        allProviders.map((provider) => this.runProviderContextChangeHandler(undefined, provider, oldContext, context)),
+        allProviders.map((tuple) =>
+          this.runProviderContextChangeHandler(tuple.name, tuple.provider, oldContext, context),
+        ),
       );
     }
   }
@@ -106,7 +118,7 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
    * @param {string} clientName The name to identify the client
    * @returns {EvaluationContext} Evaluation context
    */
-  getContext(clientName: string): EvaluationContext;
+  getContext(clientName?: string): EvaluationContext;
   getContext(nameOrUndefined?: string): EvaluationContext {
     const clientName = stringOrUndefined(nameOrUndefined);
     if (clientName) {
@@ -196,19 +208,25 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI<Provider, Hook> impleme
     oldContext: EvaluationContext,
     newContext: EvaluationContext,
   ): Promise<void> {
-      const providerName = provider.metadata.name;
-      return provider.onContextChange?.(oldContext, newContext).then(() => {
-        this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
-          emitter?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
-        });
-        this._events?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
-      }).catch((err) => {
-        this._logger?.error(`Error running ${provider.metadata.name}'s context change handler:`, err);
-        this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
-          emitter?.emit(ProviderEvents.Error, { clientName, providerName, message: err?.message, });
-        });
-        this._events?.emit(ProviderEvents.Error, { clientName, providerName, message: err?.message, });
+    const providerName = provider.metadata.name;
+    try {
+      await provider.onContextChange?.(oldContext, newContext);
+
+      // only run the event handlers if the onContextChange method succeeded
+      this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
+        emitter?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
       });
+      this._events?.emit(ProviderEvents.ContextChanged, { clientName, providerName });
+    } catch (err) {
+      // run error handlers instead
+      const error = err as Error | undefined;
+      const message = `Error running ${provider?.metadata?.name}'s context change handler: ${error?.message}`;
+      this._logger?.error(`${message}`, err);
+      this.getAssociatedEventEmitters(clientName).forEach((emitter) => {
+        emitter?.emit(ProviderEvents.Error, { clientName, providerName, message });
+      });
+      this._events?.emit(ProviderEvents.Error, { clientName, providerName, message });
+    }
   }
 }
 
diff --git a/packages/shared/src/open-feature.ts b/packages/shared/src/open-feature.ts
index 45715434..e57ea560 100644
--- a/packages/shared/src/open-feature.ts
+++ b/packages/shared/src/open-feature.ts
@@ -108,6 +108,13 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
     this._events.removeHandler(eventType, handler);
   }
 
+  /**
+   * Removes all event handlers.
+   */
+  clearHandlers(): void {
+    this._events.removeAllHandlers();
+  }
+
   /**
    * Gets the current handlers for the given provider event type.
    * @param {AnyProviderEvent} eventType The provider event type to get the current handlers for


Test patch:
diff --git a/packages/client/test/evaluation-context.spec.ts b/packages/client/test/evaluation-context.spec.ts
index 823bd123..11892dda 100644
--- a/packages/client/test/evaluation-context.spec.ts
+++ b/packages/client/test/evaluation-context.spec.ts
@@ -12,9 +12,12 @@ class MockProvider implements Provider {
     return Promise.resolve();
   }
 
-  resolveBooleanEvaluation(): ResolutionDetails<boolean> {
-    throw new Error('Not implemented');
-  }
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  resolveBooleanEvaluation = jest.fn((flagKey: string, defaultValue: boolean, context: EvaluationContext) => {
+    return {
+      value: true
+    };
+  });
 
   resolveNumberEvaluation(): ResolutionDetails<number> {
     throw new Error('Not implemented');
@@ -76,6 +79,57 @@ describe('Evaluation Context', () => {
         expect(OpenFeature.getContext(clientName)).toEqual(globalContext);
       });
 
+      it('should call onContextChange for appropriate provider with appropriate context', async () => {
+        const globalContext: EvaluationContext = { scope: 'global' };
+        const testContext: EvaluationContext = { scope: 'test' };
+        const clientName = 'appropriateProviderTest';
+        const defaultProvider = new MockProvider();
+        const provider1 = new MockProvider();
+
+        await OpenFeature.setProviderAndWait(defaultProvider);
+        await OpenFeature.setProviderAndWait(clientName, provider1);
+
+        // Spy on context changed handlers of both providers
+        const defaultProviderSpy = jest.spyOn(defaultProvider, 'onContextChange');
+        const provider1Spy = jest.spyOn(provider1, 'onContextChange');
+
+        await OpenFeature.setContext(globalContext);
+        await OpenFeature.setContext(clientName, testContext);
+
+        // provider one should get global and specific context calls
+        expect(defaultProviderSpy).toHaveBeenCalledWith({}, globalContext);
+        expect(provider1Spy).toHaveBeenCalledWith(globalContext, testContext);
+      });
+
+      it('should pass correct context to resolver', async () => {
+        const globalContext: EvaluationContext = { scope: 'global' };
+        const testContext: EvaluationContext = { scope: 'test' };
+        const clientName = 'correctContextTest';
+        const defaultProvider = new MockProvider();
+        const provider1 = new MockProvider();
+
+        await OpenFeature.setProviderAndWait(defaultProvider);
+        await OpenFeature.setProviderAndWait(clientName, provider1);
+
+        // Spy on boolean resolvers of both providers
+        const defaultProviderSpy = jest.spyOn(defaultProvider, 'resolveBooleanEvaluation');
+        const provider1Spy = jest.spyOn(provider1, 'resolveBooleanEvaluation');
+
+        await OpenFeature.setContext(globalContext);
+        await OpenFeature.setContext(clientName, testContext);
+
+        const defaultClient = OpenFeature.getClient();
+        const provider1Client = OpenFeature.getClient(clientName);
+
+        const flagName = 'some-flag';
+        defaultClient.getBooleanValue(flagName, false);
+        provider1Client.getBooleanValue(flagName, false);
+
+        // provider one should get global and specific context
+        expect(defaultProviderSpy).toHaveBeenCalledWith(flagName, false, globalContext, expect.anything());
+        expect(provider1Spy).toHaveBeenCalledWith(flagName, false, testContext, expect.anything());
+      });
+
       it('should only call a providers onContextChange once when clearing context', async () => {
         const globalContext: EvaluationContext = { scope: 'global' };
         const testContext: EvaluationContext = { scope: 'test' };
diff --git a/packages/client/test/events.spec.ts b/packages/client/test/events.spec.ts
index 6b7ff0bd..02a47a57 100644
--- a/packages/client/test/events.spec.ts
+++ b/packages/client/test/events.spec.ts
@@ -26,6 +26,7 @@ class MockProvider implements Provider {
   private asyncDelay?: number;
   private enableEvents: boolean;
   status?: ProviderStatus = undefined;
+  onContextChange?: () => Promise<void>;
 
   constructor(options?: {
     hasInitialize?: boolean;
@@ -33,6 +34,7 @@ class MockProvider implements Provider {
     asyncDelay?: number;
     enableEvents?: boolean;
     failOnInit?: boolean;
+    noContextChanged?: boolean;
     failOnContextChange?: boolean;
     name?: string;
   }) {
@@ -43,6 +45,9 @@ class MockProvider implements Provider {
     this.enableEvents = options?.enableEvents ?? true;
     this.failOnInit = options?.failOnInit ?? false;
     this.failOnContextChange = options?.failOnContextChange ?? false;
+    if (!options?.noContextChanged) {
+      this.onContextChange = this.changeHandler;
+    }
 
     if (this.enableEvents) {
       this.events = new OpenFeatureEventEmitter();
@@ -62,16 +67,6 @@ class MockProvider implements Provider {
 
   initialize: jest.Mock<Promise<void>, []> | undefined;
 
-  async onContextChange(): Promise<void> {
-      return new Promise((resolve, reject) => setTimeout(() => {
-        if (this.failOnContextChange) {
-          reject(new Error(ERR_MESSAGE));
-        } else {
-          resolve();
-        }
-      }, this.asyncDelay));
-  }
-
   resolveBooleanEvaluation(): ResolutionDetails<boolean> {
     throw new Error('Not implemented');
   }
@@ -87,6 +82,18 @@ class MockProvider implements Provider {
   resolveStringEvaluation(): ResolutionDetails<string> {
     throw new Error('Not implemented');
   }
+
+  private changeHandler() {
+    return new Promise<void>((resolve, reject) =>
+      setTimeout(() => {
+        if (this.failOnContextChange) {
+          reject(new Error(ERR_MESSAGE));
+        } else {
+          resolve();
+        }
+      }, this.asyncDelay),
+    );
+  }
 }
 
 describe('Events', () => {
@@ -96,6 +103,7 @@ describe('Events', () => {
 
   afterEach(async () => {
     await OpenFeature.clearProviders();
+    OpenFeature.clearHandlers();
     jest.clearAllMocks();
     clientId = uuid();
     // hacky, but it's helpful to clear the handlers between tests
@@ -610,7 +618,6 @@ describe('Events', () => {
               expect(details?.clientName).toEqual(clientId);
               expect(details?.providerName).toEqual(provider.metadata.name);
               done();
-              OpenFeature.removeHandler(ProviderEvents.ContextChanged, handler);
             } catch (e) {
               done(e);
             }
@@ -630,7 +637,6 @@ describe('Events', () => {
               expect(details?.clientName).toEqual(clientId);
               expect(details?.providerName).toEqual(provider.metadata.name);
               done();
-              OpenFeature.removeHandler(ProviderEvents.Error, handler);
             } catch (e) {
               done(e);
             }
@@ -643,15 +649,25 @@ describe('Events', () => {
       describe('context set for different client', () => {
         it("If the provider's `on context changed` function terminates normally, associated `PROVIDER_CONTEXT_CHANGED` handlers MUST run.", (done) => {
           const provider = new MockProvider({ initialStatus: ProviderStatus.READY });
+          let runCount = 0;
 
           OpenFeature.setProvider(clientId, provider);
 
+          // expect 2 runs, since 2 providers are impacted by this context change (global)
           const handler = (details?: EventDetails) => {
             try {
-              expect(details?.clientName).toBeUndefined();
-              expect(details?.providerName).toEqual(provider.metadata.name);
-              OpenFeature.removeHandler(ProviderEvents.ContextChanged, handler);
-              done();
+              runCount++;
+              // one run should be global
+              if (details?.clientName === undefined) {
+                expect(details?.providerName).toEqual(OpenFeature.getProviderMetadata().name);
+              } else if (details?.clientName === clientId) {
+                // one run should be for client
+                expect(details?.clientName).toEqual(clientId);
+                expect(details?.providerName).toEqual(provider.metadata.name);
+              }
+              if (runCount == 2) {
+                done();
+              }
             } catch (e) {
               done(e);
             }
@@ -669,10 +685,10 @@ describe('Events', () => {
 
           const handler = (details?: EventDetails) => {
             try {
-              expect(details?.clientName).toBeUndefined();
+              // expect only one error run, because only one provider throws
+              expect(details?.clientName).toEqual(clientId);
               expect(details?.providerName).toEqual(provider.metadata.name);
-              expect(details?.message).toEqual(ERR_MESSAGE);
-              OpenFeature.removeHandler(ProviderEvents.Error, handler);
+              expect(details?.message).toBeTruthy();
               done();
             } catch (e) {
               done(e);
@@ -696,7 +712,6 @@ describe('Events', () => {
           try {
             expect(details?.clientName).toEqual(clientId);
             expect(details?.providerName).toEqual(provider.metadata.name);
-            OpenFeature.removeHandler(ProviderEvents.ContextChanged, handler);
             done();
           } catch (e) {
             done(e);
@@ -717,8 +732,7 @@ describe('Events', () => {
           try {
             expect(details?.clientName).toEqual(clientId);
             expect(details?.providerName).toEqual(provider.metadata.name);
-            expect(details?.message).toEqual(ERR_MESSAGE);
-            OpenFeature.removeHandler(ProviderEvents.Error, handler);
+            expect(details?.message).toBeTruthy();
             done();
           } catch (e) {
             done(e);
@@ -730,5 +744,29 @@ describe('Events', () => {
 
       });
     });
+
+    describe('provider', () => {
+      describe('has no onContextChange handler', () => {
+        it('runs API ContextChanged event handler', (done) => {
+          const noChangeHandlerProvider = 'noChangeHandlerProvider';
+          const provider = new MockProvider({ initialStatus: ProviderStatus.READY, noContextChanged: true });
+  
+          OpenFeature.setProvider(noChangeHandlerProvider, provider);
+          OpenFeature.setContext(noChangeHandlerProvider, {});
+  
+          const handler = (details?: EventDetails) => {
+            try {
+              expect(details?.clientName).toEqual(noChangeHandlerProvider);
+              expect(details?.providerName).toEqual(provider.metadata.name);
+              done();
+            } catch (e) {
+              done(e);
+            }
+          };
+  
+          OpenFeature.addHandler(ProviderEvents.ContextChanged, handler);
+        });
+      });
+    });
   });
 });