Repository: open-feature/js-sdk
Pull request #472 (2023-06-29 21:09:27)

fix: named client events
* fixed issues where named client wouldnt get events from a default provider

toddbaert: @lukas-reining I think this is a bug. I found it doing some integration testing. The new test failed without my change. I think what's happened is the transfers of handlers fails when the client name is null, but we still need to transfer the "unbound" emitters when the default provider is changed.

Please take a look and make sure the solution makes sense to you.
toddbaert: @lukas-reining that makes sense, and I very much like your refactors!

Patch:
diff --git a/packages/shared/src/filter.ts b/packages/shared/src/filter.ts
new file mode 100644
index 00000000..66b8a995
--- /dev/null
+++ b/packages/shared/src/filter.ts
@@ -0,0 +1,9 @@
+/**
+ * Checks if a value is not null or undefined and returns it as type assertion
+ * @template T
+ * @param {T} input The value to check
+ * @returns If the value is not null or undefined
+ */
+export function isDefined<T>(input?: T | null | undefined): input is T {
+  return typeof input !== 'undefined' && input !== null;
+}
diff --git a/packages/shared/src/open-feature.ts b/packages/shared/src/open-feature.ts
index a414423c..7bc31dce 100644
--- a/packages/shared/src/open-feature.ts
+++ b/packages/shared/src/open-feature.ts
@@ -10,6 +10,7 @@ import {
 } from './types';
 import { EventDetails, EventHandler, Eventing, OpenFeatureEventEmitter, ProviderEvents } from './events';
 import { objectOrUndefined, stringOrUndefined } from './type-guards';
+import { isDefined } from './filter';
 
 export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProvider> implements Eventing {
   protected _transactionContextPropagator: TransactionContextPropagator = NOOP_TRANSACTION_CONTEXT_PROPAGATOR;
@@ -104,21 +105,28 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
       return this;
     }
 
-    const clientEmitter = this.getAndCacheEventEmitterForClient(clientName);
+    // get the named emitter, or if this is the default provider, get all event emitters not associated with a provider
+    const emitters = clientName ? [this.getAndCacheEventEmitterForClient(clientName)] : this.getUnboundEmitters();
 
     if (typeof provider.initialize === 'function') {
       provider
         .initialize?.(this._context)
         ?.then(() => {
-          clientEmitter.emit(ProviderEvents.Ready, { clientName });
+          emitters.forEach((emitter) => {
+            emitter?.emit(ProviderEvents.Ready, { clientName });
+          });
           this._events?.emit(ProviderEvents.Ready, { clientName });
         })
         ?.catch((error) => {
-          clientEmitter.emit(ProviderEvents.Error, { clientName, message: error.message });
+          emitters.forEach((emitter) => {
+            emitter?.emit(ProviderEvents.Error, { clientName, message: error.message });
+          });
           this._events?.emit(ProviderEvents.Error, { clientName, message: error.message });
         });
     } else {
-      clientEmitter.emit(ProviderEvents.Ready, { clientName });
+      emitters.forEach((emitter) => {
+        emitter?.emit(ProviderEvents.Ready, { clientName });
+      });
       this._events?.emit(ProviderEvents.Ready, { clientName });
     }
 
@@ -128,7 +136,7 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
       this._defaultProvider = provider;
     }
 
-    this.transferListeners(oldProvider, provider, clientName, clientEmitter);
+    this.transferListeners(oldProvider, provider, clientName, emitters);
 
     // Do not close a provider that is bound to any client
     if (![...this._clientProviders.values(), this._defaultProvider].includes(oldProvider)) {
@@ -167,11 +175,18 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
     return newEmitter;
   }
 
+  private getUnboundEmitters(): OpenFeatureEventEmitter[] {
+    const namedProviders = [...this._clientProviders.keys()];
+    const eventEmitterNames = [...this._clientEvents.keys()].filter(isDefined);
+    const unboundEmitterNames = eventEmitterNames.filter((name) => !namedProviders.includes(name));
+    return unboundEmitterNames.map((name) => this._clientEvents.get(name)).filter(isDefined);
+  }
+
   private transferListeners(
     oldProvider: P,
     newProvider: P,
     clientName: string | undefined,
-    clientEmitter: OpenFeatureEventEmitter
+    emitters: (OpenFeatureEventEmitter | undefined)[]
   ) {
     this._clientEventHandlers
       .get(clientName)
@@ -182,7 +197,9 @@ export abstract class OpenFeatureCommonAPI<P extends CommonProvider = CommonProv
       (eventType) => {
         const handler = async (details?: EventDetails) => {
           // on each event type, fire the associated handlers
-          clientEmitter.emit(eventType, { ...details, clientName });
+          emitters.forEach((emitter) => {
+            emitter?.emit(eventType, { ...details, clientName });
+          });
           this._events.emit(eventType, { ...details, clientName });
         };
 


Test patch:
diff --git a/packages/client/test/events.spec.ts b/packages/client/test/events.spec.ts
index 364febe1..c737c985 100644
--- a/packages/client/test/events.spec.ts
+++ b/packages/client/test/events.spec.ts
@@ -1,6 +1,7 @@
 import {
   EventDetails,
   JsonValue,
+  NOOP_PROVIDER,
   OpenFeature,
   OpenFeatureEventEmitter,
   Provider,
@@ -76,6 +77,10 @@ describe('Events', () => {
     clientId = uuid();
   });
 
+  beforeEach(() => {
+    OpenFeature.setProvider(NOOP_PROVIDER);
+  });
+
   describe('Requirement 5.1.1', () => {
     describe('provider implements events', () => {
       it('The provider defines a mechanism for signalling the occurrence of an event`PROVIDER_READY`', (done) => {
@@ -192,6 +197,45 @@ describe('Events', () => {
       OpenFeature.setProvider(clientId, provider);
     });
 
+    it('anonymous provider with named client should run', (done) => {
+      const defaultProvider = new MockProvider({
+        failOnInit: false,
+        initialStatus: ProviderStatus.NOT_READY,
+        name: 'default',
+      });
+      const unboundName = 'some-new-unbound-name';
+
+      // get a client using the default because it has not other mapping
+      const unBoundClient = OpenFeature.getClient(unboundName);
+      unBoundClient.addHandler(ProviderEvents.ConfigurationChanged, () => {
+        done();
+      });
+
+      // set the default provider
+      OpenFeature.setProvider(defaultProvider);
+
+      // fire events
+      defaultProvider.events?.emit(ProviderEvents.ConfigurationChanged);
+    });
+
+    it('anonymous provider with named client should run init events', (done) => {
+      const defaultProvider = new MockProvider({
+        failOnInit: false,
+        initialStatus: ProviderStatus.NOT_READY,
+        name: 'default',
+      });
+      const unboundName = 'some-other-unbound-name';
+
+      // get a client using the default because it has not other mapping
+      const unBoundClient = OpenFeature.getClient(unboundName);
+      unBoundClient.addHandler(ProviderEvents.Ready, () => {
+        done();
+      });
+
+      // set the default provider
+      OpenFeature.setProvider(defaultProvider);
+    });
+
     it('un-bound client event handlers still run after new provider set', (done) => {
       const defaultProvider = new MockProvider({ name: 'default' });
       const namedProvider = new MockProvider();
diff --git a/packages/server/test/events.spec.ts b/packages/server/test/events.spec.ts
index 691d6aa4..90a5c7b7 100644
--- a/packages/server/test/events.spec.ts
+++ b/packages/server/test/events.spec.ts
@@ -10,6 +10,7 @@ import {
   ResolutionDetails,
 } from '../src';
 import { v4 as uuid } from 'uuid';
+import { NOOP_PROVIDER } from '../src';
 
 class MockProvider implements Provider {
   readonly metadata: ProviderMetadata;
@@ -79,6 +80,10 @@ describe('Events', () => {
     clientId = uuid();
   });
 
+  beforeEach(() => {
+    OpenFeature.setProvider(NOOP_PROVIDER);
+  });
+
   describe('Requirement 5.1.1', () => {
     describe('provider implements events', () => {
       it('The provider defines a mechanism for signalling the occurrence of an event`PROVIDER_READY`', (done) => {
@@ -195,6 +200,45 @@ describe('Events', () => {
       OpenFeature.setProvider(clientId, provider);
     });
 
+    it('anonymous provider with named client should run', (done) => {
+      const defaultProvider = new MockProvider({
+        failOnInit: false,
+        initialStatus: ProviderStatus.NOT_READY,
+        name: 'default',
+      });
+      const unboundName = 'some-new-unbound-name';
+
+      // get a client using the default because it has not other mapping
+      const unBoundClient = OpenFeature.getClient(unboundName);
+      unBoundClient.addHandler(ProviderEvents.ConfigurationChanged, () => {
+        done();
+      });
+
+      // set the default provider
+      OpenFeature.setProvider(defaultProvider);
+
+      // fire events
+      defaultProvider.events?.emit(ProviderEvents.ConfigurationChanged);
+    });
+
+    it('anonymous provider with named client should run init events', (done) => {
+      const defaultProvider = new MockProvider({
+        failOnInit: false,
+        initialStatus: ProviderStatus.NOT_READY,
+        name: 'default',
+      });
+      const unboundName = 'some-other-unbound-name';
+
+      // get a client using the default because it has not other mapping
+      const unBoundClient = OpenFeature.getClient(unboundName);
+      unBoundClient.addHandler(ProviderEvents.Ready, () => {
+        done();
+      });
+
+      // set the default provider
+      OpenFeature.setProvider(defaultProvider);
+    });
+
     it('un-bound client event handlers still run after new provider set', (done) => {
       const defaultProvider = new MockProvider({ name: 'default' });
       const namedProvider = new MockProvider();