-
Notifications
You must be signed in to change notification settings - Fork 34
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: add JS config handling to webpack and jest configs #515
Changes from 7 commits
07285fe
a65980f
9661ada
26de6b7
a3cc567
d664423
54e8a46
0876775
9acebc6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -129,7 +129,6 @@ if you need to do this and are running into problems. | |||||
|
||||||
## Local module configuration for Webpack | ||||||
|
||||||
|
||||||
The development webpack configuration allows engineers to create a | ||||||
\"module.config.js\" file containing local module overrides. This means | ||||||
that if you\'re developing a new feature in a shared library | ||||||
|
@@ -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) | ||||||
|
||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
// examples of acceptable filepaths | ||||||
|
||||||
JS_CONFIG_FILEPATH="{HOME}/frontends/frontend-app-learner-dashboard/prod.env.config.js" | ||||||
|
||||||
JS_CONFIG_FILEPATH="{HOME}/frontends/frontend-app-profile/stage.env.config.jsx" | ||||||
|
||||||
## Requiring Jest to reference env.config.js | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean that an Maybe it already is and I just missed it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, |
||||||
|
||||||
Jest doesn't rely on Webpack to merge the JS-based config into the Config Document, | ||||||
so to ensure Jest is aware of the environment variables in env.config, add the following to the MFE's `setupTest.js` | ||||||
|
||||||
import envConfig from '../env.config'; | ||||||
import mergeConfig from '@edx/frontend-platform'; | ||||||
|
||||||
... | ||||||
|
||||||
mergeConfig(envConfig); | ||||||
|
||||||
## Development | ||||||
|
||||||
This project leverages the command line interface for webpack, jest, | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,15 @@ const fs = require('fs'); | |
|
||
const presets = require('../lib/presets'); | ||
|
||
// This assigns the envConfigPath filepath based on whether env.config exists, otherwise it uses the fallback filepath. | ||
let envConfigPath = path.resolve(__dirname, './jest/fallback.env.config.js'); | ||
const appEnvConfigPath = path.resolve(process.cwd(), './env.config.js'); | ||
const appEnvConfigPathJs = path.resolve(process.cwd(), './env.config.js'); | ||
const appEnvConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx'); | ||
Comment on lines
+9
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [curious] Is there a chance someone may want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would! ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we use https://www.npmjs.com/package/glob to find the file with regex instead? It is easier to improve in the future. |
||
|
||
if (fs.existsSync(appEnvConfigPath)) { | ||
envConfigPath = appEnvConfigPath; | ||
if (fs.existsSync(appEnvConfigPathJs)) { | ||
envConfigPath = appEnvConfigPathJs; | ||
} else if (fs.existsSync(appEnvConfigPathJsx)) { | ||
envConfigPath = appEnvConfigPathJsx; | ||
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a specific reason this logic prioritizes |
||
} | ||
|
||
module.exports = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ const Dotenv = require('dotenv-webpack'); | |
const dotenv = require('dotenv'); | ||
const HtmlWebpackPlugin = require('html-webpack-plugin'); | ||
const path = require('path'); | ||
const fs = require('fs'); | ||
const PostCssAutoprefixerPlugin = require('autoprefixer'); | ||
const PostCssRTLCSS = require('postcss-rtlcss'); | ||
const PostCssCustomMediaCSS = require('postcss-custom-media'); | ||
|
@@ -17,6 +18,18 @@ const presets = require('../lib/presets'); | |
const resolvePrivateEnvConfig = require('../lib/resolvePrivateEnvConfig'); | ||
const getLocalAliases = require('./getLocalAliases'); | ||
|
||
// Provides the env.config object that is available in local development so that devserver port number | ||
// can be assigned below. If no env.config exists (JS or JSX), then it provides an empty object. | ||
let envConfig = {}; | ||
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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [suggestion] I wonder if it might make sense to create a helper/utility function (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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What Adam said. Plus, we need to document somewhere in the README that the |
||
|
||
// Add process env vars. Currently used only for setting the | ||
// server port and the publicPath | ||
dotenv.config({ | ||
|
@@ -174,7 +187,7 @@ module.exports = merge(commonConfig, { | |
// reloading. | ||
devServer: { | ||
host: '0.0.0.0', | ||
port: process.env.PORT || 8080, | ||
port: envConfig.PORT || process.env.PORT || 8080, | ||
jsnwesson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
historyApiFallback: { | ||
index: path.join(PUBLIC_PATH, 'index.html'), | ||
disableDotRule: true, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ const NewRelicSourceMapPlugin = require('@edx/new-relic-source-map-webpack-plugi | |
const HtmlWebpackPlugin = require('html-webpack-plugin'); | ||
const MiniCssExtractPlugin = require('mini-css-extract-plugin'); | ||
const path = require('path'); | ||
const fs = require('fs'); | ||
const PostCssAutoprefixerPlugin = require('autoprefixer'); | ||
const PostCssRTLCSS = require('postcss-rtlcss'); | ||
const PostCssCustomMediaCSS = require('postcss-custom-media'); | ||
|
@@ -23,6 +24,25 @@ const HtmlWebpackNewRelicPlugin = require('../lib/plugins/html-webpack-new-relic | |
const commonConfig = require('./webpack.common.config'); | ||
const presets = require('../lib/presets'); | ||
|
||
/** This condition confirms whether the configuration for the MFE has switched to a JS-based configuration | ||
* as previously implemented in frontend-build and frontend-platform. If the environment variable exists, then | ||
* an env.config.js file will be created at the root directory and its env variables can be accessed with getConfig(). | ||
jsnwesson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* https://github.com/openedx/frontend-build/blob/master/docs/0002-js-environment-config.md | ||
* https://github.com/openedx/frontend-platform/blob/master/docs/decisions/0007-javascript-file-configuration.rst | ||
*/ | ||
|
||
const envConfigPath = process.env.JS_CONFIG_FILEPATH; | ||
let envConfig = {}; | ||
|
||
if (envConfigPath) { | ||
const envConfigFilename = envConfigPath.slice(envConfigPath.indexOf('env.config')); | ||
fs.copyFileSync(envConfigPath, envConfigFilename); | ||
|
||
const newConfigFilepath = path.resolve(process.cwd(), envConfigFilename); | ||
envConfig = require(newConfigFilepath); | ||
} | ||
|
||
// Add process env vars. Currently used only for setting the PUBLIC_PATH. | ||
dotenv.config({ | ||
path: path.resolve(process.cwd(), '.env'), | ||
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [curious] Would we expect the other There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually went ahead and removed these changes as this would've meant using a This will mean that there's no clear solution yet for if we ever want to replace the |
||
})); | ||
extraPlugins.push(new NewRelicSourceMapPlugin({ | ||
applicationId: process.env.NEW_RELIC_APP_ID, | ||
applicationId: envConfig.NEW_RELIC_APP_ID || process.env.NEW_RELIC_APP_ID, | ||
apiKey: process.env.NEW_RELIC_ADMIN_KEY, | ||
staticAssetUrl: process.env.BASE_URL, | ||
staticAssetUrl: envConfig.BASE_URL || process.env.BASE_URL, | ||
// upload source maps in prod builds only | ||
noop: typeof process.env.NEW_RELIC_ADMIN_KEY === 'undefined', | ||
})); | ||
|
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.
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.