diff --git a/docs/decisions/0017-bundled-app-config-defaults.rst b/docs/decisions/0017-bundled-app-config-defaults.rst new file mode 100644 index 00000000..487af62e --- /dev/null +++ b/docs/decisions/0017-bundled-app-config-defaults.rst @@ -0,0 +1,106 @@ +Bundled App Config Defaults +########################### + +Status +====== + +Accepted + + +Context +======= + +``getAppConfig`` merges site-wide and per-app configuration with +``merge({}, commonAppConfig, appConfigs[id])``, so per-app config wins on +overlapping keys. That ordering is correct for the pair it was designed +around: edx-platform maps ``/api/mfe_config/v1`` onto ``commonAppConfig`` and +``/api/mfe_config/v1?mfe=`` onto per-app ``config``, and an operator's +app-specific override must beat their site-wide values. + +The problem is that ``App.config`` does two unrelated jobs. It holds values +bundled by the app author at build time, and values supplied by an operator +through ``site.config`` or the runtime config API. ``mergeApp`` flattens both +into one object before ``addAppConfigs`` runs, so the provenance is lost. + +Every key an app bundles is therefore a key ``commonAppConfig`` can never +supply. + +The ideal merge priority would be: + +* Lowest: Bundled app config (App author provided) +* Next: Common app config (Operator provided) +* Highest: App specific override config (Operator provided) + +This ADR proposes that mechanism. + + +Decision +======== + +Add an optional ``defaultConfig`` to the ``App`` interface, for values bundled +by the app author. ``config`` remains the operator's field:: + + export interface App { + // ... + defaultConfig?: AppConfig; + config?: AppConfig; + // ... + } + +``defaultConfig`` resolves below ``commonAppConfig``, which resolves below +``config``, giving the priority described above. + +The field is optional and additive: for an app that does not set it, resolution +produces exactly today's result. + + +Consequences +============ + +App authors move shipped defaults to ``defaultConfig``, and ``commonAppConfig`` +starts working for every key rather than only those no app happened to bundle. + +Override ergonomics improve as well. Today an operator overriding one value +must remember to spread the app's existing config back in, and forgetting the +inner spread silently discards every other default:: + + { ...exampleApp, config: { ...exampleApp.config, SOME_KEY: true } } + +With a separate field, the outer spread carries ``defaultConfig`` through:: + + { ...exampleApp, config: { SOME_KEY: true } } + + +Rejected alternatives +===================== + +Flipping ``commonAppConfig`` precedence +--------------------------------------- + +Rejected because ``MFE_CONFIG_OVERRIDES`` reaches apps through per-app +``config``, so the flip breaks a channel operators depend on. + +An app-level convention +------------------------ + +Each app could merge its own defaults underneath the runtime's result, for +instance ``merge({}, DEFAULTS, getAppConfig(appId))``. This yields correct +values with no frontend-base change, but every app reimplements the layering +and must apply it at each read site and the runtime gains no visibility into the +defaults. + +A reserved key inside ``config`` +--------------------------------- + +Bundled values under a magic key such as ``config.__defaults``. Not +expressible in the type system, still routes both kinds of value through a +single field, and invites collisions with real keys. + +Dropping bundled defaults entirely +----------------------------------- + +Apps could stop shipping defaults and fall back at their read sites. This is +often the right answer for a given app, but not as a general policy: some +defaults are product decisions an app owns and an operator should be able to +override, such as a default logo. Removing the field would leave nowhere to +express them. diff --git a/docs/how_tos/migrate-frontend-app.md b/docs/how_tos/migrate-frontend-app.md index 1947ffb6..d81bb8da 100644 --- a/docs/how_tos/migrate-frontend-app.md +++ b/docs/how_tos/migrate-frontend-app.md @@ -749,12 +749,12 @@ const examplePageUrl = getUrlForRouteRole('example'); App-specific config values -------------------------- -App-specific configuration can be expressed by adding an `config` section to the app, allowing arbitrary variables: +App-specific configuration can be expressed by adding a `defaultConfig` section to the app, allowing arbitrary variables: ```js const app: App = { ... - config: { + defaultConfig: { myCustomVariableName: 'my custom variable value', }, }; @@ -768,6 +768,15 @@ getAppConfig('myapp').myCustomVariableName Or via `useAppConfig()` (with no need to specify the appId), if `CurrentAppProvider` is wrapping your app. +`getAppConfig` resolves three sources, in order of increasing precedence: the app's `defaultConfig`, the site's `commonAppConfig`, and the app's `config`. Bundle an app's own values in `defaultConfig`; `config` is where operators override them, in a site config file or via the runtime config API: + +```js +// In a site.config file +apps: [ + { ...app, config: { myCustomVariableName: 'operator value' } }, +], +``` + Complete examples ----------------- diff --git a/runtime/config/index.test.ts b/runtime/config/index.test.ts index 50b0ecbd..3160f50a 100644 --- a/runtime/config/index.test.ts +++ b/runtime/config/index.test.ts @@ -414,6 +414,115 @@ describe('mergeSiteConfig', () => { }); }); + describe('getAppConfig with defaultConfig', () => { + it('should return defaultConfig values when nothing else is set', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'defaults-only-app', defaultConfig: { BUNDLED: 'bundled-value' } }], + }); + addAppConfigs(); + + expect(getAppConfig('defaults-only-app')).toEqual({ BUNDLED: 'bundled-value' }); + }); + + it('should let commonAppConfig override defaultConfig', () => { + setSiteConfig({ + ...defaultSiteConfig, + commonAppConfig: { SHARED: 'common' }, + apps: [{ + appId: 'defaults-common-app', + defaultConfig: { SHARED: 'bundled', BUNDLED_ONLY: 'yes' }, + }], + }); + addAppConfigs(); + + expect(getAppConfig('defaults-common-app')).toEqual({ + SHARED: 'common', + BUNDLED_ONLY: 'yes', + }); + }); + + it('should resolve defaultConfig below commonAppConfig below config', () => { + setSiteConfig({ + ...defaultSiteConfig, + commonAppConfig: { SHARED: 'common', COMMON_AND_APP: 'common' }, + apps: [{ + appId: 'three-layer-app', + defaultConfig: { SHARED: 'bundled', BUNDLED_ONLY: 'yes' }, + config: { SHARED: 'app-specific', COMMON_AND_APP: 'app-specific' }, + }], + }); + addAppConfigs(); + + expect(getAppConfig('three-layer-app')).toEqual({ + SHARED: 'app-specific', + COMMON_AND_APP: 'app-specific', + BUNDLED_ONLY: 'yes', + }); + }); + + it('should deep merge all three layers', () => { + setSiteConfig({ + ...defaultSiteConfig, + commonAppConfig: { NESTED: { b: 'common', c: 'common' } }, + apps: [{ + appId: 'deep-merge-app', + defaultConfig: { NESTED: { a: 'bundled', b: 'bundled', c: 'bundled' } }, + config: { NESTED: { c: 'app-specific' } }, + }], + }); + addAppConfigs(); + + expect(getAppConfig('deep-merge-app')).toEqual({ + NESTED: { a: 'bundled', b: 'common', c: 'app-specific' }, + }); + }); + + it('should not let runtime config write into defaultConfig', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ + appId: 'runtime-override-app', + defaultConfig: { SHARED: 'bundled', BUNDLED_ONLY: 'yes' }, + }], + }); + addAppConfigs(); + + mergeSiteConfig( + { apps: [{ appId: 'runtime-override-app', config: { SHARED: 'runtime' } }] }, + { limitAppMergeToConfig: true } + ); + addAppConfigs(); + + expect(getSiteConfig().apps![0].defaultConfig).toEqual({ + SHARED: 'bundled', + BUNDLED_ONLY: 'yes', + }); + expect(getAppConfig('runtime-override-app')).toEqual({ + SHARED: 'runtime', + BUNDLED_ONLY: 'yes', + }); + }); + + it('should deep merge defaultConfig in a full app merge', () => { + setSiteConfig({ + ...defaultSiteConfig, + apps: [{ appId: 'full-merge-app', defaultConfig: { KEEP: 'yes', REPLACE: 'old' } }], + }); + + mergeSiteConfig({ + apps: [{ appId: 'full-merge-app', defaultConfig: { REPLACE: 'new', ADDED: 'yes' } }], + }); + addAppConfigs(); + + expect(getAppConfig('full-merge-app')).toEqual({ + KEEP: 'yes', + REPLACE: 'new', + ADDED: 'yes', + }); + }); + }); + describe('getProvides', () => { it('should return empty array when no apps exist', () => { setSiteConfig({ ...defaultSiteConfig, apps: [] }); diff --git a/runtime/config/index.ts b/runtime/config/index.ts index 0dbd4cfa..17784e9d 100644 --- a/runtime/config/index.ts +++ b/runtime/config/index.ts @@ -283,7 +283,8 @@ function mergeApps( /* * Merge a pair of Apps with the same appId. Deep-merges `config` (and, in the - * full-merge case, `provides`); other fields take `newApp`'s value verbatim. + * full-merge case, `defaultConfig` and `provides`); other fields take `newApp`'s + * value verbatim. * The result is built via `Object.getOwnPropertyDescriptors` so any lazy * getters survive: a snapshot via `lodash.merge` or spread would invoke the * getter at merge time and freeze its return value, which is typically empty @@ -296,8 +297,9 @@ function mergeApp( newApp: App, options: { configOnly?: boolean } = {}, ): App { - // configOnly mode: preserve `oldApp` (identity, slots, etc.) and deep-merge - // only `newApp.config` on top. + // configOnly mode: preserve `oldApp` (identity, slots, defaultConfig, etc.) + // and deep-merge only `newApp.config` on top. Operator-supplied config can + // never write into an app's bundled `defaultConfig`. if (options.configOnly) { if (!newApp.config) { return oldApp; @@ -307,9 +309,13 @@ function mergeApp( }); } - // Full mode: take `newApp` (identity, slots, etc.) and deep-merge `config` - // and `provides` from `oldApp`. Other fields take `newApp`'s value verbatim. + // Full mode: take `newApp` (identity, slots, etc.) and deep-merge + // `defaultConfig`, `config`, and `provides` from `oldApp`. Other fields take + // `newApp`'s value verbatim. const deepMerged: Record = {}; + if (oldApp.defaultConfig !== undefined || newApp.defaultConfig !== undefined) { + deepMerged.defaultConfig = merge({}, oldApp.defaultConfig, newApp.defaultConfig); + } if (oldApp.config !== undefined || newApp.config !== undefined) { deepMerged.config = merge({}, oldApp.config, newApp.config); } @@ -327,6 +333,10 @@ function cloneAppDescriptors(source: App, overrides: Record): A return Object.create(Object.getPrototypeOf(source), descriptors) as App; } +/* Bundled by app authors via `App.defaultConfig`. Kept separate from + `appConfigs` so that operator-supplied config can never write into it. */ +const appDefaultConfigs: Record = {}; + const appConfigs: Record = {}; /** @@ -339,7 +349,10 @@ export function addAppConfigs() { if (!apps) return; for (const app of apps) { - const { appId, config } = app; + const { appId, config, defaultConfig } = app; + if (defaultConfig !== undefined) { + appDefaultConfigs[appId] = defaultConfig; + } if (config !== undefined) { appConfigs[appId] = config; } @@ -348,12 +361,21 @@ export function addAppConfigs() { publish(CONFIG_CHANGED); } +/** + * Resolves an app's configuration, deep merging the three sources in order of + * increasing precedence: + * + * - `App.defaultConfig`, bundled by the app author + * - `SiteConfig.commonAppConfig`, supplied site-wide by an operator + * - `App.config`, supplied per-app by an operator + */ export function getAppConfig(id: string) { const { commonAppConfig } = getSiteConfig(); - if (commonAppConfig === undefined) { + const defaultConfig = appDefaultConfigs[id]; + if (defaultConfig === undefined && commonAppConfig === undefined) { return appConfigs[id]; } - return merge({}, commonAppConfig, appConfigs[id]); + return merge({}, defaultConfig, commonAppConfig, appConfigs[id]); } export function mergeAppConfig(id: string, newAppConfig: AppConfig) { diff --git a/types.ts b/types.ts index 55b245e4..4e81a595 100644 --- a/types.ts +++ b/types.ts @@ -29,6 +29,7 @@ export interface App { providers?: AppProvider[]; slots?: SlotOperation[]; externalScripts?: ExternalScriptLoaderClass[]; + defaultConfig?: AppConfig; config?: AppConfig; provides?: Record; }