-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: MFE configuration at runtime #335
Conversation
Thanks for the pull request, @dcoa! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
c94e3d3
to
c8a8fd9
Compare
@dcoa Thank you for your contribution. |
ae3e1d2
to
aaa87ef
Compare
Hi @natabene this PR is ready for review. |
Codecov Report
@@ Coverage Diff @@
## master #335 +/- ##
==========================================
+ Coverage 80.53% 81.24% +0.71%
==========================================
Files 38 38
Lines 904 917 +13
Branches 169 170 +1
==========================================
+ Hits 728 745 +17
+ Misses 164 160 -4
Partials 12 12
Continue to review full report at Codecov.
|
I think this should be on your radar @arbrandes |
@natabene could you help me re-run the test, please? |
Hi, @davidjoy @adamstankiewicz I'm working on changes to make it possible to read the response of the API to configure frontend applications at runtime, I hope you can check this pr and give us feedback 🙏 I have this openedx/frontend-app-account#603 too to make it possible to change the favicon and the site name in the title tag, could you check it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still looking and don't have a lot of time today, but wanted to give some early feedback! Will be able to look more closely on Monday.
src/initialize.js
Outdated
@@ -223,7 +252,7 @@ export async function initialize({ | |||
publish(APP_PUBSUB_INITIALIZED); | |||
|
|||
// Configuration | |||
await handlers.config(); | |||
if (getConfig().MFE_CONFIG_API_URL) { await runtimeConfig(); } else { await handlers.config(); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd suggest moving this if
statement logic into the default config handler. This logic here would continue to just say await handlers.config();
and we would optionally call the runtimeConfig
function if MFE_CONFIG_API_URL
exists. That way we enable folks to continue to override all of our default config handling.
Note that currently the config
handler is set to noOp
, so we'll just need to create a new one similar to auth
, analytics
, or initError
. Let me know if this doesn't make sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I consider this, but many applications have the override handler to marge additional values for that specific MFE (account example), which make even if you have MFE_CONFIG_API_URL don't call the API.
That's why I put it there because you can set that additional values with the API or set by env file.
May be I could move the if into runtimeConfig
and call like:
await handlers.config();
await runtimeConfig();
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, that makes sense. Yeah, how about the version you suggested. That way we preserve any overrides and then layer in the runtime config after. This sounds good:
await handlers.config();
await runtimeConfig();
src/initialize.js
Outdated
} catch (error) { | ||
// eslint-disable-next-line no-console | ||
console.error('Error with config API', error.message); | ||
setConfig({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should let the MFE keep its build-time configuration rather than using setConfig
here. I think it'd be more correct to leave all the build-time config values in there, rather than removing everything but BASE_URL. The way this is now, all the values will start returning undefined
instead of whatever's in the build-time config, which will likely be reasonable fallbacks in many cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I can remove the setConfig
and allow the use of default build-time values when the API fails.
@@ -67,6 +67,7 @@ let config = { | |||
LOGO_TRADEMARK_URL: process.env.LOGO_TRADEMARK_URL, | |||
LOGO_WHITE_URL: process.env.LOGO_WHITE_URL, | |||
FAVICON_URL: process.env.FAVICON_URL, | |||
MFE_CONFIG_API_URL: process.env.MFE_CONFIG_API_URL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this value needs to be added in a few more places. As a way to discover places it should appear, I tried searching for LOGO_TRADEMARK_URL
above and noted that config values in this file are also set in:
.env.test
.env.development
setupTest.js
Basically, it needs to be defaulted in those files for completeness sake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add the key without value by default because is optional :)
feat: MFE configuration at runtime (openedx#335)
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Only a single change related to the `LMS_BASE_URL` setting was required. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Only a single change related to the `LMS_BASE_URL` setting was required. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Only a single change related to the `LMS_BASE_URL` setting was required. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Only a single change related to the `LMS_BASE_URL` setting was required. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Only a single change related to the `LMS_BASE_URL` setting was required. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Only a single change related to the `LMS_BASE_URL` setting was required. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335
(This reintroduces the change in 9f84230 that was later reverted by 67b0b33.) frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
frontend-platform supports runtime configuration since 2.5.0 (see the PR that introduced it[1], but it requires MFE cooperation. This implements just that: by avoiding making configuration values constant, it should now be possible to change them after initialization. Almost all changes here relate to the `LMS_BASE_URL` setting, which in most places was treated as a constant. [1] openedx/frontend-platform#335 This is a backport to Olive of openedx#377
Description:
This draft PR serves as a discussion holder for the work of making mfes capable of being configured during runtime instead of during build.
This PR creates a new function to allow runtime configuration through an API call to the platform.
How to test
Note: You can combine buildtime and runtime configuration
Note: Related information FWG Issue
Merge checklist:
Post merge: