Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(fix) App not hot updating with slot config changes #357

Merged
merged 3 commits into from
Mar 15, 2022
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
4 changes: 2 additions & 2 deletions .husky/pre-commit
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
set -e # die on error

yarn run extract-translations
npx pretty-quick --staged
npx lerna run document --since master
yarn pretty-quick --staged
yarn lerna run document --since master
6 changes: 3 additions & 3 deletions .husky/pre-push
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,6 @@

set -e # die on error

npx lerna run lint --since master
npx lerna run test --since master
npx lerna run typescript --since master
yarn lerna run lint --since master
yarn lerna run test --since master
yarn lerna run typescript --since master
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ import {
Config,
ConfigInternalStore,
configInternalStore,
getExtensionInternalStore,
implementerToolsConfigStore,
temporaryConfigStore,
useExtensionInternalStore,
useStore,
useStoreWithActions,
} from "@openmrs/esm-framework";
import {
Button,
Expand Down Expand Up @@ -89,13 +90,13 @@ export const Configuration: React.FC<ConfigurationProps> = () => {
toggleIsJsonModeEnabled,
isConfigToolbarOpen,
toggleIsToolbarOpen,
} = useStore(implementerToolsStore, actions);
const { devDefaultsAreOn, toggleDevDefaults } = useStore(
} = useStoreWithActions(implementerToolsStore, actions);
const { devDefaultsAreOn, toggleDevDefaults } = useStoreWithActions(
configInternalStore,
configActions
);
const { config } = useStore(implementerToolsConfigStore);
const extensionStore = useExtensionInternalStore();
const extensionStore = useStore(getExtensionInternalStore());
const tempConfigStore = useStore(temporaryConfigStore);
const [filterText, setFilterText] = useState("");
const tempConfig = tempConfigStore.config;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
import React from "react";
import styles from "./styles.css";
import Close16 from "@carbon/icons-react/es/close/16";
import { useExtensionInternalStore, useStore } from "@openmrs/esm-framework";
import {
getExtensionInternalStore,
useStore,
useStoreWithActions,
} from "@openmrs/esm-framework";
import { Button } from "carbon-components-react";
import { Portal } from "./portal";
import { ExtensionOverlay } from "./extension-overlay.component";
import { ImplementerToolsStore, implementerToolsStore } from "../store";

export default function UiEditor() {
const { slots, extensions } = useExtensionInternalStore();
const { slots, extensions } = useStore(getExtensionInternalStore());
const { isOpen: implementerToolsIsOpen } = useStore(implementerToolsStore);

return (
Expand Down Expand Up @@ -66,7 +70,10 @@ const actions = {
};

export function ExitButton() {
const { toggleIsUIEditorEnabled } = useStore(implementerToolsStore, actions);
const { toggleIsUIEditorEnabled } = useStoreWithActions(
implementerToolsStore,
actions
);
return (
<Button
className={styles.exitButton}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
temporaryConfigStore,
} from "./state";
import { Type } from "../types";
import { getExtensionSlotConfigStore } from "..";
import { getExtensionSlotsConfigStore } from "..";

const mockConfigInternalStore =
configInternalStore as MockedStore<ConfigInternalStore>;
Expand Down Expand Up @@ -854,7 +854,8 @@ describe("extension slot config", () => {
},
},
});
const { config } = getExtensionSlotConfigStore("fooSlot").getState();
const { config } =
getExtensionSlotsConfigStore().getState().slots["fooSlot"];
expect(config).toStrictEqual({
add: ["bar", "baz"],
remove: ["zap"],
Expand Down Expand Up @@ -892,7 +893,8 @@ describe("extension slot config", () => {
},
});
await Config.getConfig("foo-module");
const { config } = getExtensionSlotConfigStore("fooSlot").getState();
const { config } =
getExtensionSlotsConfigStore().getState().slots["fooSlot"];
expect(config).toStrictEqual({ remove: ["bar"] });
});

Expand Down
18 changes: 13 additions & 5 deletions packages/framework/esm-config/src/module-config/module-config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { clone, reduce, mergeDeepRight } from "ramda";
import { clone, reduce, mergeDeepRight, equals } from "ramda";
import {
Config,
ConfigObject,
Expand All @@ -23,9 +23,9 @@ import {
configExtensionStore,
getConfigStore,
getExtensionConfigStore,
getExtensionSlotConfigStore,
implementerToolsConfigStore,
temporaryConfigStore,
getExtensionSlotsConfigStore,
} from "./state";
import type {} from "@openmrs/esm-globals";
import { TemporaryConfigStore } from "..";
Expand Down Expand Up @@ -125,9 +125,17 @@ function computeExtensionSlotConfigs(
tempState: TemporaryConfigStore
) {
const slotConfigs = getExtensionSlotConfigs(state, tempState);
for (let [slotName, config] of Object.entries(slotConfigs)) {
const slotStore = getExtensionSlotConfigStore(slotName);
slotStore.setState({ loaded: true, config });
const newSlotStoreEntries = Object.fromEntries(
Object.entries(slotConfigs).map(([slotName, config]) => [
slotName,
{ loaded: true, config },
])
);
const slotStore = getExtensionSlotsConfigStore();
const oldState = slotStore.getState();
const newState = { slots: { ...oldState.slots, ...newSlotStoreEntries } };
Copy link
Contributor

Choose a reason for hiding this comment

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

This will not remove oldState slots if they are meant to be removed. Is it desired to remove old unused slots from state?

Copy link
Member

Choose a reason for hiding this comment

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

I asked @brandones a similar question once and he indicated the idea is to keep the slots around so that the implementer tools can track them even if the slot itself is no longer rendered.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ibacher is correct. There will not be so many that it will be a memory problem.

if (!equals(oldState, newState)) {
slotStore.setState(newState);
}
}

Expand Down
39 changes: 26 additions & 13 deletions packages/framework/esm-config/src/module-config/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -141,29 +141,42 @@ export function getConfigStore(moduleName: string) {
}

/**
* Configuration for a specific extension slot
* Configuration for all the specific extension slots
* @internal
*/
export interface ExtensionSlotConfigStore {
config: ExtensionSlotConfigObject;
loaded: boolean;
export interface ExtensionSlotsConfigStore {
slots: {
[slotName: string]: {
config: ExtensionSlotConfigObject;
loaded: boolean;
};
};
}

function initializeExtensionSlotConfigStore() {
return {
config: {},
loaded: false,
};
/** @internal */
export function getExtensionSlotsConfigStore() {
return getGlobalStore<ExtensionSlotsConfigStore>(`config-extension-slots`, {
slots: {},
});
}

/** @internal */
export function getExtensionSlotConfigStore(slotName: string) {
return getGlobalStore<ExtensionSlotConfigStore>(
`config-extension-slots-${slotName}`,
initializeExtensionSlotConfigStore()
export function getExtensionSlotConfig(slotName: string) {
return getExtensionSlotConfigFromStore(
getExtensionSlotsConfigStore().getState(),
slotName
);
}

/** @internal */
export function getExtensionSlotConfigFromStore(
state: ExtensionSlotsConfigStore,
slotName: string
) {
const slotConfig = state.slots[slotName];
return slotConfig ?? { loaded: false, config: {} };
}

/**
* A store for each mounted extension's config
* @internal
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export const getExtensionSlotConfig = jest.fn().mockResolvedValue({});
export const getExtensionSlotConfigs = jest.fn().mockResolvedValue({});
56 changes: 46 additions & 10 deletions packages/framework/esm-extensions/src/extensions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@

import {
ExtensionSlotConfigObject,
getExtensionSlotConfigStore,
ExtensionSlotsConfigStore,
getExtensionSlotConfig,
getExtensionSlotConfigFromStore,
getExtensionSlotsConfigStore,
} from "@openmrs/esm-config";
import isEqual from "lodash-es/isEqual";
import {
getExtensionInternalStore,
ExtensionSlotState,
Expand All @@ -30,16 +34,40 @@ const extensionInternalStore = getExtensionInternalStore();
const extensionStore = getExtensionStore();

// Keep the output store updated
extensionInternalStore.subscribe((internalStore) => {
function updateExtensionOutputStore(
internalState: ExtensionInternalStore,
extensionSlotConfigs: ExtensionSlotsConfigStore
) {
const slots: Record<string, ExtensionSlotState> = {};
for (let [slotName, slot] of Object.entries(internalStore.slots)) {
for (let [slotName, slot] of Object.entries(internalState.slots)) {
// Only include registered slots
if (slot.moduleName) {
// Only include registered slots
const assignedExtensions = getAssignedExtensions(slotName);
const { config } = getExtensionSlotConfigFromStore(
extensionSlotConfigs,
slot.name
);
const assignedExtensions = getAssignedExtensionsFromData(
slotName,
internalState,
config
);
slots[slotName] = { moduleName: slot.moduleName, assignedExtensions };
}
}
extensionStore.setState({ slots: slots });
if (!isEqual(extensionStore.getState().slots, slots)) {
extensionStore.setState({ slots });
}
}

extensionInternalStore.subscribe((internalStore) => {
updateExtensionOutputStore(
internalStore,
getExtensionSlotsConfigStore().getState()
);
});

getExtensionSlotsConfigStore().subscribe((slotConfigs) => {
updateExtensionOutputStore(extensionInternalStore.getState(), slotConfigs);
});

function createNewExtensionSlotInfo(
Expand Down Expand Up @@ -246,11 +274,11 @@ export function getConnectedExtensions(
);
}

export function getAssignedExtensions(
slotName: string
function getAssignedExtensionsFromData(
slotName: string,
internalState: ExtensionInternalStore,
config: ExtensionSlotConfigObject
): Array<AssignedExtension> {
const internalState = extensionInternalStore.getState();
const config = getExtensionSlotConfigStore(slotName).getState().config;
const attachedIds = internalState.slots[slotName].attachedIds;
const assignedIds = calculateAssignedIds(config, attachedIds);
const extensions: Array<AssignedExtension> = [];
Expand All @@ -269,6 +297,14 @@ export function getAssignedExtensions(
return extensions;
}

export function getAssignedExtensions(
slotName: string
): Array<AssignedExtension> {
const internalState = extensionInternalStore.getState();
const { config } = getExtensionSlotConfig(slotName);
return getAssignedExtensionsFromData(slotName, internalState, config);
}

function calculateAssignedIds(
config: ExtensionSlotConfigObject,
attachedIds: Array<string>
Expand Down
Loading