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
13 changes: 6 additions & 7 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,18 @@
"start": "openmrs develop",
"verify": "turbo run lint test typescript",
"postinstall": "husky install",
"test": "cross-env TZ=UTC jest --config jest.config.json --verbose false --passWithNoTests --color",
"test-watch": "cross-env TZ=UTC jest --watch --config jest.config.json --color",
"test": "turbo run test",
"test-e2e": "playwright test",
"coverage": "yarn test --coverage"
"coverage": "turbo run test --coverage"
},
"devDependencies": {
"@playwright/test": "^1.50.1",
"@swc/core": "^1.11.29",
"@swc/jest": "^0.2.38",
"@testing-library/dom": "^9.3.4",
"@testing-library/jest-dom": "^6.1.5",
"@testing-library/react": "^14.1.2",
"@testing-library/user-event": "^14.5.2",
"@testing-library/dom": "^10.4.1",
"@testing-library/jest-dom": "^6.8.0",
"@testing-library/react": "^16.3.0",
"@testing-library/user-event": "^14.6.1",
"@types/jest": "^29.5.12",
"@types/react": "^18.3.21",
"@types/react-dom": "^18.3.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,13 @@ describe('implementer tools config', () => {
_type: Type.Array,
_value: [],
},
expression: {
_default: undefined,
_description: expect.any(String),
_source: 'default',
_type: Type.String,
_value: undefined,
},
},
'Translation overrides': {
_default: {},
Expand Down Expand Up @@ -1261,7 +1268,7 @@ describe('extension config', () => {
expect(result).toStrictEqual({
bar: 'qux',
baz: 'bazzy',
'Display conditions': { privileges: [] },
'Display conditions': { expression: undefined, privileges: [] },
'Translation overrides': {},
});
expect(console.error).not.toHaveBeenCalled();
Expand All @@ -1284,7 +1291,7 @@ describe('extension config', () => {
expect(result).toStrictEqual({
bar: 'qux',
baz: 'quiz',
'Display conditions': { privileges: [] },
'Display conditions': { expression: undefined, privileges: [] },
'Translation overrides': {},
});
expect(console.error).not.toHaveBeenCalled();
Expand Down Expand Up @@ -1317,10 +1324,9 @@ describe('extension config', () => {
const result = getExtensionConfig('barSlot', 'fooExt').getState().config;
expect(result).toStrictEqual({
qux: 'quxolotl',
'Display conditions': { privileges: [] },
'Display conditions': { expression: undefined, privileges: [] },
'Translation overrides': {},
});
expect(console.error).not.toHaveBeenCalled();
});

it("uses the 'configure' config if one is present, with extension config schema", () => {
Expand All @@ -1343,7 +1349,7 @@ describe('extension config', () => {
const result = getExtensionConfig('barSlot', 'fooExt#id2').getState().config;
expect(result).toStrictEqual({
qux: 'quxotic',
'Display conditions': { privileges: [] },
'Display conditions': { expression: undefined, privileges: [] },
'Translation overrides': {},
});
});
Expand Down
131 changes: 81 additions & 50 deletions packages/framework/esm-config/src/module-config/module-config.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
/** @module @category Config */
import { clone, reduce, mergeDeepRight, equals, omit } from 'ramda';
import { clone, equals, reduce, mergeDeepRight, omit } from 'ramda';
import type { Config, ConfigObject, ConfigSchema, ExtensionSlotConfig } from '../types';
import { Type } from '../types';
import { isArray, isBoolean, isUuid, isNumber, isObject, isString } from '../validators/type-validators';
import { validator } from '../validators/validator';
import { type ConfigExtensionStore, type ConfigInternalStore, type ConfigStore } from './state';
import {
configInternalStore,
configExtensionStore,
configInternalStore,
getConfigStore,
getExtensionConfig,
getExtensionSlotsConfigStore,
getExtensionsConfigStore,
implementerToolsConfigStore,
temporaryConfigStore,
getExtensionSlotsConfigStore,
} from './state';
import { type TemporaryConfigStore } from '..';

Expand All @@ -37,42 +37,37 @@ import { type TemporaryConfigStore } from '..';
* store values at the end. `computeExtensionConfigs` calls `getGlobalStore`,
* which creates stores.
*/
computeModuleConfig(configInternalStore.getState(), temporaryConfigStore.getState());
configInternalStore.subscribe((configState) => computeModuleConfig(configState, temporaryConfigStore.getState()));
temporaryConfigStore.subscribe((tempConfigState) =>
computeModuleConfig(configInternalStore.getState(), tempConfigState),
);

computeImplementerToolsConfig(configInternalStore.getState(), temporaryConfigStore.getState());
configInternalStore.subscribe((configState) =>
computeImplementerToolsConfig(configState, temporaryConfigStore.getState()),
);
temporaryConfigStore.subscribe((tempConfigState) =>
computeImplementerToolsConfig(configInternalStore.getState(), tempConfigState),
);

computeExtensionSlotConfigs(configInternalStore.getState(), temporaryConfigStore.getState());
configInternalStore.subscribe((configState) =>
computeExtensionSlotConfigs(configState, temporaryConfigStore.getState()),
);
temporaryConfigStore.subscribe((tempConfigState) =>
computeExtensionSlotConfigs(configInternalStore.getState(), tempConfigState),
);

computeExtensionConfigs(
configInternalStore.getState(),
configExtensionStore.getState(),
temporaryConfigStore.getState(),
);
configInternalStore.subscribe((configState) => {
computeExtensionConfigs(configState, configExtensionStore.getState(), temporaryConfigStore.getState());
});
configExtensionStore.subscribe((extensionState) => {
computeExtensionConfigs(configInternalStore.getState(), extensionState, temporaryConfigStore.getState());
});
temporaryConfigStore.subscribe((tempConfigState) => {
computeExtensionConfigs(configInternalStore.getState(), configExtensionStore.getState(), tempConfigState);
});
// Store unsubscribe functions to allow cleanup (e.g., in tests or hot module reloading)
const configSubscriptions: Array<() => void> = [];

/**
* Recomputes all configuration derived stores based on current state of input stores.
* Called whenever any input store (configInternalStore, temporaryConfigStore, configExtensionStore) changes.
*/
function recomputeAllConfigs() {
Copy link
Member

Choose a reason for hiding this comment

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

This should be roughly equivalent to what was happening before, albeit with less chaining.

const configState = configInternalStore.getState();
const tempConfigState = temporaryConfigStore.getState();
const extensionState = configExtensionStore.getState();

computeModuleConfig(configState, tempConfigState);
computeImplementerToolsConfig(configState, tempConfigState);
computeExtensionSlotConfigs(configState, tempConfigState);
computeExtensionConfigs(configState, extensionState, tempConfigState);
}

function setupConfigSubscriptions() {
// Initial computation
recomputeAllConfigs();

// Subscribe to all input stores with a single handler
// This ensures we only recompute once even if multiple stores change simultaneously
configSubscriptions.push(configInternalStore.subscribe(recomputeAllConfigs));
configSubscriptions.push(temporaryConfigStore.subscribe(recomputeAllConfigs));
configSubscriptions.push(configExtensionStore.subscribe(recomputeAllConfigs));
}

// Set up subscriptions at module load time
setupConfigSubscriptions();

function computeModuleConfig(state: ConfigInternalStore, tempState: TemporaryConfigStore) {
for (let moduleName of Object.keys(state.schemas)) {
Expand All @@ -83,21 +78,24 @@ function computeModuleConfig(state: ConfigInternalStore, tempState: TemporaryCon
// available, which as of this writing blocks the schema definition from occurring
// for modules loaded based on their extensions.
const moduleStore = getConfigStore(moduleName);
let newState;
if (state.moduleLoaded[moduleName]) {
const config = getConfigForModule(moduleName, state, tempState);
moduleStore.setState({
newState = {
translationOverridesLoaded: true,
loaded: true,
config,
});
};
} else {
const config = getConfigForModuleImplicitSchema(moduleName, state, tempState);
moduleStore.setState({
newState = {
translationOverridesLoaded: true,
loaded: false,
config,
});
};
}

moduleStore.setState(newState);
}
}

Expand All @@ -109,14 +107,21 @@ function computeExtensionSlotConfigs(state: ConfigInternalStore, tempState: Temp
const slotStore = getExtensionSlotsConfigStore();
const oldState = slotStore.getState();
const newState = { slots: { ...oldState.slots, ...newSlotStoreEntries } };
if (!equals(oldState, newState)) {

if (!equals(oldState.slots, newState.slots)) {
Copy link
Member

Choose a reason for hiding this comment

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

These comparison guards should mean that we're calling setState() less frequently

slotStore.setState(newState);
}
}

function computeImplementerToolsConfig(state: ConfigInternalStore, tempConfigState: TemporaryConfigStore) {
const oldState = implementerToolsConfigStore.getState();
const config = getImplementerToolsConfig(state, tempConfigState);
implementerToolsConfigStore.setState({ config });
const newState = { config };

// Use deep equality on the actual config content, not the wrapper object
if (!equals(oldState.config, newState.config)) {
implementerToolsConfigStore.setState(newState);
}
}

function computeExtensionConfigs(
Expand All @@ -137,12 +142,19 @@ function computeExtensionConfigs(
tempConfigState,
);

configs[extension.slotName] = {
...configs[extension.slotName],
[extension.extensionId]: { config, loaded: true },
};
if (!configs[extension.slotName]) {
configs[extension.slotName] = {};
}
configs[extension.slotName][extension.extensionId] = { config, loaded: true };
}
const extensionsConfigStore = getExtensionsConfigStore();
const oldState = extensionsConfigStore.getState();
const newState = { configs };

// Use deep equality to only update if configs actually changed
if (!equals(oldState.configs, newState.configs)) {
extensionsConfigStore.setState(newState);
}
getExtensionsConfigStore().setState({ configs });
}

/*
Expand Down Expand Up @@ -862,6 +874,20 @@ export function clearConfigErrors(keyPath?: string) {
}
}

/**
* Cleans up all config store subscriptions and re-establishes them. This is primarily
* useful for testing, where subscriptions set up at module load time need to be cleared
* between tests to prevent infinite update loops. After clearing, subscriptions are
* re-established so the config system continues to work normally.
*
* @internal
*/
export function resetConfigSystem() {
configSubscriptions.forEach((unsubscribe) => unsubscribe());
configSubscriptions.length = 0;
setupConfigSubscriptions();
}

/**
* Copied over from esm-extensions. It rightly belongs to that module, but esm-config
* cannot depend on esm-extensions.
Expand Down Expand Up @@ -905,6 +931,11 @@ const implicitConfigSchema: ConfigSchema = {
_type: Type.Array,
_default: [],
},
expression: {
_description: 'The expression that determines whether the extension is displayed',
_type: Type.String,
_default: undefined,
},
},
...translationOverridesSchema,
};
2 changes: 1 addition & 1 deletion packages/framework/esm-config/src/module-config/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ function initializeConfigStore() {

/** @internal */
export function getConfigStore(moduleName: string) {
// We use a store for each module's config, named `config-${moduleName}`
// We use a store for each module's config, named `config-module-${moduleName}`
return getGlobalStore<ConfigStore>(`config-module-${moduleName}`, initializeConfigStore());
}

Expand Down
14 changes: 14 additions & 0 deletions packages/framework/esm-expression-evaluator/src/evaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,13 @@ export function evaluateAsType<T>(
throw `Unknown expression type ${expression}. Expressions must either be a string or pre-compiled string.`;
}

if (typeof expression === 'string' && expression.trim().length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

One issue here is that we didn't prevent attempted evaluation of an empty string

throw {
type: 'Empty expression',
message: 'Expression cannot be an empty string',
};
}

if (typeof variables === 'undefined' || variables === null) {
variables = {};
}
Expand Down Expand Up @@ -286,6 +293,13 @@ export async function evaluateAsTypeAsync<T>(
);
}

if (typeof expression === 'string' && expression.trim().length === 0) {
throw {
type: 'Empty expression',
message: 'Expression cannot be an empty string',
};
}

if (typeof variables === 'undefined' || variables === null) {
variables = {};
}
Expand Down
Loading
Loading