feat: JavaScript file configuration of apps #474

davidjoy · 2023-04-11T20:34:00Z

This adds the ability to configure an application via an env.config.js file rather than environment variables or .env files. This mechanism is much more flexible and powerful than environment variables as described in the associated ADR.

It also improves documentation around how to configure applications, providing more detail on the various methods.

Finally, it allows the logging, analytics, and auth services to be configured via the configuration document now that we can supply an alternate implementation in an env.config.js file. This allows configuration of these services without forking the MFE.

It doesn't remove the ability to supply a service implementation via the initialize() function's arguments since that would technically be a breaking change and it could conceivably be in use in forks today. At a later date we could choose to remove that mechanism.

fixes openedx/wg-frontend#11

Description:

Describe what this pull request changes, and why. Include implications for people using this change.

Merge checklist:

Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

Post merge:

After the build finishes for the merged commit, verify the new release has been pushed to NPM.

codecov · 2023-04-11T20:39:10Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.17 🎉

Comparison is base (6912ed5) 82.87% compared to head (84adafd) 83.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #474      +/-   ##
==========================================
+ Coverage   82.87%   83.05%   +0.17%     
==========================================
  Files          40       40              
  Lines        1051     1062      +11     
  Branches      189      194       +5     
==========================================
+ Hits          871      882      +11     
  Misses        168      168              
  Partials       12       12

Impacted Files	Coverage Δ
src/config.js	`54.54% <ø> (ø)`
src/initialize.js	`98.78% <100.00%> (+0.18%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

davidjoy · 2023-04-11T20:55:49Z

package.json

@@ -75,6 +75,7 @@
  },
  "peerDependencies": {
    "@edx/paragon": ">= 10.0.0 < 21.0.0",
+    "@edx/frontend-build": ">= 8.1.0",


This is the version of frontend-build that enables env.config as a magic import. So we didn't have a peer dependency here before, but we technically do now. It's unlikely to bite anyone since 8.1.0 was quite a while ago.

[curious/thinking aloud] Is there a way we could treat @edx/frontend-build as an optional dependency instead (docs)? In theory, someone may have created or could create an MFE using @edx/frontend-platform without @edx/frontend-build; it feels like introducing the peer dependency is potentially a breaking change as a result, even though I agree it's probably fair to assume this won't bite anyone.

I believe the challenge with treating @edx/frontend-build as an optionalDependency would be conditionally importing env.config.js and only using it, if the import was successful.

FWIW, I don't feel too strongly about this suggestion; just thinking of potential alternatives that don't require a coupling with @edx/frontend-build for the Webpack implementation should consumers of @edx/frontend-platform ever want to use a different implementation of Webpack than @edx/frontend-build 😃

For the record (Adam and I talked about this a bit offline) - the only way I could get env.config to work as an import was to use specialized webpack config (in frontend-build) to fallback to an empty import if the file couldn't be found. Since the entire mechanism is predicated on that behavior, and it's implemented in frontend-build, it feels a bit unavoidable to add the peer dependency. There's just not a way I've ever found to do this without that added magic (which was merged into frontend-build some time ago in openedx/frontend-build#200)

FWIW, we may have ended up needing to do something similar anyways given the direction this PR is headed to be able to fallback to a local Paragon version in the externally hosted Paragon CSS urls or if a externally hosted Paragon CSS urls fails to load, we need to expose the paths to the locally installed Paragon theme via frontend-build such that frontend-platform has access to them (otherwise, each MFE would have to pass args to initialize which feels like could be avoided).

src/initialize.js

davidjoy · 2023-04-11T20:57:23Z

src/initialize.js

 /*
 * Set or overrides configuration through an API.
 * This method allows runtime configuration.
 * Set a basic configuration when an error happen and allow initError and display the ErrorPage.
 */
-
-export async function runtimeConfig() {
+async function runtimeConfig() {


I took away the export on this line intentionally, as this is an internal function and nothing is consuming the export.

docs/decisions/0007-javascript-file-configuration.rst

kdmccormick · 2023-04-12T13:58:21Z

I do not have my head fully wrapped around this, but I trust the reviews of those that do.

With my Build-Test-Release hat on, though, the questions that the ADR leaves me with are: Does this jive well with runtime configuration, which is now required by the community release? And are you happy with how runtime config is implemented, now that there's a deprecation path for .env-based configuration?

docs/decisions/0007-javascript-file-configuration.rst

adamstankiewicz · 2023-04-12T13:56:23Z

src/initialize.js

+    const loggingServiceImpl = getConfig().loggingService || loggingService;
+    const analyticsServiceImpl = getConfig().analyticsService || analyticsService;
+    const authServiceImpl = getConfig().authService || authService;


src/initialize.js

adamstankiewicz · 2023-04-12T14:00:15Z

docs/decisions/0007-javascript-file-configuration.rst

+For the above reasons, we will deprecate environment variable configuration in favor of JavaScript
+file configuration.


We might want to keep the legacy environment variable configuration around and not deprecate it. There's not much reason to deprecate it that I can think of (e.g., not much maintenance overhead to support it), though it'll probably start to get used less with the introduction of env.config and the MFE runtime config API.

We could observe usage over time and, if we see that very few MFEs use the environment variable configuration mechanism then, deprecate at that time.

I hear you - the thing that comes to mind is that if we start using env.config.js so that we can pass real data types into the app instead of just strings, we'll presumably start removing the code that converts "false" to false, etc., in which case for given MFEs the old config won't work properly anymore. Unless we just make the logic more complicated to accommodate both. 😬 I guess this is a problem even in the interim before an official deprecation/removal. Hm - will have to think about this more.

On second thought, it may not be too much of a problem. The MFEs tend to have arguments in their calls to initialize where they pull config out of process.env if it exists. The new jsFileConfig function runs after that. The first can continue to massage strings into other data types, and the second doesn't have to worry about it. They can co-exist.

davidjoy · 2023-04-12T14:51:07Z

With my Build-Test-Release hat on, though, the questions that the ADR leaves me with are: Does this jive well with runtime configuration, which is now required by the community release? And are you happy with how runtime config is implemented, now that there's a deprecation path for .env-based configuration?

@kdmccormick So this is compatible with runtime config, and solves some use cases that that can't solve around extensibility and customization. In their load order, runtime config wins/is the last applied.

If I had to look in a crystal ball, I'd say we ultimately use runtime config for most configuration that JSON can handle, so to speak, and this becomes a mechanism for doing more complex config at build-time, maybe centered around customization of classes, functions, etc.

If/when modular MFEs become a thing it's possible all of this becomes moot and we re-invent the whole system, but I figure we'll cross that bridge when we come to it.

Between this and @adamstankiewicz 's comments, I'm open to backing off the stance in the ADR. But it does sort of suck to have to leave in a bunch of data massaging from strings to ints/bools/nulls/arrays/etc whenever something gets loaded via process.env.

kdmccormick · 2023-04-12T15:01:36Z

So this is compatible with runtime config, and solves some use cases that that can't solve around extensibility and customization. In their load order, runtime config wins/is the last applied.

@davidjoy Cool, thanks for the clarification! I recommend adding that note the ADR.

I'm open to backing off the stance in the ADR. But it does sort of suck to have to leave in a bunch of data massaging from strings to ints/bools/nulls/arrays/etc whenever something gets loaded via process.env.

Since Olive, the community-supported release requires runtime config (static config isn't even an option). Furthermore, if some setting values need to be changed between releases (eg "false" -> false), the tutor upgrade tool gives us a great automated way to make those changes happen. So, I fully support deprecating static config if you see it fit.

brian-smith-tcril

This looks really cool!

src/config.js

davidjoy · 2023-05-23T17:01:30Z

@kdmccormick and @adamstankiewicz - so I think after reading through everything here, I'm not going to back off the deprecation of environment variable config. I'm also not 100% sure I have the will to fully follow through on getting rid of it myself. I think this probably looks like filing a DEPR ticket with all the deets on how we get rid of it, yeah? Just trying to think through how to deliver on the intention in the ADR. 🤔

This adds the ability to configure an application via an env.config.js file rather than environment variables or .env files. This mechanism is much more flexible and powerful than environment variables as described in the associated ADR. It also improves documentation around how to configure applications, providing more detail on the various methods. Finally, it allows the logging, analytics, and auth services to be configured via the configuration document now that we can supply an alternate implementation in an env.config.js file. This allows configuration of these services without forking the MFE.

fixing a few typos, adding a section about the relationship between JS file config and runtime config, and cleaning up some formatting.

Based on PR feedback - be clearer about how the configuration methods overlap and what their best use cases are.

arbrandes

This is great! Thanks David!

kdmccormick · 2023-05-23T19:03:27Z

@davidjoy

I'm not going to back off the deprecation of environment variable config. I'm also not 100% sure I have the will to fully follow through on getting rid of it myself. I think this probably looks like filing a DEPR ticket with all the deets on how we get rid of it, yeah? Just trying to think through how to deliver on the intention in the ADR.

Sounds good! If you are able to write a DEPR ticket with removal steps, and then nudge it in the direction of actually happening (eg, emitting a warning when static config is used), then I think it's pretty likely someone in Frontend WG would eventually be able to drive the removal.

Turns out if you put each test in its own file, jest.mock can be used to change the implementation of env.config.js for each test file. This lets us test all the clauses in jsFileConfig. It also means we no longer need the actual env.config.js file, since we’re mocking it!

Previous commit removed this file, forgetting that it’s also necessary for the example app. Reinstated it here with a value specific to the example app so we can still prove the test suite works.

davidjoy · 2023-05-23T20:22:43Z

So I'm going to merge this first, then create the DEPR ticket if only so I can link to the ADR in this PR once it hits its final home on master.

adamstankiewicz · 2023-05-23T22:40:00Z

My only concern with the deprecation of the .env-based config approach is that it's the mechanism we use to support changing the default config in .env.development during local development within different settings that we don't want to end up committed, e.g. Algolia API keys/secrets. That is, the Git-ignored .env.private file.

In a world with only MFE runtime config and env.config.js, I believe we'd have to solely rely on the MFE runtime config API to create "private" config settings that we don't want to put into env.config.js to avoid the risk of getting it committed.

This becomes a developer experience issue in that I need to have the MFE runtime config API configured in edx-platform to be able to do any development with my private config setting in the MFE, whereas before we could just create a .env.private file to avoid needing to be coupled to the MFE runtime config API during development.

Is there a possibility we could support a env.config.private.js or similar to avoid this coupling during local development? This just came up in at least 2-3 conversations in the last 2 weeks where it was much easier to help someone create a private environment variable via .env.private versus needing to do a big knowledge share around setting up the MFE runtime config API locally.

adamstankiewicz · 2023-05-23T22:47:48Z

package.json

@@ -75,6 +75,7 @@
  },
  "peerDependencies": {
    "@edx/paragon": ">= 10.0.0 < 21.0.0",
+    "@edx/frontend-build": ">= 8.1.0",


FWIW, we may have ended up needing to do something similar anyways given the direction this PR is headed to be able to fallback to a local Paragon version in the externally hosted Paragon CSS urls or if a externally hosted Paragon CSS urls fails to load, we need to expose the paths to the locally installed Paragon theme via frontend-build such that frontend-platform has access to them (otherwise, each MFE would have to pass args to initialize which feels like could be avoided).

src/config.js

adamstankiewicz · 2023-05-23T22:50:18Z

src/initialize.js

+Note that the env.config.js file in frontend-platform's root directory is NOT used by the actual
+initialization code, it's just there for the test suite and example application.
+*/
+import envConfig from 'env.config'; // eslint-disable-line import/no-unresolved


[curious] Could we modify the default ESLint rules in @edx/frontend-build to disable the import/no-unresolved rule specifically for env.config? That way, we wouldn't need to disable the ESLint rule here. Given the file is exposed via @edx/frontend-build, it feels reasonable to have @edx/frontend-build make it pass ESLint, too.

Can you do that with import/no-unresolved? If so, sure, sounds like a nice piece of polish. 😄

@davidjoy See https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-unresolved.md#ignore.

brian-smith-tcril · 2023-05-24T18:04:59Z

Is there a possibility we could support a env.config.private.js or similar to avoid this coupling during local development?

I also think this is a good idea. Having a quick and easy way to override anything that's coming from runtime config when doing local dev is huge.

brian-smith-tcril

This is awesome! ❤️ how well documented everything is!

arbrandes · 2023-05-24T19:03:05Z

Picking out the relevant bit to reply to the whole of @adamstankiewicz's comment:

Is there a possibility we could support a env.config.private.js

I was assuming env.config.js - and for that matter, a potential env.config.development.js - wouldn't be committed at all except maybe as a template file. Something with a heading like "this is a sample file that your deployment environment should generate." This would make a private version unnecessary.

The only question I still have is about an .env.test replacement. Ideally, we wouldn't have one either. But it does add to the things that need to be set up/mocked in the code.

davidjoy · 2023-05-24T20:29:25Z

So this is up for debate, but I was assuming (and didn't explicitly say anywhere) that env.config.js would be as @arbrandes describes - it wouldn't be checked in, but there'd be an env.config.js.example file that is, and that file would probably contain a helpful comment and a reasonable development/local configuration. I think that solves the 'private' situation. env.config.js would be .gitignore'd.

As for testing, we could add something like this to the setupTest.js file:

jest.mock('env.config.js', () => ({
  BASE_URL: 'booyah',
  // etc.
}));

That would work, and not require us to have a test version of the env.config.js file.

The alternative might be to go back to frontend-build and try to make it do something more conditional based on NODE_ENV and look for env.config.prod.js, env.config.dev.js, env.config.test.js, env.config.private.js, but I'm not sure that's much better. The prod config wouldn't be useful for anyone (like .env now), the dev config will likely be different whether you're using Tutor or devstack and only useful for the latter, etc. Not sure that buys us much.

We have agreed we’d like to DEPR environment variable config, so I’m going to merge the ADR as “Accepted”

brian-smith-tcril · 2023-05-24T20:35:19Z

.gitignoreing .env.config.js sounds like it's probably the move

davidjoy · 2023-05-26T14:34:33Z

Thanks everyone for your input and smarts. 😄 Next up, a DEPR ticket and guidance on how to move forward.

Mashal-m · 2023-07-12T09:15:10Z

Hi @davidjoy! I am currently working on upgrading the frontend-app-learner-portal-programs to React 17. However, I am facing an issue when upgrading the frontend platform to version 4.5.0 (which includes this PR work). I doubt this issue is occurring because the MFE was created without utilizing a frontend build and is using Gatsby. I would like to hear your thoughts on this.
Here is my react 17 PR.

Attaching error screenshots below:

adamstankiewicz · 2023-07-12T12:07:48Z

@Mashal-m Note that @edx/frontend-platform now has a peer dependency with @edx/frontend-build, introduced through this PR. In the current state, it is assumed that @edx/frontend-build will be used alongside @edx/frontend-platform for MFE applications, which as you pointed out is not the case for frontend-app-learner-portal-programs.

See this comment thread for a bit more detail on this coupling with @edx/frontend-build: #474 (comment)

From the linked comment thread above:

it feels like introducing the peer dependency is potentially a breaking change as a result, even though I agree it's probably fair to assume this won't bite anyone.

Looks like our assumption at the time that introducing the coupling between @edx/frontend-platform and @edx/frontend-build wouldn't be a breaking change for any consumers was unfortunately inaccurate. /cc @davidjoy

Based on your screenshots, the specific issue is importing env.config.js within the Jest tests. See @edx/frontend-build's base jest.config.js file which configures Jest to handle env.config.js imports, for example here where env.config is added to moduleNameMapper. To resolve your repo's test issues, you might be able to update frontend-app-learner-portal-programs' jest.config.js file to include a similar config as @edx/frontend-build's.

If you're also running into issues when running/building frontend-app-learner-portal-programs, an additional path forward for you may be to update frontend-app-learner-portal-programs' onCreateWebpackConfig (source) to replicate what @edx/frontend-build does to create a Webpack alias import or, alternatively, possibly use a Gatsby plugin like gatsby-plugin-alias-imports (source):

// from https://github.com/openedx/frontend-build/blob/master/config/webpack.common.config.js
// (applied during `npm start` and `npm run build`)

{
  // ....
  resolve: {
    alias: {
      'env.config': path.resolve(process.cwd(), './env.config'),
    },
    fallback: {
      // This causes the system to return an empty object if it can't find an env.config.js file in
      // the application being built.
      'env.config': false,
    },
}

davidjoy · 2023-07-17T17:58:39Z

Oof, I think @adamstankiewicz covered it well, so I don't have much to add. Let us know how it goes.

davidjoy commented Apr 11, 2023

View reviewed changes

src/initialize.js Show resolved Hide resolved

davidjoy commented Apr 11, 2023

View reviewed changes

dcoa reviewed Apr 12, 2023

View reviewed changes

docs/decisions/0007-javascript-file-configuration.rst Outdated Show resolved Hide resolved

adamstankiewicz reviewed Apr 12, 2023

View reviewed changes

davidjoy force-pushed the davidjoy/feat_js_file_config branch from b684963 to 6d62612 Compare April 12, 2023 15:09

brian-smith-tcril reviewed Apr 14, 2023

View reviewed changes

src/config.js Outdated Show resolved Hide resolved

src/config.js Outdated Show resolved Hide resolved

src/config.js Show resolved Hide resolved

arbrandes mentioned this pull request May 17, 2023

Adolfo's reviews openedx/axim-engineering#696

Closed

davidjoy force-pushed the davidjoy/feat_js_file_config branch 3 times, most recently from 373c2f4 to 1c5b644 Compare May 23, 2023 16:57

davidjoy requested review from adamstankiewicz, brian-smith-tcril, kdmccormick and dcoa May 23, 2023 17:01

davidjoy and others added 4 commits May 23, 2023 13:59

docs: Update docs/decisions/0007-javascript-file-configuration.rst

a67784d

docs: JS file config ADR review feedback

bda306a

fixing a few typos, adding a section about the relationship between JS file config and runtime config, and cleaning up some formatting.

docs: improving documentation around when to use config methods

1458037

Based on PR feedback - be clearer about how the configuration methods overlap and what their best use cases are.

davidjoy force-pushed the davidjoy/feat_js_file_config branch from 1c5b644 to 1458037 Compare May 23, 2023 18:00

arbrandes approved these changes May 23, 2023

View reviewed changes

davidjoy added 2 commits May 23, 2023 15:50

test: we actually need the env.config.js file for the example app

e44f959

Previous commit removed this file, forgetting that it’s also necessary for the example app. Reinstated it here with a value specific to the example app so we can still prove the test suite works.

davidjoy force-pushed the davidjoy/feat_js_file_config branch from ff80c6c to e44f959 Compare May 23, 2023 20:07

adamstankiewicz approved these changes May 23, 2023

View reviewed changes

brian-smith-tcril approved these changes May 24, 2023

View reviewed changes

docs: accepting the js file config ADR

84adafd

We have agreed we’d like to DEPR environment variable config, so I’m going to merge the ADR as “Accepted”

davidjoy force-pushed the davidjoy/feat_js_file_config branch from 0c1df3b to 84adafd Compare May 24, 2023 20:31

arbrandes mentioned this pull request May 25, 2023

Support for JS-based config in frontend-platform openedx/wg-frontend#11

Closed

3 tasks

davidjoy merged commit b18d520 into openedx:master May 26, 2023

davidjoy deleted the davidjoy/feat_js_file_config branch May 26, 2023 14:34

Mashal-m mentioned this pull request Jul 13, 2023

build: update platform and implement env.config openedx/frontend-app-learner-portal-programs#281

Merged

Mashal-m mentioned this pull request Jul 31, 2023

Add frontend-build 12.9.0-alpha.1 in peer dep #529

Merged

4 tasks

davidjoy mentioned this pull request Jun 28, 2024

Remove peer dependency on frontend-build #713

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: JavaScript file configuration of apps #474

feat: JavaScript file configuration of apps #474

davidjoy commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading

davidjoy Apr 11, 2023 •

edited

Loading

adamstankiewicz Apr 12, 2023

davidjoy May 11, 2023

adamstankiewicz May 23, 2023 •

edited

Loading

davidjoy Apr 11, 2023

kdmccormick commented Apr 12, 2023

adamstankiewicz Apr 12, 2023

adamstankiewicz Apr 12, 2023

davidjoy Apr 12, 2023

davidjoy May 11, 2023

davidjoy commented Apr 12, 2023

kdmccormick commented Apr 12, 2023

brian-smith-tcril left a comment

davidjoy commented May 23, 2023

arbrandes left a comment

kdmccormick commented May 23, 2023

davidjoy commented May 23, 2023

adamstankiewicz commented May 23, 2023 •

edited

Loading

adamstankiewicz May 23, 2023 •

edited

Loading

adamstankiewicz May 23, 2023

davidjoy May 24, 2023

adamstankiewicz May 26, 2023

brian-smith-tcril commented May 24, 2023

brian-smith-tcril left a comment

arbrandes commented May 24, 2023 •

edited

Loading

davidjoy commented May 24, 2023

brian-smith-tcril commented May 24, 2023

davidjoy commented May 26, 2023

Mashal-m commented Jul 12, 2023

adamstankiewicz commented Jul 12, 2023

davidjoy commented Jul 17, 2023

		For the above reasons, we will deprecate environment variable configuration in favor of JavaScript
		file configuration.

feat: JavaScript file configuration of apps #474

feat: JavaScript file configuration of apps #474

Conversation

davidjoy commented Apr 11, 2023 • edited Loading

codecov bot commented Apr 11, 2023 • edited Loading

Codecov Report

davidjoy Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamstankiewicz May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdmccormick commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidjoy commented Apr 12, 2023

kdmccormick commented Apr 12, 2023

brian-smith-tcril left a comment

Choose a reason for hiding this comment

davidjoy commented May 23, 2023

arbrandes left a comment

Choose a reason for hiding this comment

kdmccormick commented May 23, 2023

davidjoy commented May 23, 2023

adamstankiewicz commented May 23, 2023 • edited Loading

adamstankiewicz May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-smith-tcril commented May 24, 2023

brian-smith-tcril left a comment

Choose a reason for hiding this comment

arbrandes commented May 24, 2023 • edited Loading

davidjoy commented May 24, 2023

brian-smith-tcril commented May 24, 2023

davidjoy commented May 26, 2023

Mashal-m commented Jul 12, 2023

adamstankiewicz commented Jul 12, 2023

davidjoy commented Jul 17, 2023

davidjoy commented Apr 11, 2023 •

edited

Loading

codecov bot commented Apr 11, 2023 •

edited

Loading

davidjoy Apr 11, 2023 •

edited

Loading

adamstankiewicz May 23, 2023 •

edited

Loading

adamstankiewicz commented May 23, 2023 •

edited

Loading

adamstankiewicz May 23, 2023 •

edited

Loading

arbrandes commented May 24, 2023 •

edited

Loading