feat: add App.defaultConfig for bundled app config defaults - #298
Conversation
2f026d3 to
29158d8
Compare
|
I do like the idea of adding the new app default config as a way to separate the concerns of "app author wants to set a default for something" and "site operator wants to override a site default for one app." I'm now trying to think through the different configs, and it feels to me like we might be missing one:
I know we have non-app "common" defaults in site config, and maybe that's a better place for anything that would fall into the "???" category to live, but I figured it was worth raising. |
That doesn't exist at the level of frontend-base or apps. It's something for the site, and we already have a place for it: commonAppConfig. |
|
We do have non-app "common" defaults (in the optional section of frontend-base/runtime/config/index.ts Line 134 in 253812b What we don't have are common app defaults. I don't see not having those as something that should block this PR, and I don't see an immediate need for it, but I wanted to open a discussion about it. |
brian-smith-tcril
left a comment
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Couple notes on the ADR but overall this looks great!
defaultConfig now holds the author's values and resolves below commonAppConfig, which resolves below config; it is optional, so an app that doesn't set it resolves exactly as before. Co-Authored-By: Claude <noreply@anthropic.com>
29158d8 to
48ed704
Compare
|
🎉 This PR is included in version 2.0.0-alpha.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Every key an app declares in its own `config` is a key `commonAppConfig` can never supply, since app config is the highest-precedence layer. Authn declared twenty-nine, so values an operator set platform-wide through `MFE_CONFIG` silently lost to bundled defaults. Three survive as genuine defaults and move to `App.defaultConfig` (frontend-base ADR 0017, added by openedx/frontend-base#298), which resolves below `commonAppConfig`: `DISABLE_ENTERPRISE_LOGIN`, whose absence would quietly re-enable the enterprise flow, and `LOGO_URL` and `LOGO_WHITE_URL`, which carry the same default frontend-base ships for the shell header. Two of the rest were read nowhere at all, `FAVICON_URL` and `LOGO_TRADEMARK_URL`. Fifteen were no-ops against reads that already guard for absence. The remaining nine were placeholders whose reads did not guard, and those reads now do: `SEARCH_CATALOG_URL` was a latent `TypeError` in the embedded welcome-page flow; `INFO_EMAIL` produced `mailto:undefined`; the activation and password-reset support links rendered dead anchors; and the four `BANNER_IMAGE_*` keys asked the browser for `/undefined`. `MARKETING_SITE_BASE_URL` wrapped the logo in a link at ten near-identical sites and defaulted to a devstack host. Those collapse into one `BrandLogo`, which renders the logo unlinked when no marketing site is configured, rather than pointing it at the app's own origin. The peer dependency moves to `@openedx/frontend-base` 2.0.0-alpha.4, the first release carrying `App.defaultConfig`. Three test defects surfaced while auditing and are fixed here, since the audit is what makes the boolean contract real: `AccountActivationMessage.test.jsx` called `mergeAppConfig` without its `appId`, so its overrides never landed and the confirmation suite asserted the activation copy; boolean flags were overridden with env-era strings that strict `=== false` reads can never honour; and `SHOW_CONFIGURABLE_EDX_FIELDS` was set in four tests but read nowhere. Part of #1687. Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
Description
App.configheld two unrelated kinds of value: the defaults an app author bundles with the app, and the configuration an operator supplies throughsite.configor the runtime config API. Because both went through one field, every key an app bundled was a keycommonAppConfigcould never supply, and an operator overriding a single value had to spread the app's existing config back in or silently discard the rest.This adds an optional
App.defaultConfigfor the app author's values.getAppConfigresolves it belowcommonAppConfig, which resolves belowconfig. The two fields are held separately, so operator-supplied configuration, including anything arriving via the runtime config API, can never write into an app's bundled defaults.The field is additive: an app that does not set it resolves exactly as it does today. Nothing in this repository bundles app defaults, so there was nothing to migrate. The migration how-to now points app authors at
defaultConfig, and ADR 0017 records the decision.Closes #268.
LLM usage notice
Built with assistance from Claude.