From 1c92ffec5d72192887643c55f0b6dba14d0c5025 Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Wed, 19 Feb 2025 16:33:34 +0000 Subject: [PATCH] feat: add support for abort controllers to event handlers Signed-off-by: Michael Beemer --- .../react/src/evaluation/use-feature-flag.ts | 18 ++++------ .../use-open-feature-client-status.ts | 20 +++++------ .../client/internal/open-feature-client.ts | 9 ++++- packages/server/test/events.spec.ts | 31 +++++++++++++++- packages/shared/src/events/eventing.ts | 7 ++++ packages/shared/src/open-feature.ts | 19 ++++++---- packages/shared/test/events.spec.ts | 36 ++++++------------- .../client/internal/open-feature-client.ts | 9 ++++- packages/web/test/events.spec.ts | 31 +++++++++++++++- 9 files changed, 120 insertions(+), 60 deletions(-) diff --git a/packages/react/src/evaluation/use-feature-flag.ts b/packages/react/src/evaluation/use-feature-flag.ts index 899c042cc..7d3bab251 100644 --- a/packages/react/src/evaluation/use-feature-flag.ts +++ b/packages/react/src/evaluation/use-feature-flag.ts @@ -280,6 +280,7 @@ function attachHandlersAndResolve( const defaultedOptions = { ...DEFAULT_OPTIONS, ...useProviderOptions(), ...normalizeOptions(options) }; const client = useOpenFeatureClient(); const status = useOpenFeatureClientStatus(); + const controller = new AbortController(); // suspense if (defaultedOptions.suspendUntilReady && status === ProviderStatus.NOT_READY) { @@ -322,28 +323,23 @@ function attachHandlersAndResolve( useEffect(() => { if (status === ProviderStatus.NOT_READY) { // update when the provider is ready - client.addHandler(ProviderEvents.Ready, updateEvaluationDetailsCallback); + client.addHandler(ProviderEvents.Ready, updateEvaluationDetailsCallback, { signal: controller.signal }); } if (defaultedOptions.updateOnContextChanged) { // update when the context changes - client.addHandler(ProviderEvents.ContextChanged, updateEvaluationDetailsCallback); + client.addHandler(ProviderEvents.ContextChanged, updateEvaluationDetailsCallback, { signal: controller.signal }); } - return () => { - // cleanup the handlers - client.removeHandler(ProviderEvents.Ready, updateEvaluationDetailsCallback); - client.removeHandler(ProviderEvents.ContextChanged, updateEvaluationDetailsCallback); - }; - }, []); - useEffect(() => { if (defaultedOptions.updateOnConfigurationChanged) { // update when the provider configuration changes - client.addHandler(ProviderEvents.ConfigurationChanged, configurationChangeCallback); + client.addHandler(ProviderEvents.ConfigurationChanged, configurationChangeCallback, { + signal: controller.signal, + }); } return () => { // cleanup the handlers - client.removeHandler(ProviderEvents.ConfigurationChanged, configurationChangeCallback); + controller.abort(); }; }, []); diff --git a/packages/react/src/provider/use-open-feature-client-status.ts b/packages/react/src/provider/use-open-feature-client-status.ts index fac4a42b7..544cf5b54 100644 --- a/packages/react/src/provider/use-open-feature-client-status.ts +++ b/packages/react/src/provider/use-open-feature-client-status.ts @@ -10,22 +10,18 @@ import { ProviderEvents } from '@openfeature/web-sdk'; export function useOpenFeatureClientStatus(): ProviderStatus { const client = useOpenFeatureClient(); const [status, setStatus] = useState(client.providerStatus); + const controller = new AbortController(); useEffect(() => { const updateStatus = () => setStatus(client.providerStatus); - client.addHandler(ProviderEvents.ConfigurationChanged, updateStatus); - client.addHandler(ProviderEvents.ContextChanged, updateStatus); - client.addHandler(ProviderEvents.Error, updateStatus); - client.addHandler(ProviderEvents.Ready, updateStatus); - client.addHandler(ProviderEvents.Stale, updateStatus); - client.addHandler(ProviderEvents.Reconciling, updateStatus); + client.addHandler(ProviderEvents.ConfigurationChanged, updateStatus, { signal: controller.signal }); + client.addHandler(ProviderEvents.ContextChanged, updateStatus, { signal: controller.signal }); + client.addHandler(ProviderEvents.Error, updateStatus, { signal: controller.signal }); + client.addHandler(ProviderEvents.Ready, updateStatus, { signal: controller.signal }); + client.addHandler(ProviderEvents.Stale, updateStatus, { signal: controller.signal }); + client.addHandler(ProviderEvents.Reconciling, updateStatus, { signal: controller.signal }); return () => { - client.removeHandler(ProviderEvents.ConfigurationChanged, updateStatus); - client.removeHandler(ProviderEvents.ContextChanged, updateStatus); - client.removeHandler(ProviderEvents.Error, updateStatus); - client.removeHandler(ProviderEvents.Ready, updateStatus); - client.removeHandler(ProviderEvents.Stale, updateStatus); - client.removeHandler(ProviderEvents.Reconciling, updateStatus); + controller.abort(); }; }, [client]); diff --git a/packages/server/src/client/internal/open-feature-client.ts b/packages/server/src/client/internal/open-feature-client.ts index 08a577845..4f440e940 100644 --- a/packages/server/src/client/internal/open-feature-client.ts +++ b/packages/server/src/client/internal/open-feature-client.ts @@ -12,6 +12,7 @@ import type { OpenFeatureError, FlagMetadata, ResolutionDetails, + EventOptions, } from '@openfeature/core'; import { ErrorCode, @@ -79,7 +80,7 @@ export class OpenFeatureClient implements Client { return this.providerStatusAccessor(); } - addHandler(eventType: ProviderEvents, handler: EventHandler): void { + addHandler(eventType: ProviderEvents, handler: EventHandler, options?: EventOptions): void { this.emitterAccessor().addHandler(eventType, handler); const shouldRunNow = statusMatchesEvent(eventType, this._providerStatus); @@ -95,6 +96,12 @@ export class OpenFeatureClient implements Client { this._logger?.error('Error running event handler:', err); } } + + if (options?.signal && typeof options.signal.addEventListener === 'function') { + options.signal.addEventListener('abort', () => { + this.removeHandler(eventType, handler); + }); + } } removeHandler(eventType: ProviderEvents, handler: EventHandler) { diff --git a/packages/server/test/events.spec.ts b/packages/server/test/events.spec.ts index 07b3babe3..010a0f33d 100644 --- a/packages/server/test/events.spec.ts +++ b/packages/server/test/events.spec.ts @@ -449,7 +449,21 @@ describe('Events', () => { expect(OpenFeature.getHandlers(eventType)).toHaveLength(0); }); - it('The API provides a function allowing the removal of event handlers', () => { + it('The event handler can be removed using an abort signal', () => { + const abortController = new AbortController(); + const handler1 = jest.fn(); + const handler2 = jest.fn(); + const eventType = ProviderEvents.Stale; + + OpenFeature.addHandler(eventType, handler1, { signal: abortController.signal }); + OpenFeature.addHandler(eventType, handler2); + expect(OpenFeature.getHandlers(eventType)).toHaveLength(2); + + abortController.abort(); + expect(OpenFeature.getHandlers(eventType)).toHaveLength(1); + }); + + it('The API provides a function allowing the removal of event handlers from client', () => { const client = OpenFeature.getClient(domain); const handler = jest.fn(); const eventType = ProviderEvents.Stale; @@ -459,6 +473,21 @@ describe('Events', () => { client.removeHandler(eventType, handler); expect(client.getHandlers(eventType)).toHaveLength(0); }); + + it('The event handler on the client can be removed using an abort signal', () => { + const abortController = new AbortController(); + const client = OpenFeature.getClient(domain); + const handler1 = jest.fn(); + const handler2 = jest.fn(); + const eventType = ProviderEvents.Stale; + + client.addHandler(eventType, handler1, { signal: abortController.signal }); + client.addHandler(eventType, handler2); + expect(client.getHandlers(eventType)).toHaveLength(2); + + abortController.abort(); + expect(client.getHandlers(eventType)).toHaveLength(1); + }); }); describe('Requirement 5.3.1', () => { diff --git a/packages/shared/src/events/eventing.ts b/packages/shared/src/events/eventing.ts index 976e12cf4..c3ae8b9db 100644 --- a/packages/shared/src/events/eventing.ts +++ b/packages/shared/src/events/eventing.ts @@ -66,6 +66,9 @@ export type EventDetails< export type EventHandler< T extends ServerProviderEvents | ClientProviderEvents = ServerProviderEvents | ClientProviderEvents, > = (eventDetails?: EventDetails) => Promise | unknown; +export type EventOptions = { + signal?: AbortSignal; +}; export interface Eventing { /** @@ -73,6 +76,7 @@ export interface Eventing * The handlers are called in the order they have been added. * @param eventType The provider event type to listen to * @param {EventHandler} handler The handler to run on occurrence of the event type + * @param {EventOptions} options Optional options such as signal for aborting */ addHandler( eventType: T extends ClientProviderEvents @@ -83,14 +87,17 @@ export interface Eventing ? ClientProviderEvents.ConfigurationChanged : ServerProviderEvents.ConfigurationChanged >, + options?: EventOptions, ): void; addHandler( eventType: T extends ClientProviderEvents ? ClientNotChangeEvents : ServerNotChangeEvents, handler: EventHandler, + options?: EventOptions, ): void; addHandler( eventType: T extends ClientProviderEvents ? ClientProviderEvents : ServerProviderEvents, handler: EventHandler, + options?: EventOptions, ): void; /** diff --git a/packages/shared/src/open-feature.ts b/packages/shared/src/open-feature.ts index 58e1e6a14..fc82bc908 100644 --- a/packages/shared/src/open-feature.ts +++ b/packages/shared/src/open-feature.ts @@ -7,14 +7,13 @@ import type { EventDetails, EventHandler, Eventing, - GenericEventEmitter} from './events'; -import { - AllProviderEvents, - statusMatchesEvent, + EventOptions, + GenericEventEmitter, } from './events'; +import { AllProviderEvents, statusMatchesEvent } from './events'; import { isDefined } from './filter'; import type { BaseHook, EvaluationLifeCycle } from './hooks'; -import type { Logger, ManageLogger} from './logger'; +import type { Logger, ManageLogger } from './logger'; import { DefaultLogger, SafeLogger } from './logger'; import type { ClientProviderStatus, CommonProvider, ProviderMetadata, ServerProviderStatus } from './provider'; import { objectOrUndefined, stringOrUndefined } from './type-guards'; @@ -154,8 +153,9 @@ export abstract class OpenFeatureCommonAPI< * API (global) events run for all providers. * @param {AnyProviderEvent} eventType The provider event type to listen to * @param {EventHandler} handler The handler to run on occurrence of the event type + * @param {EventOptions} options Optional options such as signal for aborting */ - addHandler(eventType: T, handler: EventHandler): void { + addHandler(eventType: T, handler: EventHandler, options?: EventOptions): void { [...new Map([[undefined, this._defaultProvider]]), ...this._domainScopedProviders].forEach((keyProviderTuple) => { const domain = keyProviderTuple[0]; const provider = keyProviderTuple[1].provider; @@ -173,6 +173,11 @@ export abstract class OpenFeatureCommonAPI< }); this._apiEmitter.addHandler(eventType, handler); + if (options?.signal && typeof options.signal.addEventListener === 'function') { + options.signal.addEventListener('abort', () => { + this.removeHandler(eventType, handler); + }); + } } /** @@ -248,7 +253,7 @@ export abstract class OpenFeatureCommonAPI< // initialize the provider if it implements "initialize" and it's not already registered if (typeof provider.initialize === 'function' && !this.allProviders.includes(provider)) { initializationPromise = provider - .initialize?.(domain ? this._domainScopedContext.get(domain) ?? this._context : this._context) + .initialize?.(domain ? (this._domainScopedContext.get(domain) ?? this._context) : this._context) ?.then(() => { wrappedProvider.status = this._statusEnumType.READY; // fetch the most recent event emitters, some may have been added during init diff --git a/packages/shared/test/events.spec.ts b/packages/shared/test/events.spec.ts index f490892a9..a5597329f 100644 --- a/packages/shared/test/events.spec.ts +++ b/packages/shared/test/events.spec.ts @@ -14,17 +14,23 @@ class TestEventEmitter extends GenericEventEmitter { } } -// a little function to make sure we're at least waiting for the event loop +// a little function to make sure we're at least waiting for the event loop // to clear before we start making assertions const wait = (millis = 0) => { - return new Promise(resolve => {setTimeout(resolve, millis);}); + return new Promise((resolve) => { + setTimeout(resolve, millis); + }); }; describe('GenericEventEmitter', () => { + const emitter = new TestEventEmitter(); + + afterEach(() => { + emitter.removeAllHandlers(); + }); + describe('addHandler should', function () { it('attach handler for event type', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); emitter.emit(AllProviderEvents.Ready); @@ -35,8 +41,6 @@ describe('GenericEventEmitter', () => { }); it('attach several handlers for event type', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); const handler2 = jest.fn(); const handler3 = jest.fn(); @@ -64,7 +68,6 @@ describe('GenericEventEmitter', () => { debug: () => done(), }; - const emitter = new TestEventEmitter(); emitter.setLogger(logger); emitter.addHandler(AllProviderEvents.Ready, async () => { @@ -74,8 +77,6 @@ describe('GenericEventEmitter', () => { }); it('trigger handler for event type', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); emitter.emit(AllProviderEvents.Ready); @@ -87,7 +88,6 @@ describe('GenericEventEmitter', () => { it('trigger handler for event type with event data', async function () { const event: ReadyEvent = { message: 'message' }; - const emitter = new TestEventEmitter(); const handler1 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); @@ -99,8 +99,6 @@ describe('GenericEventEmitter', () => { }); it('trigger several handlers for event type', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); const handler2 = jest.fn(); const handler3 = jest.fn(); @@ -121,8 +119,6 @@ describe('GenericEventEmitter', () => { describe('removeHandler should', () => { it('remove single handler', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); @@ -138,8 +134,6 @@ describe('GenericEventEmitter', () => { describe('removeAllHandlers should', () => { it('remove all handlers for event type', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); const handler2 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); @@ -156,8 +150,6 @@ describe('GenericEventEmitter', () => { }); it('remove same handler when assigned to multiple events', async function () { - const emitter = new TestEventEmitter(); - const handler = jest.fn(); emitter.addHandler(AllProviderEvents.Stale, handler); emitter.addHandler(AllProviderEvents.ContextChanged, handler); @@ -174,8 +166,6 @@ describe('GenericEventEmitter', () => { }); it('allow addition/removal of duplicate handlers', async function () { - const emitter = new TestEventEmitter(); - const handler = jest.fn(); emitter.addHandler(AllProviderEvents.Stale, handler); emitter.addHandler(AllProviderEvents.Stale, handler); @@ -191,8 +181,6 @@ describe('GenericEventEmitter', () => { }); it('allow duplicate event handlers and call them', async function () { - const emitter = new TestEventEmitter(); - const handler = jest.fn(); emitter.addHandler(AllProviderEvents.Stale, handler); emitter.addHandler(AllProviderEvents.Stale, handler); @@ -205,8 +193,6 @@ describe('GenericEventEmitter', () => { }); it('remove all handlers only for event type', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); const handler2 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); @@ -223,8 +209,6 @@ describe('GenericEventEmitter', () => { }); it('remove all handlers if no event type is given', async function () { - const emitter = new TestEventEmitter(); - const handler1 = jest.fn(); const handler2 = jest.fn(); emitter.addHandler(AllProviderEvents.Ready, handler1); diff --git a/packages/web/src/client/internal/open-feature-client.ts b/packages/web/src/client/internal/open-feature-client.ts index 0e0379f8e..7eed9a9a6 100644 --- a/packages/web/src/client/internal/open-feature-client.ts +++ b/packages/web/src/client/internal/open-feature-client.ts @@ -12,6 +12,7 @@ import type { OpenFeatureError, FlagMetadata, ResolutionDetails, + EventOptions, } from '@openfeature/core'; import { ErrorCode, @@ -74,7 +75,7 @@ export class OpenFeatureClient implements Client { return this.providerStatusAccessor(); } - addHandler(eventType: ProviderEvents, handler: EventHandler): void { + addHandler(eventType: ProviderEvents, handler: EventHandler, options: EventOptions): void { this.emitterAccessor().addHandler(eventType, handler); const shouldRunNow = statusMatchesEvent(eventType, this.providerStatus); @@ -90,6 +91,12 @@ export class OpenFeatureClient implements Client { this._logger?.error('Error running event handler:', err); } } + + if (options?.signal && typeof options.signal.addEventListener === 'function') { + options.signal.addEventListener('abort', () => { + this.removeHandler(eventType, handler); + }); + } } removeHandler(notificationType: ProviderEvents, handler: EventHandler): void { diff --git a/packages/web/test/events.spec.ts b/packages/web/test/events.spec.ts index ea020b438..208b027fc 100644 --- a/packages/web/test/events.spec.ts +++ b/packages/web/test/events.spec.ts @@ -476,7 +476,21 @@ describe('Events', () => { expect(OpenFeature.getHandlers(eventType)).toHaveLength(0); }); - it('The API provides a function allowing the removal of event handlers', () => { + it('The event handler can be removed using an abort signal', () => { + const abortController = new AbortController(); + const handler1 = jest.fn(); + const handler2 = jest.fn(); + const eventType = ProviderEvents.Stale; + + OpenFeature.addHandler(eventType, handler1, { signal: abortController.signal }); + OpenFeature.addHandler(eventType, handler2); + expect(OpenFeature.getHandlers(eventType)).toHaveLength(2); + + abortController.abort(); + expect(OpenFeature.getHandlers(eventType)).toHaveLength(1); + }); + + it('The API provides a function allowing the removal of event handlers from client', () => { const client = OpenFeature.getClient(domain); const handler = jest.fn(); const eventType = ProviderEvents.Stale; @@ -486,6 +500,21 @@ describe('Events', () => { client.removeHandler(eventType, handler); expect(client.getHandlers(eventType)).toHaveLength(0); }); + + it('The event handler on the client can be removed using an abort signal', () => { + const abortController = new AbortController(); + const client = OpenFeature.getClient(domain); + const handler1 = jest.fn(); + const handler2 = jest.fn(); + const eventType = ProviderEvents.Stale; + + client.addHandler(eventType, handler1, { signal: abortController.signal }); + client.addHandler(eventType, handler2); + expect(client.getHandlers(eventType)).toHaveLength(2); + + abortController.abort(); + expect(client.getHandlers(eventType)).toHaveLength(1); + }); }); describe('Requirement 5.3.1', () => {