diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts index d86c2cc0f..77e3889db 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 implements ManageContext> { @@ -81,16 +86,23 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI 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((acc, [, provider]) => { - acc.push(provider); + .reduce((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), + ), ); } } @@ -196,19 +208,25 @@ export class OpenFeatureAPI extends OpenFeatureCommonAPI impleme oldContext: EvaluationContext, newContext: EvaluationContext, ): Promise { - 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/client/test/events.spec.ts b/packages/client/test/events.spec.ts index 6b7ff0bdd..02a47a577 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; 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, []> | undefined; - async onContextChange(): Promise { - return new Promise((resolve, reject) => setTimeout(() => { - if (this.failOnContextChange) { - reject(new Error(ERR_MESSAGE)); - } else { - resolve(); - } - }, this.asyncDelay)); - } - resolveBooleanEvaluation(): ResolutionDetails { throw new Error('Not implemented'); } @@ -87,6 +82,18 @@ class MockProvider implements Provider { resolveStringEvaluation(): ResolutionDetails { throw new Error('Not implemented'); } + + private changeHandler() { + return new Promise((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); + }); + }); + }); }); }); diff --git a/packages/shared/src/open-feature.ts b/packages/shared/src/open-feature.ts index 457154340..e57ea5601 100644 --- a/packages/shared/src/open-feature.ts +++ b/packages/shared/src/open-feature.ts @@ -108,6 +108,13 @@ export abstract class OpenFeatureCommonAPI