feat!: update runtime config support#157
Open
brian-smith-tcril wants to merge 8 commits intoopenedx:mainfrom
Open
feat!: update runtime config support#157brian-smith-tcril wants to merge 8 commits intoopenedx:mainfrom
brian-smith-tcril wants to merge 8 commits intoopenedx:mainfrom
Conversation
BREAKING CHANGE: rename `mfeConfigApiUrl` to `runtimeConfigJsonUrl` BREAKING CHANGE: remove `siteId` from config URL building add cache buster for development environments
3 tasks
- 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 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- 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 <noreply@anthropic.com>
Contributor
Author
|
Used Claude to update tests, found a few other things in the process. The Claude changes were PR'd on my fork. I left a comment on that PR with the log. brian-smith-tcril#1 |
- 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
brian-smith-tcril
force-pushed
the
runtime-config
branch
from
20ce407 to
d05935e
Compare
runtime/config/index.ts
Outdated
Comment on lines
196
to
202
| * 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 |
Contributor
Author
There was a problem hiding this comment.
better explain what appConfigOnly implies:
- still merge all non-app parts of the config
- only merge the "config" parts of each app
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: rename
mfeConfigApiUrltoruntimeConfigJsonUrlBREAKING CHANGE: remove
siteIdfrom config URL buildingadd cache buster for development environments