Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions chartlets.js/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@
* Add `multiple` property for `Select` component to enable the selection
of multiple elements. The `default` mode is supported at the moment.

* Static information about callbacks retrieved from API is not cached
reducing unnecessary processing and improving performance. (#113)

* Relaxed requirements for adding new components to Chartlets.js via
plugins. Implementing `ComponentProps` is now optional. (#115)

Comment on lines +9 to +11
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this from another PR added by mistake here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will follow

* Callbacks will now only be invoked when there’s an actual change in state,
reducing unnecessary processing and improving performance. (#112)

Expand Down
7 changes: 7 additions & 0 deletions chartlets.js/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions chartlets.js/packages/lib/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"preview": "vite preview"
},
"dependencies": {
"micro-memoize": "^4.1.3",
"microdiff": "^1.4",
"zustand": "^5.0"
},
Expand Down
123 changes: 89 additions & 34 deletions chartlets.js/packages/lib/src/actions/handleHostStoreChange.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import memoize from "micro-memoize";

import type {
Callback,
CallbackRef,
CallbackRequest,
ContribRef,
InputRef,
} from "@/types/model/callback";
import type { Input } from "@/types/model/channel";
import { getInputValues } from "@/actions/helpers/getInputValues";
import { formatObjPath } from "@/utils/objPath";
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";
import { shallowEqualArrays } from "@/utils/shallowEqualArrays";
import type { ContribPoint } from "@/types/model/extension";

/**
* A reference to a property of an input of a callback of a contribution.
Expand All @@ -21,23 +24,31 @@
property: string;
}

export function handleHostStoreChange() {
/**
* This action is called once a host store change has been detected.
* Note this will only create callback requests for callbacks whose input
* value are affected by the change.
*
* @returns The array of callback requests made or `undefined`,
* if no callback requests have been made.
*/
export function handleHostStoreChange(): CallbackRequest[] | undefined {
const { extensions, configuration, contributionsRecord } = store.getState();
const { hostStore } = configuration;
if (!hostStore) {
// Exit if no host store configured.
// Actually, we should not come here.
return;
return undefined;
}
synchronizeThemeMode(hostStore);
if (extensions.length === 0) {
// Exit if there are no extensions (yet)
return;
return undefined;
}
const propertyRefs = getHostStorePropertyRefs();
const propertyRefs = getPropertyRefsForContribPoints(contributionsRecord);
if (!propertyRefs || propertyRefs.length === 0) {
// Exit if there are is nothing to be changed
return;
return undefined;

Check warning on line 51 in chartlets.js/packages/lib/src/actions/handleHostStoreChange.ts

View check run for this annotation

Codecov / codecov/patch

chartlets.js/packages/lib/src/actions/handleHostStoreChange.ts#L51

Added line #L51 was not covered by tests
}
const callbackRequests = getCallbackRequests(
propertyRefs,
Expand All @@ -49,9 +60,12 @@
(callbackRequest): callbackRequest is CallbackRequest =>
callbackRequest !== undefined,
);
if (filteredCallbackRequests && filteredCallbackRequests.length > 0) {
invokeCallbacks(filteredCallbackRequests);
if (!filteredCallbackRequests || !filteredCallbackRequests.length) {
return undefined;
}

invokeCallbacks(filteredCallbackRequests);
return filteredCallbackRequests;
}

// Exporting for testing only
Expand Down Expand Up @@ -103,36 +117,77 @@
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.
// The hostStorePropertyRefs only depend on the static
// contribution infos.

/**
* Get the static list of host state property references for all contributions.
* Get the static list of host state property references
* for given contribution points.
* Note: the export exists only for testing.
*/
function getHostStorePropertyRefs(): PropertyRef[] {
const { contributionsRecord } = store.getState();
export const getPropertyRefsForContribPoints = memoize(
_getPropertyRefsForContribPoints,
);

function _getPropertyRefsForContribPoints(
contributionsRecord: Record<ContribPoint, ContributionState[]>,
): PropertyRef[] {
const propertyRefs: PropertyRef[] = [];
Object.getOwnPropertyNames(contributionsRecord).forEach((contribPoint) => {
const contributions = contributionsRecord[contribPoint];
contributions.forEach((contribution, contribIndex) => {
(contribution.callbacks || []).forEach(
(callback, callbackIndex) =>
(callback.inputs || []).forEach((input, inputIndex) => {
if (!input.noTrigger && input.id === "@app" && input.property) {
propertyRefs.push({
contribPoint,
contribIndex,
callbackIndex,
inputIndex,
property: formatObjPath(input.property),
});
}
}),
[] as Input[],
);
propertyRefs.push(
...getPropertyRefsForContributions(contribPoint, contributions),
);
});
return propertyRefs;
}

/**
* Get the static list of host state property references
* for given contributions.
*/
const getPropertyRefsForContributions = memoize(
_getPropertyRefsForContributions,
);

function _getPropertyRefsForContributions(
contribPoint: string,
contributions: ContributionState[],
): PropertyRef[] {
const propertyRefs: PropertyRef[] = [];
contributions.forEach((contribution, contribIndex) => {
propertyRefs.push(
...getPropertyRefsForCallbacks(
contribPoint,
contribIndex,
contribution.callbacks,
),
);
});
return propertyRefs;
}

/**
* Get the static list of host state property references
* for given callbacks.
*/
const getPropertyRefsForCallbacks = memoize(_getPropertyRefsForCallbacks);

function _getPropertyRefsForCallbacks(
contribPoint: string,
contribIndex: number,
callbacks: Callback[] | undefined,
) {
const propertyRefs: PropertyRef[] = [];
(callbacks || []).forEach((callback, callbackIndex) => {
const inputs = callback.inputs || [];
inputs.forEach((input, inputIndex) => {
if (!input.noTrigger && input.id === "@app" && input.property) {
propertyRefs.push({
contribPoint,
contribIndex,
callbackIndex,
inputIndex,
property: formatObjPath(input.property),
});
}
});
});
return propertyRefs;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it } from "vitest";
import { store } from "@/store";
import {
getCallbackRequests,
getPropertyRefsForContribPoints,
handleHostStoreChange,
type PropertyRef,
} from "./handleHostStoreChange";
Expand All @@ -11,6 +12,7 @@ import type { ContributionState } from "@/types/state/contribution";
describe("handleHostStoreChange", () => {
let listeners: (() => void)[] = [];
let hostState: Record<string, unknown> = {};
// noinspection JSUnusedGlobalSymbols
const hostStore = {
get: (key: string) => hostState[key],
set: (key: string, value: unknown) => {
Expand Down Expand Up @@ -46,6 +48,76 @@ describe("handleHostStoreChange", () => {
expect(store.getState().themeMode).toEqual("light");
});

it("should memoize computation of property refs", () => {
const contributionsRecord: Record<string, ContributionState[]> = {
panels: [
{
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" }],
},
{
function: {
name: "callback2",
parameters: [],
return: {},
},
inputs: [
{ id: "@app", property: "datasetId" },
{ id: "@app", property: "variableName" },
],
outputs: [{ id: "plot", property: "value" }],
},
],
},
],
};
const propertyRefs1 = getPropertyRefsForContribPoints(contributionsRecord);
const propertyRefs2 = getPropertyRefsForContribPoints(contributionsRecord);
const propertyRefs3 = getPropertyRefsForContribPoints({
...contributionsRecord,
});
expect(propertyRefs1).toBe(propertyRefs2);
expect(propertyRefs2).not.toBe(propertyRefs3);
expect(propertyRefs1).toEqual([
{
callbackIndex: 0,
contribIndex: 0,
contribPoint: "panels",
inputIndex: 0,
property: "variableName",
},
{
callbackIndex: 1,
contribIndex: 0,
contribPoint: "panels",
inputIndex: 0,
property: "datasetId",
},
{
callbackIndex: 1,
contribIndex: 0,
contribPoint: "panels",
inputIndex: 1,
property: "variableName",
},
]);
expect(propertyRefs1).toEqual(propertyRefs2);
expect(propertyRefs1).toEqual(propertyRefs3);
expect(propertyRefs2).toEqual(propertyRefs3);
});

it("should generate callback requests", () => {
const extensions = [{ name: "e0", version: "0", contributes: ["panels"] }];
store.setState({
Expand Down Expand Up @@ -110,12 +182,20 @@ describe("handleHostStoreChange", () => {
},
});
hostStore.set("variableName", "CHL");
handleHostStoreChange();
expect(handleHostStoreChange()).toEqual([
{
contribPoint: "panel",
contribIndex: 0,
callbackIndex: 0,
inputIndex: 0,
inputValues: ["CHL"],
property: "variableName",
},
]);

// calling it second time for coverage. No state change changes the
// control flow
handleHostStoreChange();
// TODO: Update this test to assert the generated callback request
// calling it second time for coverage,
// because the no-state-change changes the control flow
expect(handleHostStoreChange()).toBeUndefined();
});

it("should memoize second call with same arguments", () => {
Expand Down
27 changes: 23 additions & 4 deletions chartlets.js/packages/lib/src/types/state/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,33 @@ import type { FrameworkOptions } from "./options";

export type ThemeMode = "dark" | "light" | "system";

// TODO: Split contributionsRecord into two fields comprising static
// contribution data and dynamic contribution states.
// This will allow memoizing the computation of property references
// (PropertyRef[]) on the level of the StoreState from static data only.
// The property references would then be just computed once.
// See function getPropertyRefsForContribPoints()
// in actions/handleHostStoreChange.ts

/**
* The state of the Chartlets main store.
*/
export interface StoreState {
/** Framework configuration */
/**
* Framework configuration.
*/
configuration: FrameworkOptions;
/** All extensions */
/**
* All extensions.
*/
extensions: Extension[];
/** API call result from `GET /contributions`. */
/**
* API call result from `GET /contributions`.
*/
contributionsResult: ApiResult<Contributions>;
/** A record that maps contribPoint --> ContributionState[].*/
/**
* A record that maps contribPoint --> ContributionState[].
*/
contributionsRecord: Record<ContribPoint, ContributionState[]>;
/**
* The app's current theme mode.
Expand Down
12 changes: 0 additions & 12 deletions chartlets.js/packages/lib/src/utils/compare.ts

This file was deleted.

Loading