Fb/custom change handler no null value (#66)

* some tests for future behavior

* some tests for future behavior 2

* trigger change handlers after internal state update

* change handlers get the actual new value in case of deletion/resets

* update tests

* slightly more docs for change handlers

Author: rlindner81

docs/usage/index.md

@@ -266,6 +266,14 @@ toggles.registerFeatureValueChangeHandler("/srv/util/logger/logLevel", async (ne
 });
 ```
 
+Some important caveats for change handlers:
+
+- **scopes**: The handler will always receive the new value that is relevant for the scopeMap that is passed along.
+  Meaning it will be called with new values that can be highly restricted and `getFeatureValue` calls with the same
+  feature key can return different values for other scopeMaps or no scopeMap.
+- **deletion**: If values are deleted, by setting them to `null`, the change handler will receive the new actual
+  value for the relevant scopeMap and not the change value of `null`.
+
 {: .info }
 Registering any callback will not require that the feature toggles are initialized, so this can happen on top-level.
 

src/featureToggles.js

@@ -1296,14 +1296,26 @@ class FeatureToggles {
             return;
           }
 
-          await this._triggerChangeHandlers(featureKey, oldValue, newValue, scopeMap, options);
           FeatureToggles._updateStateScopedValuesOneScopeInPlace(
             this.__stateScopedValues,
             featureKey,
             newValue,
             scopeKey,
             options
           );
+
+          // NOTE: the change handler expects the actual value.
+          const newActualValue =
+            newValue !== null
+              ? newValue
+              : FeatureToggles._getFeatureValueForScopeAndStateAndFallback(
+                  this.__superScopeCache,
+                  this.__stateScopedValues,
+                  this.__fallbackValues,
+                  featureKey,
+                  scopeMap
+                );
+          await this._triggerChangeHandlers(featureKey, oldValue, newActualValue, scopeMap, options);
         } catch (err) {
           logger.error(
             new VError(
@@ -1344,14 +1356,24 @@ class FeatureToggles {
         featureKey,
         scopeMap
       );
-      await this._triggerChangeHandlers(featureKey, oldValue, newValue, scopeMap, options);
       FeatureToggles._updateStateScopedValuesOneScopeInPlace(
         this.__stateScopedValues,
         featureKey,
         newValue,
         scopeKey,
         options
       );
+      const newActualValue =
+        newValue !== null
+          ? newValue
+          : FeatureToggles._getFeatureValueForScopeAndStateAndFallback(
+              this.__superScopeCache,
+              this.__stateScopedValues,
+              this.__fallbackValues,
+              featureKey,
+              scopeMap
+            );
+      await this._triggerChangeHandlers(featureKey, oldValue, newActualValue, scopeMap, options);
       return;
     }
 
@@ -1419,10 +1441,10 @@ class FeatureToggles {
    *
    * The change handler gets the new value as well as the old value, immediately after the update is propagated.
    *
-   * @param {boolean | number | string | null}  newValue
-   * @param {boolean | number | string}         oldValue
-   * @param {Object}                            [scopeMap]  optional scope restrictions
-   * @param {ChangeOptions}                     [options]   optional switch to clear all sub scopes
+   * @param {boolean | number | string}  newValue
+   * @param {boolean | number | string}  oldValue
+   * @param {Object}                     [scopeMap]  optional scope restrictions
+   * @param {ChangeOptions}              [options]   optional switch to clear all sub scopes
    */
   /**
    * Register given handler to receive changes of given feature value key.

test/featureToggles.test.js

@@ -53,9 +53,6 @@ describe("feature toggles test", () => {
     redisWrapperMock._reset();
     envMock._reset();
     featureToggles = new FeatureToggles({ redisKey, redisChannel });
-  });
-
-  afterEach(() => {
     jest.clearAllMocks();
   });
 
@@ -1223,4 +1220,56 @@ describe("feature toggles test", () => {
       expect(featureToggles.getFeatureValue(FEATURE.E, scopeMap)).toMatchInlineSnapshot(`9`);
     });
   });
+
+  describe("changeHandler details", () => {
+    const handler = jest.fn();
+    const tenant = "t1";
+    const user = "u1";
+    const fallbackValue = mockFallbackValues[FEATURE.C];
+    const rootValue = "root";
+    const tenantValue = "t1";
+    const tenantUserValue = "t1u1";
+
+    beforeEach(async () => {
+      await featureToggles.initializeFeatures({ config: mockConfig });
+      featureToggles.registerFeatureValueChangeHandler(FEATURE.C, handler);
+      await featureToggles.changeFeatureValue(FEATURE.C, rootValue);
+      await featureToggles.changeFeatureValue(FEATURE.C, tenantValue, { tenant });
+      await featureToggles.changeFeatureValue(FEATURE.C, tenantUserValue, { tenant, user });
+      jest.clearAllMocks();
+    });
+
+    test("reset tenantUser should fall back to tenant", async () => {
+      await featureToggles.changeFeatureValue(FEATURE.C, null, { tenant, user });
+      expect(handler).toHaveBeenCalledTimes(1);
+      expect(handler).toHaveBeenCalledWith(tenantValue, tenantUserValue, { tenant, user }, undefined);
+    });
+    test("reset tenant should fall back to root", async () => {
+      await featureToggles.changeFeatureValue(FEATURE.C, null, { tenant });
+      expect(handler).toHaveBeenCalledTimes(1);
+      expect(handler).toHaveBeenCalledWith(rootValue, tenantValue, { tenant }, undefined);
+    });
+    test("reset root should fall back to fallback", async () => {
+      await featureToggles.changeFeatureValue(FEATURE.C, null);
+      expect(handler).toHaveBeenCalledTimes(1);
+      expect(handler).toHaveBeenCalledWith(fallbackValue, rootValue, undefined, undefined);
+    });
+    test("reset with clearSubScopes root should fall back to fallback", async () => {
+      await featureToggles.changeFeatureValue(FEATURE.C, null, undefined, { clearSubScopes: true });
+      expect(handler).toHaveBeenCalledTimes(1);
+      expect(handler).toHaveBeenCalledWith(fallbackValue, rootValue, undefined, { clearSubScopes: true });
+    });
+
+    test("get in changeHandler should give new value", async () => {
+      let newValueCheck;
+      const newTenantUserValue = "super";
+      handler.mockImplementationOnce(() => {
+        newValueCheck = featureToggles.getFeatureValue(FEATURE.C, { tenant, user });
+      });
+      await featureToggles.changeFeatureValue(FEATURE.C, newTenantUserValue, { tenant, user });
+      expect(newValueCheck).toBe(newTenantUserValue);
+      expect(handler).toHaveBeenCalledTimes(1);
+      expect(handler).toHaveBeenCalledWith(newTenantUserValue, tenantUserValue, { tenant, user }, undefined);
+    });
+  });
 });