feat: add App.defaultConfig for bundled app config defaults

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=<name>`` 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.

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
 -----------------
 

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: [] });

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<string, unknown> = {};
+  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<string, unknown>): 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<string, AppConfig> = {};
+
 const appConfigs: Record<string, AppConfig> = {};
 
 /**
@@ -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) {

types.ts

@@ -29,6 +29,7 @@ export interface App {
   providers?: AppProvider[];
   slots?: SlotOperation[];
   externalScripts?: ExternalScriptLoaderClass[];
+  defaultConfig?: AppConfig;
   config?: AppConfig;
   provides?: Record<string, unknown>;
 }