From 49c90562f8efb58946b4c98189a9488cc24f0979 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 21 Jan 2026 16:32:12 -0500 Subject: [PATCH 1/8] feat!: update runtime config support BREAKING CHANGE: rename `mfeConfigApiUrl` to `runtimeConfigJsonUrl` BREAKING CHANGE: remove `siteId` from config URL building add cache buster for development environments --- docs/how_tos/migrate-frontend-app.md | 2 +- runtime/config/index.ts | 4 ++-- runtime/initialize.js | 17 +++++++++++------ runtime/initialize.test.js | 4 ++-- shell/site.config.dev.tsx | 2 +- test-site/site.config.build.tsx | 2 +- test-site/site.config.dev.tsx | 2 +- types.ts | 2 +- 8 files changed, 20 insertions(+), 15 deletions(-) diff --git a/docs/how_tos/migrate-frontend-app.md b/docs/how_tos/migrate-frontend-app.md index 890cca7c..ed8e568d 100644 --- a/docs/how_tos/migrate-frontend-app.md +++ b/docs/how_tos/migrate-frontend-app.md @@ -609,7 +609,7 @@ Other configuration is now optional, and many values have been given sensible de - environment: EnvironmentTypes - basename: string -- mfeConfigApiUrl: string | null +- runtimeConfigJsonUrl: string | null - accessTokenCookieName: string - languagePreferenceCookieName: string - userInfoCookieName: string diff --git a/runtime/config/index.ts b/runtime/config/index.ts index 1222c0fb..8399d239 100644 --- a/runtime/config/index.ts +++ b/runtime/config/index.ts @@ -88,7 +88,7 @@ * * https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/mfe_config_api/docs/decisions/0001-mfe-config-api.rst * - * The runtime configuration method can be enabled by supplying a mfeConfigApiUrl via one of the other + * The runtime configuration method can be enabled by supplying a runtimeConfigJsonUrl via one of the other * two configuration methods above. * * Runtime configuration is particularly useful if you need to supply different configurations to @@ -123,7 +123,7 @@ let siteConfig: SiteConfig = { apps: [], externalRoutes: [], externalLinkUrlOverrides: [], - mfeConfigApiUrl: null, + runtimeConfigJsonUrl: null, theme: {}, accessTokenCookieName: 'edx-jwt-cookie-header-payload', csrfTokenApiPath: '/csrf/api/v1/token', diff --git a/runtime/initialize.js b/runtime/initialize.js index 9030d820..461ea0a2 100644 --- a/runtime/initialize.js +++ b/runtime/initialize.js @@ -90,6 +90,7 @@ import { } from './logging'; import { GoogleAnalyticsLoader } from './scripts'; import { publish } from './subscriptions'; +import { EnvironmentTypes } from '../types'; /** * If set in configuration, a basename will be prepended to all relative routes under BrowserRouter. @@ -165,17 +166,21 @@ async function fileConfig() { */ async function runtimeConfig() { try { - const { mfeConfigApiUrl, siteId } = getSiteConfig(); + const { runtimeConfigJsonUrl, environment } = getSiteConfig(); - if (mfeConfigApiUrl) { + if (runtimeConfigJsonUrl) { const apiConfig = { headers: { accept: 'application/json' } }; const apiService = await configureCache(); - const params = new URLSearchParams(); - params.append('mfe', siteId); - const url = `${mfeConfigApiUrl}?${params.toString()}`; + const runtimeConfigUrl = new URL(runtimeConfigJsonUrl); - const { data } = await apiService.get(url, apiConfig); + // In development mode, add a timestamp as a cache buster + // to support live-editing runtime config JSON + if (environment === EnvironmentTypes.DEVELOPMENT) { + runtimeConfigUrl.searchParams.set('timestamp', Date.now()); + } + + const { data } = await apiService.get(runtimeConfigUrl.toString(), apiConfig); mergeSiteConfig(data); } } catch (error) { diff --git a/runtime/initialize.test.js b/runtime/initialize.test.js index b75f9b89..38627a76 100644 --- a/runtime/initialize.test.js +++ b/runtime/initialize.test.js @@ -274,7 +274,7 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - mfeConfigApiUrl: 'http://localhost:18000/api/mfe/v1/config', + runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config', siteId: 'auth', }); } @@ -324,7 +324,7 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - mfeConfigApiUrl: 'http://localhost:18000/api/mfe/v1/config', + runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config', siteId: 'auth', }); } diff --git a/shell/site.config.dev.tsx b/shell/site.config.dev.tsx index 32f29527..63ebed95 100644 --- a/shell/site.config.dev.tsx +++ b/shell/site.config.dev.tsx @@ -43,7 +43,7 @@ const siteConfig: SiteConfig = { // API URLs lmsBaseUrl: 'http://local.openedx.io:8000', - mfeConfigApiUrl: 'http://apps.local.openedx.io:8080/api/mfe_config/v1', + runtimeConfigJsonUrl: 'http://apps.local.openedx.io:8080/api/mfe_config/v1', }; export default siteConfig; diff --git a/test-site/site.config.build.tsx b/test-site/site.config.build.tsx index 7aeffedc..abd9585a 100644 --- a/test-site/site.config.build.tsx +++ b/test-site/site.config.build.tsx @@ -13,7 +13,7 @@ const siteConfig: SiteConfig = { logoutUrl: 'http://local.openedx.io:8000/logout', environment: EnvironmentTypes.PRODUCTION, - mfeConfigApiUrl: 'http://apps.local.openedx.io:8080/api/mfe_config/v1', + runtimeConfigJsonUrl: 'http://apps.local.openedx.io:8080/api/mfe_config/v1', apps: [ shellApp, headerApp, diff --git a/test-site/site.config.dev.tsx b/test-site/site.config.dev.tsx index e6feff4f..41bfcb24 100644 --- a/test-site/site.config.dev.tsx +++ b/test-site/site.config.dev.tsx @@ -13,7 +13,7 @@ const siteConfig: SiteConfig = { logoutUrl: 'http://local.openedx.io:8000/logout', environment: EnvironmentTypes.DEVELOPMENT, - mfeConfigApiUrl: 'http://apps.local.openedx.io:8080/api/mfe_config/v1', + runtimeConfigJsonUrl: 'http://apps.local.openedx.io:8080/api/mfe_config/v1', apps: [ shellApp, headerApp, diff --git a/types.ts b/types.ts index 77c5cd58..6fd051fc 100644 --- a/types.ts +++ b/types.ts @@ -58,7 +58,7 @@ export interface OptionalSiteConfig { basename: string, externalRoutes: ExternalRoute[], externalLinkUrlOverrides: string[], - mfeConfigApiUrl: string | null, + runtimeConfigJsonUrl: string | null, // Theme theme: Theme, From 18bac8e5624d01fd92d25472ed001e0fa837e330 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 3 Feb 2026 20:22:29 -0500 Subject: [PATCH 2/8] test: update runtime config test for top-level config merging - Remove siteId from test config (no longer used) - Simplify configureCache mock (no URL param parsing needed) - Rename test to clarify it tests merge behavior - Test proper merge: runtime overrides build-time, build-time values preserved, runtime values added Co-Authored-By: Claude Opus 4.5 --- runtime/initialize.test.js | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/runtime/initialize.test.js b/runtime/initialize.test.js index 38627a76..0a537a1c 100644 --- a/runtime/initialize.test.js +++ b/runtime/initialize.test.js @@ -259,13 +259,15 @@ describe('initialize', () => { expect(overrideHandlers.initError).toHaveBeenCalledWith(new Error('uhoh!')); }); - it('should initialize the app with runtime configuration', async () => { + // TODO: newConfig at top of file is now unused - consider removing + it('should merge runtime configuration with build-time configuration', async () => { + const runtimeConfig = { + siteName: 'Runtime Site Name', + supportEmail: 'runtime-support@example.com', + }; + configureCache.mockReturnValueOnce(Promise.resolve({ - get: (url) => { - const params = new URL(url).search; - const mfe = new URLSearchParams(params).get('mfe'); - return ({ data: { ...newConfig.common, ...newConfig[mfe] } }); - }, + get: () => ({ data: runtimeConfig }), })); const messages = { i_am: 'a message' }; @@ -274,8 +276,9 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config', - siteId: 'auth', + runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config.json', + siteName: 'Build Time Site Name', + lmsBaseUrl: 'http://localhost:18000', }); } } @@ -301,9 +304,13 @@ describe('initialize', () => { expect(ensureAuthenticatedUser).not.toHaveBeenCalled(); expect(hydrateAuthenticatedUser).not.toHaveBeenCalled(); expect(logError).not.toHaveBeenCalled(); - expect(getSiteConfig().siteName).toBe(newConfig.common.siteName); - expect(getSiteConfig().INFO_EMAIL).toBe(newConfig.auth.INFO_EMAIL); - expect(Object.values(getSiteConfig()).includes(newConfig.learning.DISCUSSIONS_MFE_BASE_URL)).toBeFalsy(); + + // Runtime config should override build-time config for matching keys + expect(getSiteConfig().siteName).toBe('Runtime Site Name'); + // Build-time values not in runtime config should be preserved + expect(getSiteConfig().lmsBaseUrl).toBe('http://localhost:18000'); + // Runtime values not in build-time config should be added + expect(getSiteConfig().supportEmail).toBe('runtime-support@example.com'); }); describe('with mocked console.error', () => { @@ -324,8 +331,7 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config', - siteId: 'auth', + runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config.json', }); } } From e2c63433203c55b1b8b15a28fc92f3c2cf855517 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 3 Feb 2026 20:27:38 -0500 Subject: [PATCH 3/8] test: add app-level runtime config merge test - Test that app-level config in runtime properly merges with build-time app config - Simulates full flow by calling addAppConfigs() after initialize - Verifies runtime overrides build-time, build-time preserved, and runtime additions work at the app config level Co-Authored-By: Claude Opus 4.5 --- runtime/initialize.test.js | 50 +++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/runtime/initialize.test.js b/runtime/initialize.test.js index 0a537a1c..773da334 100644 --- a/runtime/initialize.test.js +++ b/runtime/initialize.test.js @@ -21,7 +21,7 @@ import { hydrateAuthenticatedUser, } from './auth'; import configureCache from './auth/LocalForageCache'; -import { getSiteConfig, mergeSiteConfig } from './config'; +import { addAppConfigs, getAppConfig, getSiteConfig, mergeSiteConfig } from './config'; import { configureI18n } from './i18n'; import { configureLogging, @@ -313,6 +313,54 @@ describe('initialize', () => { expect(getSiteConfig().supportEmail).toBe('runtime-support@example.com'); }); + it('should merge app-level runtime configuration with build-time configuration', async () => { + const runtimeConfig = { + apps: [{ + appId: 'test-app', + config: { + FEATURE_FLAG: true, + INFO_EMAIL: 'runtime@example.com', + }, + }], + }; + + configureCache.mockReturnValueOnce(Promise.resolve({ + get: () => ({ data: runtimeConfig }), + })); + + const messages = { i_am: 'a message' }; + await initialize({ + messages, + handlers: { + config: () => { + mergeSiteConfig({ + runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config.json', + apps: [{ + appId: 'test-app', + config: { + INFO_EMAIL: 'buildtime@example.com', + LOGO_URL: 'http://localhost/logo.png', + }, + }], + }); + } + } + }); + + expect(configureCache).toHaveBeenCalled(); + + // Simulate shell behavior - extract app configs from merged site config + addAppConfigs(); + + const appConfig = getAppConfig('test-app'); + // Runtime config should override build-time app config + expect(appConfig.INFO_EMAIL).toBe('runtime@example.com'); + // Build-time app config not in runtime should be preserved + expect(appConfig.LOGO_URL).toBe('http://localhost/logo.png'); + // Runtime app config not in build-time should be added + expect(appConfig.FEATURE_FLAG).toBe(true); + }); + describe('with mocked console.error', () => { beforeEach(() => { console.error = jest.fn(); From c72f4ded69f78a6eedbda0eeb2593ea63d000845 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 3 Feb 2026 20:29:00 -0500 Subject: [PATCH 4/8] refactor: remove unused newConfig test fixture Co-Authored-By: Claude Opus 4.5 --- runtime/initialize.test.js | 24 ------------------------ 1 file changed, 24 deletions(-) diff --git a/runtime/initialize.test.js b/runtime/initialize.test.js index 773da334..5e315b90 100644 --- a/runtime/initialize.test.js +++ b/runtime/initialize.test.js @@ -38,29 +38,6 @@ jest.mock('./i18n'); jest.mock('./auth/LocalForageCache'); let config = null; -const newConfig = { - common: { - siteName: 'Test Case', - accessTokenCookieName: 'edx-jwt-cookie-header-payload', - csrfTokenApiPath: '/csrf/api/v1/token', - languagePreferenceCookieName: 'openedx-language-preference', - lmsBaseUrl: 'http://test.example.com:18000', - loginUrl: 'http://test.example.com:18000/login', - logoutUrl: 'http://test.example.com:18000/logout', - refreshAccessTokenApiPath: '/login_refresh', - segmentKey: '', - userInfoCookieName: 'edx-user-info', - ignoredErrorRegex: '', - }, - auth: { - INFO_EMAIL: 'openedx@example.com', - ACTIVATION_EMAIL_SUPPORT_LINK: 'http//support.test.com', - }, - learning: { - LEGACY_THEME_NAME: 'example', - DISCUSSIONS_MFE_BASE_URL: 'http://test.example.com:2002', - }, -}; describe('initialize', () => { beforeEach(() => { @@ -259,7 +236,6 @@ describe('initialize', () => { expect(overrideHandlers.initError).toHaveBeenCalledWith(new Error('uhoh!')); }); - // TODO: newConfig at top of file is now unused - consider removing it('should merge runtime configuration with build-time configuration', async () => { const runtimeConfig = { siteName: 'Runtime Site Name', From 61320127e388f1867b7b39aa4a69522ab3cb0911 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 3 Feb 2026 20:48:04 -0500 Subject: [PATCH 5/8] refactor: use realistic config values and fake.url pattern - Replace supportEmail with theme config (realistic optional config) - Use fake.url pattern for test URLs: - fake.config.url for runtime config endpoint - fake.cdn.url for CDN resources (theme, logos) - fake.lms.url for LMS base URL Co-Authored-By: Claude Opus 4.5 --- runtime/initialize.test.js | 39 ++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/runtime/initialize.test.js b/runtime/initialize.test.js index 5e315b90..3c9896b3 100644 --- a/runtime/initialize.test.js +++ b/runtime/initialize.test.js @@ -239,7 +239,16 @@ describe('initialize', () => { it('should merge runtime configuration with build-time configuration', async () => { const runtimeConfig = { siteName: 'Runtime Site Name', - supportEmail: 'runtime-support@example.com', + theme: { + defaults: { + light: 'light', + }, + variants: { + light: { + url: 'http://fake.cdn.url/light.css', + }, + }, + }, }; configureCache.mockReturnValueOnce(Promise.resolve({ @@ -252,9 +261,9 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config.json', + runtimeConfigJsonUrl: 'http://fake.config.url/config.json', siteName: 'Build Time Site Name', - lmsBaseUrl: 'http://localhost:18000', + lmsBaseUrl: 'http://fake.lms.url', }); } } @@ -283,10 +292,13 @@ describe('initialize', () => { // Runtime config should override build-time config for matching keys expect(getSiteConfig().siteName).toBe('Runtime Site Name'); + // Build-time values not in runtime config should be preserved - expect(getSiteConfig().lmsBaseUrl).toBe('http://localhost:18000'); + expect(getSiteConfig().lmsBaseUrl).toBe('http://fake.lms.url'); + // Runtime values not in build-time config should be added - expect(getSiteConfig().supportEmail).toBe('runtime-support@example.com'); + expect(getSiteConfig().theme.defaults.light).toBe('light'); + expect(getSiteConfig().theme.variants.light.url).toBe('http://fake.cdn.url/light.css'); }); it('should merge app-level runtime configuration with build-time configuration', async () => { @@ -295,7 +307,7 @@ describe('initialize', () => { appId: 'test-app', config: { FEATURE_FLAG: true, - INFO_EMAIL: 'runtime@example.com', + INFO_EMAIL: 'runtime@fake.email', }, }], }; @@ -310,12 +322,12 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config.json', + runtimeConfigJsonUrl: 'http://fake.config.url/config.json', apps: [{ appId: 'test-app', config: { - INFO_EMAIL: 'buildtime@example.com', - LOGO_URL: 'http://localhost/logo.png', + INFO_EMAIL: 'buildtime@fake.email', + LOGO_URL: 'http://fake.cdn.url/logo.png', }, }], }); @@ -329,10 +341,13 @@ describe('initialize', () => { addAppConfigs(); const appConfig = getAppConfig('test-app'); + // Runtime config should override build-time app config - expect(appConfig.INFO_EMAIL).toBe('runtime@example.com'); + expect(appConfig.INFO_EMAIL).toBe('runtime@fake.email'); + // Build-time app config not in runtime should be preserved - expect(appConfig.LOGO_URL).toBe('http://localhost/logo.png'); + expect(appConfig.LOGO_URL).toBe('http://fake.cdn.url/logo.png'); + // Runtime app config not in build-time should be added expect(appConfig.FEATURE_FLAG).toBe(true); }); @@ -355,7 +370,7 @@ describe('initialize', () => { handlers: { config: () => { mergeSiteConfig({ - runtimeConfigJsonUrl: 'http://localhost:18000/api/mfe/v1/config.json', + runtimeConfigJsonUrl: 'http://fake.config.url/config.json', }); } } From ca88aa96342597ac5e03e36421ce2a569d3ed37a Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Tue, 3 Feb 2026 23:11:36 -0500 Subject: [PATCH 6/8] fix: merge apps by appId instead of array index - Add appConfigOnly option to mergeSiteConfig for runtime config - Runtime config: only merge config for existing apps, ignore new apps - Build-time config: full app merge by appId, allow adding new apps - Add lodash.keyby dependency for cleaner implementation Co-Authored-By: Claude Opus 4.5 --- package-lock.json | 19 +++++++++++++- package.json | 2 ++ runtime/config/index.ts | 55 +++++++++++++++++++++++++++++++++++++++-- runtime/initialize.js | 2 +- 4 files changed, 74 insertions(+), 4 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7563ccf8..09cacd47 100644 --- a/package-lock.json +++ b/package-lock.json @@ -22,6 +22,7 @@ "@stylistic/eslint-plugin": "^2.9.0", "@types/eslint__js": "^8.42.3", "@types/gradient-string": "^1.1.6", + "@types/lodash.keyby": "^4.6.9", "autoprefixer": "^10.4.20", "axios": "^1.7.9", "axios-cache-interceptor": "^1.6.0", @@ -56,6 +57,7 @@ "localforage": "^1.10.0", "localforage-memoryStorageDriver": "^0.9.2", "lodash.camelcase": "^4.3.0", + "lodash.keyby": "^4.6.0", "lodash.memoize": "^4.1.2", "lodash.merge": "^4.6.2", "lodash.snakecase": "^4.1.1", @@ -4997,7 +4999,6 @@ "version": "4.17.21", "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.17.21.tgz", "integrity": "sha512-FOvQ0YPD5NOfPgMzJihoT+Za5pdkDJWcbpuj1DjaKZIr/gxodQjY/uWEFlTNqW2ugXHUiL8lRQgw63dzKHZdeQ==", - "dev": true, "license": "MIT" }, "node_modules/@types/lodash.camelcase": { @@ -5010,6 +5011,15 @@ "@types/lodash": "*" } }, + "node_modules/@types/lodash.keyby": { + "version": "4.6.9", + "resolved": "https://registry.npmjs.org/@types/lodash.keyby/-/lodash.keyby-4.6.9.tgz", + "integrity": "sha512-N8xfQdZ2ADNPDL72TaLozIL4K1xFCMG1C1T9GN4dOFI+sn1cjl8d4U+POp8PRCAnNxDCMkYAZVD/rOBIWYPT5g==", + "license": "MIT", + "dependencies": { + "@types/lodash": "*" + } + }, "node_modules/@types/lodash.merge": { "version": "4.6.9", "resolved": "https://registry.npmjs.org/@types/lodash.merge/-/lodash.merge-4.6.9.tgz", @@ -13155,6 +13165,12 @@ "integrity": "sha512-FT1yDzDYEoYWhnSGnpE/4Kj1fLZkDFyqRb7fNt6FdYOSxlUWAtp42Eh6Wb0rGIv/m9Bgo7x4GhQbm5Ys4SG5ow==", "license": "MIT" }, + "node_modules/lodash.keyby": { + "version": "4.6.0", + "resolved": "https://registry.npmjs.org/lodash.keyby/-/lodash.keyby-4.6.0.tgz", + "integrity": "sha512-PRe4Cn20oJM2Sn6ljcZMeKgyhTHpzvzFmdsp9rK+6K0eJs6Tws0MqgGFpfX/o2HjcoQcBny1Eik9W7BnVTzjIQ==", + "license": "MIT" + }, "node_modules/lodash.memoize": { "version": "4.1.2", "resolved": "https://registry.npmjs.org/lodash.memoize/-/lodash.memoize-4.1.2.tgz", @@ -20010,6 +20026,7 @@ "resolved": "https://registry.npmjs.org/webpack-cli/-/webpack-cli-5.1.4.tgz", "integrity": "sha512-pIDJHIEI9LR0yxHXQ+Qh95k2EvXpWzZ5l+d+jIo+RdSm9MiHfzazIxwwni/p7+x4eJZuvG1AJwgC4TNQ7NRgsg==", "license": "MIT", + "peer": true, "dependencies": { "@discoveryjs/json-ext": "^0.5.0", "@webpack-cli/configtest": "^2.1.1", diff --git a/package.json b/package.json index 2982e8de..a2d55d53 100644 --- a/package.json +++ b/package.json @@ -60,6 +60,7 @@ "@stylistic/eslint-plugin": "^2.9.0", "@types/eslint__js": "^8.42.3", "@types/gradient-string": "^1.1.6", + "@types/lodash.keyby": "^4.6.9", "autoprefixer": "^10.4.20", "axios": "^1.7.9", "axios-cache-interceptor": "^1.6.0", @@ -94,6 +95,7 @@ "localforage": "^1.10.0", "localforage-memoryStorageDriver": "^0.9.2", "lodash.camelcase": "^4.3.0", + "lodash.keyby": "^4.6.0", "lodash.memoize": "^4.1.2", "lodash.merge": "^4.6.2", "lodash.snakecase": "^4.1.1", diff --git a/runtime/config/index.ts b/runtime/config/index.ts index 8399d239..c9a94ecf 100644 --- a/runtime/config/index.ts +++ b/runtime/config/index.ts @@ -100,6 +100,7 @@ * @module Config */ +import keyBy from 'lodash.keyby'; import merge from 'lodash.merge'; import { AppConfig, @@ -174,6 +175,10 @@ export function setSiteConfig(newSiteConfig: SiteConfig) { publish(CONFIG_CHANGED); } +interface MergeSiteConfigOptions { + appConfigOnly?: boolean, +} + /** * Merges additional configuration values into the site config returned by `getSiteConfig`. Will * override any values that exist with the same keys. @@ -188,10 +193,56 @@ export function setSiteConfig(newSiteConfig: SiteConfig) { * which means they will be merged recursively. See https://lodash.com/docs/latest#merge for * documentation on the exact behavior. * + * Apps are merged by appId rather than array index. By default, new apps can be added. + * When `appConfigOnly` is true, only the `config` property of existing apps is merged, + * and new apps are ignored. + * * @param {Object} newSiteConfig + * @param {Object} options + * @param {boolean} options.appConfigOnly - Only merge app config for existing apps */ -export function mergeSiteConfig(newSiteConfig: Partial) { - siteConfig = merge(siteConfig, newSiteConfig); +export function mergeSiteConfig( + newSiteConfig: Partial, + options: MergeSiteConfigOptions = {} +) { + const { appConfigOnly = false } = options; + const { apps: newApps, ...restOfNewConfig } = newSiteConfig; + + // lodash merge the top-level (non-app) part + siteConfig = merge(siteConfig, restOfNewConfig); + + // if we don't have new apps, we're done + if (!newApps?.length) { + publish(CONFIG_CHANGED); + return; + } + + // if we're doing a full merge, merge the objects + if (!appConfigOnly) { + siteConfig.apps = Object.values(merge( + keyBy(siteConfig.apps || [], 'appId'), + keyBy(newApps, 'appId') + )); + publish(CONFIG_CHANGED); + return; + } + + // we're doing a config-only merge, if we don't + // have apps already, we can't update their configs + if (!siteConfig.apps?.length) { + publish(CONFIG_CHANGED); + return; + } + + // handle config-only merging + const newAppsById = keyBy(newApps, 'appId'); + for (const app of siteConfig.apps) { + const newApp = newAppsById[app.appId]; + if (newApp?.config) { + app.config = merge(app.config, newApp.config); + } + } + publish(CONFIG_CHANGED); } diff --git a/runtime/initialize.js b/runtime/initialize.js index 461ea0a2..485091f6 100644 --- a/runtime/initialize.js +++ b/runtime/initialize.js @@ -181,7 +181,7 @@ async function runtimeConfig() { } const { data } = await apiService.get(runtimeConfigUrl.toString(), apiConfig); - mergeSiteConfig(data); + mergeSiteConfig(data, { appConfigOnly: true }); } } catch (error) { console.error('Error with config API', error.message); From d05935e8d42393b0b8e17e12cd8d28533ee5ad92 Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 4 Feb 2026 00:08:46 -0500 Subject: [PATCH 7/8] test: add unit tests for mergeSiteConfig Add comprehensive unit tests covering all code paths in mergeSiteConfig: - Top-level config merging (new values, overrides, preservation) - App merging with full merge (default behavior) - App merging with appConfigOnly option - Edge cases (empty arrays, undefined apps, missing config) Uses jest.spyOn pattern for verifying publish(CONFIG_CHANGED) calls. Co-Authored-By: Claude Opus 4.5 --- runtime/config/index.test.ts | 298 +++++++++++++++++++++++++++++++++++ 1 file changed, 298 insertions(+) create mode 100644 runtime/config/index.test.ts diff --git a/runtime/config/index.test.ts b/runtime/config/index.test.ts new file mode 100644 index 00000000..0d89f6e6 --- /dev/null +++ b/runtime/config/index.test.ts @@ -0,0 +1,298 @@ +import { CONFIG_CHANGED } from '../constants'; +import * as subscriptions from '../subscriptions'; +import { + getSiteConfig, + mergeSiteConfig, + setSiteConfig, +} from './index'; + +const defaultSiteConfig = { + siteId: '', + baseUrl: '', + siteName: '', + loginUrl: '', + logoutUrl: '', + lmsBaseUrl: '', + apps: [], +}; + +describe('mergeSiteConfig', () => { + let publishSpy: jest.SpyInstance; + + beforeEach(() => { + setSiteConfig({ ...defaultSiteConfig }); + publishSpy = jest.spyOn(subscriptions, 'publish'); + }); + + afterEach(() => { + publishSpy.mockRestore(); + }); + + describe('top-level config merging', () => { + it('should merge new values into site config', () => { + mergeSiteConfig({ + siteName: 'Test Site', + lmsBaseUrl: 'http://fake.lms.url', + }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + expect(getSiteConfig().siteName).toBe('Test Site'); + expect(getSiteConfig().lmsBaseUrl).toBe('http://fake.lms.url'); + }); + + it('should override existing values', () => { + setSiteConfig({ ...defaultSiteConfig, siteName: 'Original' }); + publishSpy.mockClear(); + + mergeSiteConfig({ siteName: 'Updated' }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + expect(getSiteConfig().siteName).toBe('Updated'); + }); + + it('should preserve existing values not in new config', () => { + setSiteConfig({ ...defaultSiteConfig, siteName: 'Original', lmsBaseUrl: 'http://original.url' }); + publishSpy.mockClear(); + + mergeSiteConfig({ siteName: 'Updated' }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + expect(getSiteConfig().siteName).toBe('Updated'); + expect(getSiteConfig().lmsBaseUrl).toBe('http://original.url'); + }); + }); + + describe('app merging (full merge, default behavior)', () => { + it('should add new apps when none exist', () => { + mergeSiteConfig({ + apps: [{ + appId: 'new-app', + config: { FEATURE: true }, + }], + }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + expect(getSiteConfig().apps).toHaveLength(1); + expect(getSiteConfig().apps![0].appId).toBe('new-app'); + expect(getSiteConfig().apps![0].config!.FEATURE).toBe(true); + }); + + it('should merge apps by appId, not array index', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [ + { appId: 'app-one', config: { VALUE: 'one' } }, + { appId: 'app-two', config: { VALUE: 'two' } }, + ], + }); + publishSpy.mockClear(); + + // New config has apps in different order + mergeSiteConfig({ + apps: [ + { appId: 'app-two', config: { EXTRA: 'added-to-two' } }, + ], + }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + const apps = getSiteConfig().apps!; + expect(apps).toHaveLength(2); + + const appOne = apps.find(a => a.appId === 'app-one')!; + const appTwo = apps.find(a => a.appId === 'app-two')!; + + // app-one should be unchanged + expect(appOne.config!.VALUE).toBe('one'); + expect(appOne.config!.EXTRA).toBeUndefined(); + + // app-two should have merged config + expect(appTwo.config!.VALUE).toBe('two'); + expect(appTwo.config!.EXTRA).toBe('added-to-two'); + }); + + it('should add new apps while preserving existing ones', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'existing-app', config: { VALUE: 'existing' } }], + }); + publishSpy.mockClear(); + + mergeSiteConfig({ + apps: [{ appId: 'new-app', config: { VALUE: 'new' } }], + }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + const apps = getSiteConfig().apps!; + expect(apps).toHaveLength(2); + expect(apps.find(a => a.appId === 'existing-app')).toBeDefined(); + expect(apps.find(a => a.appId === 'new-app')).toBeDefined(); + }); + + it('should deep merge app properties', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ + appId: 'test-app', + config: { NESTED: { a: 1, b: 2 } }, + }], + }); + publishSpy.mockClear(); + + mergeSiteConfig({ + apps: [{ + appId: 'test-app', + config: { NESTED: { b: 3, c: 4 } }, + }], + }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + const app = getSiteConfig().apps!.find(a => a.appId === 'test-app')!; + expect(app.config!.NESTED).toEqual({ a: 1, b: 3, c: 4 }); + }); + }); + + describe('app merging (appConfigOnly: true)', () => { + it('should do nothing when no existing apps', () => { + mergeSiteConfig( + { apps: [{ appId: 'new-app', config: { VALUE: 'test' } }] }, + { appConfigOnly: true } + ); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + expect(getSiteConfig().apps).toHaveLength(0); + }); + + it('should not add new apps', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'existing-app', config: { VALUE: 'existing' } }], + }); + publishSpy.mockClear(); + + mergeSiteConfig( + { apps: [{ appId: 'new-app', config: { VALUE: 'new' } }] }, + { appConfigOnly: true } + ); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + expect(getSiteConfig().apps).toHaveLength(1); + expect(getSiteConfig().apps![0].appId).toBe('existing-app'); + }); + + it('should only merge config for existing apps', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ + appId: 'test-app', + config: { ORIGINAL: 'value', OVERRIDE: 'old' }, + }], + }); + publishSpy.mockClear(); + + mergeSiteConfig( + { + apps: [{ + appId: 'test-app', + config: { OVERRIDE: 'new', ADDED: 'extra' }, + }], + }, + { appConfigOnly: true } + ); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + const app = getSiteConfig().apps![0]; + expect(app.config!.ORIGINAL).toBe('value'); + expect(app.config!.OVERRIDE).toBe('new'); + expect(app.config!.ADDED).toBe('extra'); + }); + + it('should merge by appId, not array index', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [ + { appId: 'app-one', config: { VALUE: 'one' } }, + { appId: 'app-two', config: { VALUE: 'two' } }, + ], + }); + publishSpy.mockClear(); + + // New config only has app-two at index 0 + mergeSiteConfig( + { + apps: [{ appId: 'app-two', config: { EXTRA: 'added' } }], + }, + { appConfigOnly: true } + ); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + const apps = getSiteConfig().apps!; + const appOne = apps.find(a => a.appId === 'app-one')!; + const appTwo = apps.find(a => a.appId === 'app-two')!; + + // app-one should be unchanged (was at index 0, but new config had app-two at index 0) + expect(appOne.config!.VALUE).toBe('one'); + expect(appOne.config!.EXTRA).toBeUndefined(); + + // app-two should have merged config + expect(appTwo.config!.VALUE).toBe('two'); + expect(appTwo.config!.EXTRA).toBe('added'); + }); + }); + + describe('edge cases', () => { + it('should handle empty apps array in new config', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'existing', config: { VALUE: 'test' } }], + }); + publishSpy.mockClear(); + + mergeSiteConfig({ apps: [] }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + // Should be unchanged + expect(getSiteConfig().apps).toHaveLength(1); + expect(getSiteConfig().apps![0].appId).toBe('existing'); + }); + + it('should handle undefined apps in new config', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'existing', config: { VALUE: 'test' } }], + }); + publishSpy.mockClear(); + + mergeSiteConfig({ siteName: 'Updated' }); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + // Apps should be unchanged + expect(getSiteConfig().apps).toHaveLength(1); + expect(getSiteConfig().siteName).toBe('Updated'); + }); + + it('should handle app with no config property', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'test-app', config: { ORIGINAL: 'value' } }], + }); + publishSpy.mockClear(); + + mergeSiteConfig( + { apps: [{ appId: 'test-app' }] }, + { appConfigOnly: true } + ); + + expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); + + // Config should be unchanged + expect(getSiteConfig().apps![0].config!.ORIGINAL).toBe('value'); + }); + }); +}); From b2729a9625718187a7c949779a831d016e8df08e Mon Sep 17 00:00:00 2001 From: Brian Smith Date: Wed, 4 Feb 2026 18:00:15 -0500 Subject: [PATCH 8/8] chore: rename appConfigOnly to limitAppMergeToConfig Clearer name that doesn't imply "only do app stuff" - it still merges all non-app config normally. Also improved JSDoc to clarify behavior. Co-Authored-By: Claude Opus 4.5 --- runtime/config/index.test.ts | 12 ++++++------ runtime/config/index.ts | 18 +++++++++++------- runtime/initialize.js | 2 +- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/runtime/config/index.test.ts b/runtime/config/index.test.ts index 0d89f6e6..f9d537cf 100644 --- a/runtime/config/index.test.ts +++ b/runtime/config/index.test.ts @@ -154,11 +154,11 @@ describe('mergeSiteConfig', () => { }); }); - describe('app merging (appConfigOnly: true)', () => { + describe('app merging (limitAppMergeToConfig: true)', () => { it('should do nothing when no existing apps', () => { mergeSiteConfig( { apps: [{ appId: 'new-app', config: { VALUE: 'test' } }] }, - { appConfigOnly: true } + { limitAppMergeToConfig: true } ); expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); @@ -174,7 +174,7 @@ describe('mergeSiteConfig', () => { mergeSiteConfig( { apps: [{ appId: 'new-app', config: { VALUE: 'new' } }] }, - { appConfigOnly: true } + { limitAppMergeToConfig: true } ); expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); @@ -199,7 +199,7 @@ describe('mergeSiteConfig', () => { config: { OVERRIDE: 'new', ADDED: 'extra' }, }], }, - { appConfigOnly: true } + { limitAppMergeToConfig: true } ); expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); @@ -225,7 +225,7 @@ describe('mergeSiteConfig', () => { { apps: [{ appId: 'app-two', config: { EXTRA: 'added' } }], }, - { appConfigOnly: true } + { limitAppMergeToConfig: true } ); expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); @@ -286,7 +286,7 @@ describe('mergeSiteConfig', () => { mergeSiteConfig( { apps: [{ appId: 'test-app' }] }, - { appConfigOnly: true } + { limitAppMergeToConfig: true } ); expect(publishSpy).toHaveBeenCalledWith(CONFIG_CHANGED); diff --git a/runtime/config/index.ts b/runtime/config/index.ts index c9a94ecf..1b343764 100644 --- a/runtime/config/index.ts +++ b/runtime/config/index.ts @@ -176,7 +176,7 @@ export function setSiteConfig(newSiteConfig: SiteConfig) { } interface MergeSiteConfigOptions { - appConfigOnly?: boolean, + limitAppMergeToConfig?: boolean, } /** @@ -193,19 +193,23 @@ interface MergeSiteConfigOptions { * which means they will be merged recursively. See https://lodash.com/docs/latest#merge for * documentation on the exact behavior. * - * Apps are merged by appId rather than array index. By default, new apps can be added. - * When `appConfigOnly` is true, only the `config` property of existing apps is merged, - * and new apps are ignored. + * Apps are merged by appId rather than array index. By default, apps in the incoming config + * that don't exist in the current config will be added. + * + * When `limitAppMergeToConfig` is true: + * - All non-app parts of the config are still merged normally + * - Only the `config` property of each existing app is merged + * - Apps in the incoming config that don't exist in the current config are ignored * * @param {Object} newSiteConfig * @param {Object} options - * @param {boolean} options.appConfigOnly - Only merge app config for existing apps + * @param {boolean} options.limitAppMergeToConfig - Limit app merging to only the config property of existing apps */ export function mergeSiteConfig( newSiteConfig: Partial, options: MergeSiteConfigOptions = {} ) { - const { appConfigOnly = false } = options; + const { limitAppMergeToConfig = false } = options; const { apps: newApps, ...restOfNewConfig } = newSiteConfig; // lodash merge the top-level (non-app) part @@ -218,7 +222,7 @@ export function mergeSiteConfig( } // if we're doing a full merge, merge the objects - if (!appConfigOnly) { + if (!limitAppMergeToConfig) { siteConfig.apps = Object.values(merge( keyBy(siteConfig.apps || [], 'appId'), keyBy(newApps, 'appId') diff --git a/runtime/initialize.js b/runtime/initialize.js index 485091f6..7ccbffce 100644 --- a/runtime/initialize.js +++ b/runtime/initialize.js @@ -181,7 +181,7 @@ async function runtimeConfig() { } const { data } = await apiService.get(runtimeConfigUrl.toString(), apiConfig); - mergeSiteConfig(data, { appConfigOnly: true }); + mergeSiteConfig(data, { limitAppMergeToConfig: true }); } } catch (error) { console.error('Error with config API', error.message);