Repository: open-feature/js-sdk
Pull request #931 (2024-05-03 15:19:26)

fix: skip reconciling event for synchronous onContextChange operations
## 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




Patch:
diff --git a/package-lock.json b/package-lock.json
index 9a57cd20..7ada32b8 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -13516,9 +13516,9 @@
       }
     },
     "node_modules/typedoc": {
-      "version": "0.25.12",
-      "resolved": "https://registry.npmjs.org/typedoc/-/typedoc-0.25.12.tgz",
-      "integrity": "sha512-F+qhkK2VoTweDXd1c42GS/By2DvI2uDF4/EpG424dTexSHdtCH52C6IcAvMA6jR3DzAWZjHpUOW+E02kyPNUNw==",
+      "version": "0.25.13",
+      "resolved": "https://registry.npmjs.org/typedoc/-/typedoc-0.25.13.tgz",
+      "integrity": "sha512-pQqiwiJ+Z4pigfOnnysObszLiU3mVLWAExSPf+Mu06G/qsc3wzbuM56SZQvONhHLncLUhYzOVkjFFpFfL5AzhQ==",
       "dev": true,
       "dependencies": {
         "lunr": "^2.3.9",
@@ -14164,7 +14164,7 @@
     },
     "packages/react": {
       "name": "@openfeature/react-sdk",
-      "version": "0.3.3",
+      "version": "0.3.4",
       "license": "Apache-2.0",
       "devDependencies": {
         "@openfeature/core": "*",
diff --git a/packages/client/src/open-feature.ts b/packages/client/src/open-feature.ts
index 82f1a4b3..c0bcde6c 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 8b590366..6b51ec73 100644
--- 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.


Test patch:
diff --git a/packages/client/test/events.spec.ts b/packages/client/test/events.spec.ts
index 5272694f..51ceceb8 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<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);