From 6c25f29f11ddb9d4ee617f1ed3f1d26be4f554ac Mon Sep 17 00:00:00 2001 From: Michael Beemer Date: Mon, 6 May 2024 14:15:23 -0400 Subject: [PATCH] fix: skip reconciling event for synchronous onContextChange operations (#931) ## This PR - skips emitting a reconciling event for synchronous onContextChange operations ### Notes This will avoid unexpected component rerenders for synchronous onContextChange operations. The spec states that the SDK may avoid emitting the `PROVIDER_RECONCILING` if a provider can reconcile synchronously. https://openfeature.dev/specification/sections/events#event-handlers-and-context-reconciliation --------- Signed-off-by: Michael Beemer --- packages/client/src/open-feature.ts | 22 ++++++---- packages/client/src/provider/provider.ts | 10 ++++- packages/client/test/events.spec.ts | 55 +++++++++++++++++------- 3 files changed, 61 insertions(+), 26 deletions(-) diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts index 82f1a4b35..c0bcde6c2 100644 --- a/packages/client/src/open-feature.ts +++ b/packages/client/src/open-feature.ts @@ -231,14 +231,20 @@ export class OpenFeatureAPI try { if (typeof wrapper.provider.onContextChange === 'function') { - wrapper.incrementPendingContextChanges(); - wrapper.status = this._statusEnumType.RECONCILING; - this.getAssociatedEventEmitters(domain).forEach((emitter) => { - emitter?.emit(ProviderEvents.Reconciling, { domain, providerName }); - }); - this._apiEmitter?.emit(ProviderEvents.Reconciling, { domain, providerName }); - await wrapper.provider.onContextChange(oldContext, newContext); - wrapper.decrementPendingContextChanges(); + const maybePromise = wrapper.provider.onContextChange(oldContext, newContext); + + // only reconcile if the onContextChange method returns a promise + if (typeof maybePromise?.then === 'function') { + wrapper.incrementPendingContextChanges(); + wrapper.status = this._statusEnumType.RECONCILING; + this.getAssociatedEventEmitters(domain).forEach((emitter) => { + emitter?.emit(ProviderEvents.Reconciling, { domain, providerName }); + }); + this._apiEmitter?.emit(ProviderEvents.Reconciling, { domain, providerName }); + + await maybePromise; + wrapper.decrementPendingContextChanges(); + } } // only run the event handlers, and update the state if the onContextChange method succeeded wrapper.status = this._statusEnumType.READY; diff --git a/packages/client/src/provider/provider.ts b/packages/client/src/provider/provider.ts index 8b590366a..6b51ec73f 100644 --- a/packages/client/src/provider/provider.ts +++ b/packages/client/src/provider/provider.ts @@ -19,12 +19,18 @@ export interface Provider extends CommonProvider { readonly hooks?: Hook[]; /** - * A handler function to reconcile changes when the static context. + * A handler function to reconcile changes made to the static context. * Called by the SDK when the context is changed. + * + * Returning a promise will put the provider in the RECONCILING state and + * emit the ProviderEvents.Reconciling event. + * + * Return void will avoid putting the provider in the RECONCILING state and + * **not** emit the ProviderEvents.Reconciling event. * @param oldContext * @param newContext */ - onContextChange?(oldContext: EvaluationContext, newContext: EvaluationContext): Promise; + onContextChange?(oldContext: EvaluationContext, newContext: EvaluationContext): Promise | void; /** * Resolve a boolean flag and its evaluation details. diff --git a/packages/client/test/events.spec.ts b/packages/client/test/events.spec.ts index 5272694f3..51ceceb82 100644 --- a/packages/client/test/events.spec.ts +++ b/packages/client/test/events.spec.ts @@ -22,11 +22,12 @@ class MockProvider implements Provider { readonly runsOn = 'client'; private hasInitialize: boolean; private hasContextChanged: boolean; + private asyncContextChangedHandler: boolean; private failOnInit: boolean; private failOnContextChange: boolean; private asyncDelay?: number; private enableEvents: boolean; - onContextChange?: () => Promise; + onContextChange?: () => Promise | void; initialize?: () => Promise; constructor(options?: { @@ -35,6 +36,7 @@ class MockProvider implements Provider { enableEvents?: boolean; failOnInit?: boolean; hasContextChanged?: boolean; + asyncContextChangedHandler?: boolean; failOnContextChange?: boolean; name?: string; }) { @@ -45,6 +47,7 @@ class MockProvider implements Provider { this.enableEvents = options?.enableEvents ?? true; this.failOnInit = options?.failOnInit ?? false; this.failOnContextChange = options?.failOnContextChange ?? false; + this.asyncContextChangedHandler = options?.asyncContextChangedHandler ?? true; if (this.hasContextChanged) { this.onContextChange = this.changeHandler; } @@ -80,15 +83,19 @@ class MockProvider implements Provider { } private changeHandler() { - return new Promise((resolve, reject) => - setTimeout(() => { - if (this.failOnContextChange) { - reject(new Error(ERR_MESSAGE)); - } else { - resolve(); - } - }, this.asyncDelay), - ); + if (this.asyncContextChangedHandler) { + return new Promise((resolve, reject) => + setTimeout(() => { + if (this.failOnContextChange) { + reject(new Error(ERR_MESSAGE)); + } else { + resolve(); + } + }, this.asyncDelay), + ); + } else if (this.failOnContextChange) { + throw new Error(ERR_MESSAGE); + } } } @@ -598,6 +605,25 @@ describe('Events', () => { expect(handler).toHaveBeenCalledTimes(2); }); + + it('Reconciling events are not emitted for synchronous onContextChange operations', async () => { + const provider = new MockProvider({ + hasInitialize: false, + hasContextChanged: true, + asyncContextChangedHandler: false, + }); + + const reconcileHandler = jest.fn(() => {}); + const changedEventHandler = jest.fn(() => {}); + + await OpenFeature.setProviderAndWait(domain, provider); + OpenFeature.addHandler(ProviderEvents.Reconciling, reconcileHandler); + OpenFeature.addHandler(ProviderEvents.ContextChanged, changedEventHandler); + await OpenFeature.setContext(domain, {}); + + expect(reconcileHandler).not.toHaveBeenCalled(); + expect(changedEventHandler).toHaveBeenCalledTimes(1); + }); }); describe('provider has no context changed handler', () => { @@ -615,7 +641,7 @@ describe('Events', () => { }); }); }); - + describe('client', () => { describe('provider has context changed handler', () => { it('Stale and ContextChanged are emitted', async () => { @@ -810,12 +836,9 @@ describe('Events', () => { }; client.addHandler(ProviderEvents.ContextChanged, handler); - + // update context change twice - await Promise.all([ - OpenFeature.setContext(domain, {}), - OpenFeature.setContext(domain, {}), - ]); + await Promise.all([OpenFeature.setContext(domain, {}), OpenFeature.setContext(domain, {})]); // should only have run once expect(runs).toEqual(1);