Merge pull request #110 from bcdev/yogesh-xcube-viewer-490-fix-unnecessary-state-changes

forman · web-flow · commit f6118e4e53fe · 2025-03-15T10:14:16.000+01:00
Add memoization for invoking callbacks
diff --git a/chartlets.js/CHANGES.md b/chartlets.js/CHANGES.md
@@ -3,6 +3,8 @@
 * Add `multiple` property for `Select` component to enable the selection
   of multiple elements. The `default` mode is supported at the moment.
 
+* Callbacks will now only be invoked when there’s an actual change in state, 
+  reducing unnecessary processing and improving performance. (#112)
 
 ## Version 0.1.4 (from 2025/03/06)
 
diff --git a/chartlets.js/package-lock.json b/chartlets.js/package-lock.json
diff --git a/chartlets.js/packages/lib/src/actions/handleHostStoreChange.ts b/chartlets.js/packages/lib/src/actions/handleHostStoreChange.ts
@@ -11,6 +11,7 @@ import { invokeCallbacks } from "@/actions/helpers/invokeCallbacks";
 import type { ContributionState } from "@/types/state/contribution";
 import type { HostStore } from "@/types/state/host";
 import { store } from "@/store";
+import { shallowEqualArrays } from "@/utils/compare";
 
 /**
  * A reference to a property of an input of a callback of a contribution.
@@ -43,29 +44,65 @@ export function handleHostStoreChange() {
     contributionsRecord,
     hostStore,
   );
-  if (callbackRequests && callbackRequests.length > 0) {
-    invokeCallbacks(callbackRequests);
+
+  const filteredCallbackRequests = callbackRequests.filter(
+    (callbackRequest): callbackRequest is CallbackRequest =>
+      callbackRequest !== undefined,
+  );
+  if (filteredCallbackRequests && filteredCallbackRequests.length > 0) {
+    invokeCallbacks(filteredCallbackRequests);
   }
 }
 
-function getCallbackRequests(
+// Exporting for testing only
+export function getCallbackRequests(
   propertyRefs: PropertyRef[],
   contributionsRecord: Record<string, ContributionState[]>,
   hostStore: HostStore,
-): CallbackRequest[] {
-  return propertyRefs.map((propertyRef) => {
-    const contributions = contributionsRecord[propertyRef.contribPoint];
-    const contribution = contributions[propertyRef.contribIndex];
-    const callback = contribution.callbacks![propertyRef.callbackIndex];
-    const inputValues = getInputValues(
-      callback.inputs!,
-      contribution,
+): (CallbackRequest | undefined)[] {
+  const { lastCallbackInputValues } = store.getState();
+  return propertyRefs.map((propertyRef) =>
+    getCallbackRequest(
+      propertyRef,
+      lastCallbackInputValues,
+      contributionsRecord,
       hostStore,
-    );
-    return { ...propertyRef, inputValues };
-  });
+    ),
+  );
 }
 
+const getCallbackRequest = (
+  propertyRef: PropertyRef,
+  lastCallbackInputValues: Record<string, unknown[]>,
+  contributionsRecord: Record<string, ContributionState[]>,
+  hostStore: HostStore,
+) => {
+  const contribPoint: string = propertyRef.contribPoint;
+  const contribIndex: number = propertyRef.contribIndex;
+  const callbackIndex: number = propertyRef.callbackIndex;
+  const contributions = contributionsRecord[contribPoint];
+  const contribution = contributions[contribIndex];
+  const callback = contribution.callbacks![callbackIndex];
+  const inputValues = getInputValues(callback.inputs!, contribution, hostStore);
+  const callbackId = `${contribPoint}-${contribIndex}-${callbackIndex}`;
+  const lastInputValues = lastCallbackInputValues[callbackId];
+  if (shallowEqualArrays(lastInputValues, inputValues)) {
+    // We no longer log, as the situation is quite common
+    // Enable error logging for debugging only:
+    // console.groupCollapsed("Skipping callback request");
+    // console.debug("inputValues", inputValues);
+    // console.groupEnd();
+    return undefined;
+  }
+  store.setState({
+    lastCallbackInputValues: {
+      ...lastCallbackInputValues,
+      [callbackId]: inputValues,
+    },
+  });
+  return { ...propertyRef, inputValues };
+};
+
 // TODO: use a memoized selector to get hostStorePropertyRefs
 // Note that this will only be effective and once we split the
 // static contribution infos and dynamic contribution states.
diff --git a/chartlets.js/packages/lib/src/actions/handleHostStoreChanges.test.tsx b/chartlets.js/packages/lib/src/actions/handleHostStoreChanges.test.tsx
@@ -1,6 +1,12 @@
-import { describe, it, expect, beforeEach } from "vitest";
+import { beforeEach, describe, expect, it } from "vitest";
 import { store } from "@/store";
-import { handleHostStoreChange } from "./handleHostStoreChange";
+import {
+  getCallbackRequests,
+  handleHostStoreChange,
+  type PropertyRef,
+} from "./handleHostStoreChange";
+import type { ContribPoint } from "@/types/model/extension";
+import type { ContributionState } from "@/types/state/contribution";
 
 describe("handleHostStoreChange", () => {
   let listeners: (() => void)[] = [];
@@ -15,10 +21,12 @@ describe("handleHostStoreChange", () => {
       listeners.push(_l);
     },
   };
+  let lastCallbackInputValues: Record<string, unknown[]> = {};
 
   beforeEach(() => {
     listeners = [];
     hostState = {};
+    lastCallbackInputValues = {};
   });
 
   it("should do nothing without host store", () => {
@@ -78,8 +86,137 @@ describe("handleHostStoreChange", () => {
           },
         },
       },
+      contributionsRecord: {
+        panel: [
+          {
+            name: "p0",
+            container: { title: "Panel A" },
+            extension: "e0",
+            componentResult: {},
+            initialState: {},
+            callbacks: [
+              {
+                function: {
+                  name: "callback",
+                  parameters: [],
+                  return: {},
+                },
+                inputs: [{ id: "@app", property: "variableName" }],
+                outputs: [{ id: "select", property: "value" }],
+              },
+            ],
+          },
+        ],
+      },
     });
     hostStore.set("variableName", "CHL");
     handleHostStoreChange();
+
+    // calling it second time for coverage. No state change changes the
+    // control flow
+    handleHostStoreChange();
+    //   TODO: Update this test to assert the generated callback request
+  });
+
+  it("should memoize second call with same arguments", () => {
+    const extensions = [{ name: "ext", version: "0", contributes: ["panels"] }];
+    store.setState({
+      configuration: { hostStore, logging: { enabled: false } },
+      extensions,
+      contributionsResult: {
+        status: "ok",
+        data: {
+          extensions,
+          contributions: {
+            panels: [
+              {
+                name: "ext.p1",
+                extension: "ext",
+                layout: {
+                  function: {
+                    name: "layout",
+                    parameters: [],
+                    return: {},
+                  },
+                  inputs: [],
+                  outputs: [],
+                },
+                callbacks: [
+                  {
+                    function: {
+                      name: "callback",
+                      parameters: [],
+                      return: {},
+                    },
+                    inputs: [{ id: "@app", property: "variableName" }],
+                  },
+                ],
+                initialState: {},
+              },
+            ],
+          },
+        },
+      },
+      lastCallbackInputValues: lastCallbackInputValues,
+    });
+    const propertyRefs: PropertyRef[] = [
+      {
+        contribPoint: "panel",
+        contribIndex: 0,
+        callbackIndex: 0,
+        property: "value",
+        inputIndex: 0,
+      },
+    ];
+    const contributionsRecord: Record<ContribPoint, ContributionState[]> = {
+      panel: [
+        {
+          name: "ext.p1",
+          container: { title: "Panel A" },
+          extension: "ext",
+          componentResult: {},
+          initialState: { title: "Panel A" },
+          callbacks: [
+            {
+              function: {
+                name: "callback",
+                parameters: [{ name: "param1" }],
+                return: {},
+              },
+              inputs: [{ id: "@app", property: "variableName" }],
+            },
+          ],
+        },
+      ],
+    };
+
+    hostStore.set("variableName", "CHL");
+
+    // first call -> should create callback request
+    let result = getCallbackRequests(
+      propertyRefs,
+      contributionsRecord,
+      hostStore,
+    );
+    expect(result[0]).toEqual({
+      ...propertyRefs[0],
+      inputValues: ["CHL"],
+    });
+
+    // second call -> memoized -> should not create callback request
+    result = getCallbackRequests(propertyRefs, contributionsRecord, hostStore);
+    expect(result).toEqual([undefined]);
+
+    // Third call - change state -> should create callback request
+    hostStore.set("variableName", "TMP");
+    result = getCallbackRequests(propertyRefs, contributionsRecord, hostStore);
+    expect(result[0]).toEqual({
+      ...propertyRefs[0],
+      inputValues: ["TMP"],
+    });
+
+    // fourth call -> memoized -> should not invoke callback
+    result = getCallbackRequests(propertyRefs, contributionsRecord, hostStore);
+    expect(result).toEqual([undefined]);
   });
 });
diff --git a/chartlets.js/packages/lib/src/actions/helpers/getInputValues.ts b/chartlets.js/packages/lib/src/actions/helpers/getInputValues.ts
@@ -26,7 +26,7 @@ export function getInputValues(
 
 const noValue = {};
 
-export function getInputValue(
+function getInputValue(
   input: Input,
   contributionState: ContributionState,
   hostStore?: HostStore,
diff --git a/chartlets.js/packages/lib/src/store.ts b/chartlets.js/packages/lib/src/store.ts
@@ -7,4 +7,5 @@ export const store = create<StoreState>(() => ({
   extensions: [],
   contributionsResult: {},
   contributionsRecord: {},
+  lastCallbackInputValues: {},
 }));
diff --git a/chartlets.js/packages/lib/src/types/state/store.ts b/chartlets.js/packages/lib/src/types/state/store.ts
@@ -25,4 +25,9 @@ export interface StoreState {
    * See hook `useThemeMode()`.
    */
   themeMode?: ThemeMode;
+  /**
+   * Store last input values for callback requests to avoid invoking them if
+   * there are no  state changes
+   */
+  lastCallbackInputValues: Record<string, unknown[]>;
 }
diff --git a/chartlets.js/packages/lib/src/utils/compare.test.ts b/chartlets.js/packages/lib/src/utils/compare.test.ts
@@ -0,0 +1,38 @@
+import { describe, expect, it } from "vitest";
+import { shallowEqualArrays } from "@/utils/compare";
+
+describe("Test shallowEqualArrays()", () => {
+  const arr_a: string[] = ["a", "b", "c"];
+  const arr_b: string[] = ["a", "b", "c"];
+  const arr_c: string[] = ["a", "b", "d"];
+  const arr_d: string[] = [];
+  const arr_e: string[] = ["a", "b", "c", "d"];
+  const arr_f: (string | null)[] = ["a", "b", "c", null];
+  const arr_g: (string | null)[] = ["a", "b", "c", null];
+  const arr_h = [1, [1, 2, 3], [4, 5, 6]];
+  const arr_i = [1, [1, 2, 3], [4, 5, 6]];
+  const arr_j: (number | string | null)[] = [1, 2, "c", null];
+  const arr_k: (number | string | null)[] = [1, 3, "c", null];
+  const arr_l: (number | string | null)[] = [1, 2, "c", null];
+  const arr_m: number[] = [1, 2];
+  const arr_n: number[] = [1, 2];
+  const arr_o: null[] = [null];
+  const arr_p: null[] = [null];
+  const arr_q: null[] = [];
+  it("works", () => {
+    expect(shallowEqualArrays(arr_a, arr_b)).toBe(true);
+    expect(shallowEqualArrays(arr_a, arr_c)).toBe(false);
+    expect(shallowEqualArrays(arr_a, arr_d)).toBe(false);
+    expect(shallowEqualArrays(arr_a, arr_e)).toBe(false);
+    expect(shallowEqualArrays(arr_e, arr_c)).toBe(false);
+    expect(shallowEqualArrays(arr_f, arr_g)).toBe(true);
+    expect(shallowEqualArrays(arr_h, arr_i)).toBe(false);
+    expect(shallowEqualArrays(arr_j, arr_k)).toBe(false);
+    expect(shallowEqualArrays(arr_j, arr_l)).toBe(true);
+    expect(shallowEqualArrays(arr_m, arr_n)).toBe(true);
+    expect(shallowEqualArrays(arr_m, arr_l)).toBe(false);
+    expect(shallowEqualArrays(arr_o, arr_p)).toBe(true);
+    expect(shallowEqualArrays(arr_p, arr_q)).toBe(false);
+    expect(shallowEqualArrays(arr_p)).toBe(false);
+  });
+});
diff --git a/chartlets.js/packages/lib/src/utils/compare.ts b/chartlets.js/packages/lib/src/utils/compare.ts
@@ -0,0 +1,12 @@
+export function shallowEqualArrays(
+  arr1?: unknown[],
+  arr2?: unknown[],
+): boolean {
+  if (!arr1 || !arr2) {
+    return false;
+  }
+  if (arr1.length !== arr2.length) {
+    return false;
+  }
+  return arr1.every((val, index) => val === arr2[index]);
+}
