feat: add JS config handling to webpack and jest configs #515

jsnwesson · 2024-02-14T18:50:33Z

This PR includes the following changes:

JS-based configuration is made available to Webpack Prod by passing in an environment variable
Webpack Dev config can now use the PORT assigned in env.config.js. Otherwise checks process.env.PORT or falls back to 8080.
Jest config now searches for either env.config.js or env.config.jsx for environment variables.
updated README to provide steps for using JS config in production and testing.

Resolves issues #513 and #514

* Jest config can now handle both JS and JSX env.configs * Webpack dev config will be able to assign devserver port based on the PORT in env.config.js

* Require env.config object after copying it into MFE

adamstankiewicz · 2024-03-02T01:51:26Z

config/jest.config.js

+const appEnvConfigPathJs = path.resolve(process.cwd(), './env.config.js');
+const appEnvConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx');


[curious] Is there a chance someone may want to use .ts or .tsx with the env.config file?

I would! ;)
Or at the least, let me put // @ts-check at the top of env.config.jsx to turn on VS Code's TypeScript checking, and let me write the configuration with auto-completion and type checking in the IDE.

can we use https://www.npmjs.com/package/glob to find the file with regex instead? It is easier to improve in the future.

adamstankiewicz · 2024-03-02T12:59:37Z

config/webpack.dev.config.js

+const envConfigPathJs = path.resolve(process.cwd(), './env.config.js');
+const envConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx');
+
+if (fs.existsSync(envConfigPathJs)) {
+  envConfig = require(envConfigPathJs);
+} else if (fs.existsSync(envConfigPathJsx)) {
+  envConfig = require(envConfigPathJsx);
+}


[suggestion] I wonder if it might make sense to create a helper/utility function (e.g., getEnvConfigPath) that can be used both here and in jest.config.js? E.g.

// jest.config.js const envConfigPath = getEnvConfigPath(); // returns nothing if `env.config` doesn't exist; otherwise, returns path to appropriate `env.config`. const envConfigPath = getEnvConfigPath(); if (envConfigPath) { envConfig = require(envConfigPath); }

// webpack.dev.config.js let envConfig = {}; // returns nothing if `env.config` doesn't exist; otherwise, returns path to appropriate `env.config`. const envConfigPath = getEnvConfigPath(); if (envConfigPath) { envConfig = require(envConfigPath); }

Related, I might also recommend adding support for envConfig within webpack.dev-stage.config.js file as well.

What Adam said.

Plus, we need to document somewhere in the README that the .js file takes precedence if both exist.

adamstankiewicz · 2024-03-02T13:01:24Z

config/webpack.prod.config.js

@@ -45,12 +65,12 @@ if (process.env.ENABLE_NEW_RELIC !== 'false') {
    agentID: process.env.NEW_RELIC_AGENT_ID || 'undefined_agent_id',
    trustKey: process.env.NEW_RELIC_TRUST_KEY || 'undefined_trust_key',
    licenseKey: process.env.NEW_RELIC_LICENSE_KEY || 'undefined_license_key',
-    applicationID: process.env.NEW_RELIC_APP_ID || 'undefined_application_id',
+    applicationID: envConfig.NEW_RELIC_APP_ID || process.env.NEW_RELIC_APP_ID || 'undefined_application_id',


[curious] Would we expect the other process.env.NEW_RELIC_* env vars might be usable with envConfig as well (e.g., agentID, trustKey, licenseKey, etc.)?

Are y'all looking into pluggifying the NEW_RELIC stuff, yet? It would be great for the project if we didn't have to deal with it so specifically.

I actually went ahead and removed these changes as this would've meant using a require statement in this config to fetch the environment variables, and that got in the way of using import in the env.config.js file.

This will mean that there's no clear solution yet for if we ever want to replace the process.env.ENV_VARs in this file or webpack.dev.config.js.

arbrandes

I wish I had a specific community use-case in mind for JS config beyond the Tubular usage at edx.org, but I can't object in principle: it offers significant additional flexibility, at least in theory. I also take it means it'll make pluggability more practical.

Nevertheless, we need to make sure it's not the primary use case, at least not until Tutor can make use of it.

arbrandes · 2024-03-04T17:20:35Z

README.md

+To use a private `env.config.js` file during the production build, the Webpack Production config will look for an env
+variable `process.env.JS_CONFIG_FILEPATH`, which should represent a file path to the desired `env.config.js`.
+
+The only requirement is that the filepath end with `env.config.*`, where either `.js` or `.jsx` as the extension


Suggested change

The only requirement is that the filepath end with `env.config.*`, where either `.js` or `.jsx` as the extension

The only requirement is that the filepath end with `env.config.*`, with either `.js` or `.jsx` as the extension.

arbrandes · 2024-03-04T17:25:45Z

README.md

+
+    JS_CONFIG_FILEPATH="{HOME}/frontends/frontend-app-profile/stage.env.config.jsx"
+
+## Requiring Jest to reference env.config.js


Does this mean that an env.config.js will now be required for tests to run successfully?

Maybe it already is and I just missed it.

Actually, env.config.js isn't required to run tests, but if there are any env variables or configuration in the env.config.js file that should be used for tests, this is what will be needed in the setupTest.js file.

arbrandes · 2024-03-04T17:28:17Z

README.md

@@ -218,6 +217,31 @@ locally. To serve a production build locally:
    attempt to run the build on the same port specified in the
    `env.config.js` file.

+## Creating a Production Build with env.config.js (using Tubular)


This is the only place Tubular is mentioned. I feel like we should either remove the reference entirely, or add a section that explains how to use it. I lean towards the former.

arbrandes · 2024-03-04T17:31:22Z

config/webpack.dev.config.js

+const envConfigPathJs = path.resolve(process.cwd(), './env.config.js');
+const envConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx');
+
+if (fs.existsSync(envConfigPathJs)) {
+  envConfig = require(envConfigPathJs);
+} else if (fs.existsSync(envConfigPathJsx)) {
+  envConfig = require(envConfigPathJsx);
+}


What Adam said.

Plus, we need to document somewhere in the README that the .js file takes precedence if both exist.

config/webpack.dev.config.js

config/webpack.prod.config.js

arbrandes · 2024-03-04T17:39:13Z

config/webpack.prod.config.js

@@ -45,12 +65,12 @@ if (process.env.ENABLE_NEW_RELIC !== 'false') {
    agentID: process.env.NEW_RELIC_AGENT_ID || 'undefined_agent_id',
    trustKey: process.env.NEW_RELIC_TRUST_KEY || 'undefined_trust_key',
    licenseKey: process.env.NEW_RELIC_LICENSE_KEY || 'undefined_license_key',
-    applicationID: process.env.NEW_RELIC_APP_ID || 'undefined_application_id',
+    applicationID: envConfig.NEW_RELIC_APP_ID || process.env.NEW_RELIC_APP_ID || 'undefined_application_id',


Are y'all looking into pluggifying the NEW_RELIC stuff, yet? It would be great for the project if we didn't have to deal with it so specifically.

* this is due to the fact that if any 'import' was desired in env.config it would fail due to the 'require' of env.config

brian-smith-tcril · 2024-03-19T18:28:13Z

config/jest.config.js

+if (fs.existsSync(appEnvConfigPathJs)) {
+  envConfigPath = appEnvConfigPathJs;
+} else if (fs.existsSync(appEnvConfigPathJsx)) {
+  envConfigPath = appEnvConfigPathJsx;


Is there a specific reason this logic prioritizes .js over .jsx? I don't have strong feelings either way, but the decision around the prioritization should be documented.

MaxFrank13 · 2024-04-22T16:46:42Z

Hello all! I have taken over this PR as Jason is out for the next couple weeks. I have read through comments/code and have some questions to help get me up to speed.

.ts(x) support and prioritization
- The recommendation to use glob seems straightforward but would introduce another dependency for frontend-build — are there any objections to adding this dependency?
- W.r.t prioritization, I'm having a hard time understanding why there would ever be more than one configuration file. That sounds like an implementation mistake seeing as we don’t have a mechanism for combining multiple config files in the first place. So anyone with two config files has presumably made an error in their setup, no? Regardless, I'll be happy to sort this out... once I'm less confused 😆
Same changes in jest config to go in dev and dev-stage configs
- Should we add the config changes to the other config files? If my understanding is correct, this would enable us to use the config file path rather than putting env values directly in the MFE’s env.config.{js/jsx/ts/tsx} file
- From what I gathered in the discussion, this was recommended as well as extracting the functionality into a utility to reduce code duplication. Just making sure I understand the suggestion before doing any work.
Need for specific community use case
- Is this still true post-adoption of FPF?
- In other words, is FPF a sufficient primary use case?
Clarification around jest.config and why it’s been modified
- Allows tests to access env.config values if needed — do we need specific examples of when this would be done?
- Do we have examples?

cc: @adamstankiewicz @arbrandes @jsnwesson @brian-smith-tcril @leangseu-edx

arbrandes · 2024-04-26T13:46:19Z

As discussed in the last FWG meeting, I don't see the point of adding an .env variable, here. We already have webpack configuration that pulls in env.config.js* if present. It shouldn't be frontend-build's responsibility to copy that file in.

MaxFrank13 · 2024-05-08T16:55:52Z

Closed as we aren't going forward with changes to the prod.config and don't see an immediate need for the jest.config changes. It may just need to confusion as we haven't fully migrated to JS configs yet.

jsnwesson added 6 commits March 1, 2024 20:47

feat!: enable webpack prod config to accept a JS-based config

07285fe

* Jest config can now handle both JS and JSX env.configs * Webpack dev config will be able to assign devserver port based on the PORT in env.config.js

refactor: expand ternary operators

a65980f

fix: missing require of 'fs' in webpack configs

9661ada

docs: add steps to use env.config in Jest tests

26de6b7

fix: handle scenario of no process.env value for New Relic plugins

a3cc567

* Require env.config object after copying it into MFE

chore: remove lint

d664423

jsnwesson force-pushed the jwesson/add-js-config-handling-to-webpack-jest branch from 5cab127 to d664423 Compare March 1, 2024 21:00

docs: add steps to implement JS config in production build

54e8a46

jsnwesson marked this pull request as ready for review March 1, 2024 22:35

adamstankiewicz reviewed Mar 2, 2024

View reviewed changes

arbrandes self-requested a review March 4, 2024 17:17

arbrandes requested changes Mar 4, 2024

View reviewed changes

jsnwesson added 2 commits March 14, 2024 21:34

fix: remove require statements from webpack prod and dev

0876775

* this is due to the fact that if any 'import' was desired in env.config it would fail due to the 'require' of env.config

docs: rephrase README and comment in webpack prod config

9acebc6

brian-smith-tcril reviewed Mar 19, 2024

View reviewed changes

leangseu-edx mentioned this pull request Apr 9, 2024

chore: patch frontend-build deployment openedx/frontend-app-learning#1350

Merged

arbrandes mentioned this pull request Apr 26, 2024

feat: add plugin slot to Footer using FPF openedx/frontend-app-learner-record#302

Merged

MaxFrank13 closed this May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add JS config handling to webpack and jest configs #515

feat: add JS config handling to webpack and jest configs #515

jsnwesson commented Feb 14, 2024 •

edited

Loading

adamstankiewicz Mar 2, 2024

bradenmacdonald Mar 19, 2024 •

edited

Loading

leangseu-edx Apr 3, 2024

adamstankiewicz Mar 2, 2024

arbrandes Mar 4, 2024

adamstankiewicz Mar 2, 2024

arbrandes Mar 4, 2024

jsnwesson Mar 14, 2024

arbrandes left a comment

arbrandes Mar 4, 2024

arbrandes Mar 4, 2024

jsnwesson Mar 14, 2024

arbrandes Mar 4, 2024

arbrandes Mar 4, 2024

arbrandes Mar 4, 2024

brian-smith-tcril Mar 19, 2024

MaxFrank13 commented Apr 22, 2024

arbrandes commented Apr 26, 2024

MaxFrank13 commented May 8, 2024

		const appEnvConfigPathJs = path.resolve(process.cwd(), './env.config.js');
		const appEnvConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx');

	The only requirement is that the filepath end with `env.config.*`, where either `.js` or `.jsx` as the extension
	The only requirement is that the filepath end with `env.config.*`, with either `.js` or `.jsx` as the extension.


		JS_CONFIG_FILEPATH="{HOME}/frontends/frontend-app-profile/stage.env.config.jsx"

		## Requiring Jest to reference env.config.js

feat: add JS config handling to webpack and jest configs #515

feat: add JS config handling to webpack and jest configs #515

Conversation

jsnwesson commented Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

bradenmacdonald Mar 19, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbrandes left a comment

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

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

MaxFrank13 commented Apr 22, 2024

arbrandes commented Apr 26, 2024

MaxFrank13 commented May 8, 2024

jsnwesson commented Feb 14, 2024 •

edited

Loading

bradenmacdonald Mar 19, 2024 •

edited

Loading