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 <[email protected]>
open-feature · May 6, 2024 · 6c25f29 · 6c25f29
1 parent 488ec8a
commit 6c25f29
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 26 deletions.
diff --git 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
@@ -19,12 +19,18 @@ export interface Provider extends CommonProvider<ClientProviderStatus> {
   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<void>;
+  onContextChange?(oldContext: EvaluationContext, newContext: EvaluationContext): Promise<void> | 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
@@ -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<void>;
+  onContextChange?: () => Promise<void> | void;
   initialize?: () => Promise<void>;
 
   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<void>((resolve, reject) =>
-      setTimeout(() => {
-        if (this.failOnContextChange) {
-          reject(new Error(ERR_MESSAGE));
-        } else {
-          resolve();
-        }
-      }, this.asyncDelay),
-    );
+    if (this.asyncContextChangedHandler) {
+      return new Promise<void>((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);