Skip to content
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-based configuration capabilities to Learner Record #275

Merged
merged 15 commits into from
Feb 12, 2024

Conversation

jsnwesson
Copy link
Contributor

@jsnwesson jsnwesson commented Feb 6, 2024

APER-2786

Description

This PR is to add JS-based configuration to Learner Record, which includes:

  • an example.env.config.js that includes details around how to set up a JS-based config
  • add a patch in node_modules to allow for:
    • webpack prod config will be able to accept a JS config during the build process
    • webpack dev config will be able to rely on a JS config (env.config.js) to declare the port number during local development, thus removing reliance on env.development
    • Jest config can now handle JS and JSX configurations
  • jest will be able to fetch environment variables from JS config, thus removing reliance on env.test
  • Had to add an override for sharp as @edx/frontend-build is using a version that fails in npm install. Until we are able to switch to @openedx/frontend-build -- which includes the older version of sharp that doesn't break during install, we will need this override.

Steps to run locally

  1. Copy/paste module.exports object from example.env.config.js into a new file env.config.js at the root directory
  2. Uncomment lines in webpack.dev.config.js to enable fetching the port number from env.config.js
  3. (Optional) delete .env.* files. These can exist alongside the JS-config without causing an error as long as we fetch from the ConfigDocument (done with getConfig) and not the process.env object.
  4. Run npm install
  5. Run npm run start
  6. Go to localhost:1990

Steps to run build locally

  1. Ensure env.config.js exists in root directory, but change NODE_ENV to production so webpack.prod.config.js is used for the build.
  2. Run npm run build
  3. Run npm run serve
  4. Go to localhost:1990

More on running production webpack locally

Steps to run with tubular and edx-internal

  1. Clone tubular into edx-repos directory: https://www.github.com/openedx/tubular
  2. Clone edx-internal into edx-repos directory: https://www.github.com/edx/edx-internal
    • Switch to jwesson/add-js-config-to-learner-record branch
  3. In terminal under the tubular directory:
    • python scripts/frontend_build.py --common-config-file ../edx-internal/frontends/common/stage_config.yml --env-config-file ../edx-internal/frontends/frontend-app-learner-record/test_config.yml --version-file ../frontend-app-learner-record/dist/version.json --app-name ../frontend-app-learner-record
    • Note the change in Learner Record's env.config.js, package.json, and the addition of a dist
  4. Run npm run serve in Learner Record
  5. Go to localhost:1990

Historical Context

Frontend Build ADR - JavaScript-based environment configuration
Frontend Platform ADR - Promote JavaScript file configuration and deprecate environment variable configuration

* webpack.dev.config.js will be able to use the port number provided in the env.config.js
* webpack.prod.config.js will be able to build Learner Record with a JS config passed in during build time
* Jest is now set up to retrieve environment variables from a JS config
* some tests needed to be fixed
@jsnwesson jsnwesson changed the title Add JS-based configuration capabilities to Learner Record feat!: Add JS-based configuration capabilities to Learner Record Feb 6, 2024
Copy link

codecov bot commented Feb 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ff97cc8) 70.22% compared to head (cec5f45) 70.22%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #275   +/-   ##
=======================================
  Coverage   70.22%   70.22%           
=======================================
  Files          27       27           
  Lines         403      403           
  Branches       85       85           
=======================================
  Hits          283      283           
  Misses        119      119           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -22,7 +22,7 @@ describe('program-certificates-list', () => {
it('renders the Program Certificates List', () => {
render(<ProgramCertificatesList />);

expect(screen.getByText('Verifiable Credentials')).toBeTruthy();
expect(screen.queryAllByText('Verifiable Credentials')).toBeTruthy();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests were failing locally because there were 2 instances in the DOM where 'Verifiable Credentials' existed, same with 'My Learner Records' in the following test file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's different about this branch that made those tests pass before and not here? Or had they always been failing locally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add onto the above question, how is the duplicate text rendered? Might make sense to verify with a screen.debug() to ensure it's not showing as duplicate headings/text to users.

Copy link
Contributor Author

@jsnwesson jsnwesson Feb 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TLDR;

A child component of these tested components was checking ENABLE_VERIFIABLE_CREDENTIALS in the ConfigDocument, which returned true due to the env.config.js existing in my branch. This led to the screen seeing My Learner Records and Verifiable Credentials twice, as noted in a screenshot below.

Even though the variables are an exact copy of .env.test, because process.env doesn't get initialized in these tests, the child component's check for ENABLE_VERIFIABLE_CREDENTIALS would be undefined.

Solution:

Mocking the child component (NavigationBar) in the tests, as I think it originally should have been.

Longer Read:

I found it! Alright, so in the .env.development and .env.test files, the variable ENABLE_VERIFIABLE_CREDENTIALS is set to 'true'.

When set to true, it will show something like the screenshot below:
Screenshot 2024-02-07 at 1 54 56 PM

But because the components are tested separately in Jest and the components -- and their children -- don't reference process.env.ENABLE_VERIFIABLE_CREDENTIALS, they can safely ignore the .env.development and .env.test boolean for ENABLE_VERIFIABLE_CREDENTIALS and will instead expect something like the following screenshot:
Screenshot 2024-02-07 at 1 54 51 PM
However, I did see that there's a child component within these tests that calls getConfig().ENABLE_VERIFIABLE_CREDENTIALS. This will return undefined because, again, the .env.* variables weren't initialized into the ConfigDocument as the component being tested doesn't go through the initialize process that occurs in the parent App.

Now, the reason why these tests had to be changed was because I have been using a env.config.js file in my repo, which is copy/paste of the values in env.development. The problem with that is the child component in these tests that calls getConfig().ENABLE_VERIFIABLE_CREDENTIALS will now pick up on the fact that ENABLE_VERIFIABLE_CREDENTIAL is true because the env.config.js has been merged into the config via the setupTest.js file that I modified.

import messages from './i18n';

jest.mock('@edx/frontend-platform/react/hooks', () => ({
...jest.requireActual('@edx/frontend-platform/react/hooks'),
useTrackColorSchemeChoice: jest.fn(),
}));

mergeConfig(envConfig);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jest doesn't rely on webpack to merge the JS-based config into ConfigDocument, so we have to implement it here. I might create a separate file that does this merge in the Jest directory if it makes more sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is reasonable to have in setupTest.js. I guess one question/suggestion might be whether it could be in the base setupTest.js upstream in @edx/frontend-build so that it'll be applicable to all consuming MFEs. I imagine other consuming MFEs of env.config would otherwise need to follow this same approach? Is there an opportunity to make it part of your patch and upstream contribution?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my understanding, frontend-platform is able to consume frontend-build, but frontend-build wouldn't be able to use frontend-platform's mergeConfig out of the box because Build isn't set up to import modules. Not to say it isn't a good idea, but I believe we'd need to convert everything in Build to use import instead of require so we can bring frontend-platform's getConfig in, right?

Copy link
Member

@adamstankiewicz adamstankiewicz Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good call out. I agree we can't do much here with setupTest upstream after all. Perhaps the action item then is possibly about documentation in your upstream @edx/frontend-build contribution and/or in @edx/frontend-platform to ensure consumers know to include this in their setupTest.js files if the intent is to have those settings come through in tests.

example.env.config.js Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
@@ -22,7 +22,7 @@ describe('program-certificates-list', () => {
it('renders the Program Certificates List', () => {
render(<ProgramCertificatesList />);

expect(screen.getByText('Verifiable Credentials')).toBeTruthy();
expect(screen.queryAllByText('Verifiable Credentials')).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's different about this branch that made those tests pass before and not here? Or had they always been failing locally?

webpack.dev.config.js Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
@@ -0,0 +1,12 @@
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[clarification] Similar to the other comment, I have a suspicion the jest/fallback.env.config.js used in the base jest.config.js provided by @(open)edx/frontend-build might be sufficient without having to duplicate into consuming projects?

// This file is used as a fallback to prevent build errors if an env.config.js file has not been
// defined in a consuming application.

I read this as though @(open)edx/frontend-build handles the fallback behind-the-scenes for consuming repositories such that the changes could be released without a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in your comment in the jest.config.js file, I'll very likely remove this for the PR because I agree that frontend-build should already provide this fallback.

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -22,7 +22,7 @@ describe('program-certificates-list', () => {
it('renders the Program Certificates List', () => {
render(<ProgramCertificatesList />);

expect(screen.getByText('Verifiable Credentials')).toBeTruthy();
expect(screen.queryAllByText('Verifiable Credentials')).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To add onto the above question, how is the duplicate text rendered? Might make sense to verify with a screen.debug() to ensure it's not showing as duplicate headings/text to users.

webpack.prod.config.js Outdated Show resolved Hide resolved
Comment on lines 20 to 23
config.resolve.modules = [
path.resolve(__dirname, './src'),
'node_modules',
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious/clarification] Do you mind expanding a bit on the intent for this config change?

Comment on lines 12 to 16
if (process.env.JS_CONFIG_FILEPATH) {
fs.copyFile(process.env.JS_CONFIG_FILEPATH, 'env.config.js', (err) => {
if (err) { throw err; }
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious, suggestion] I'm wondering if this condition could make sense upstream as part of the default base webpack.prod.config.js file returned by createConfig('webpack-prod'). For example, is the JS_CONFIG_FILEPATH environment variable approach something we could bake into the usage of env.config.js more formally for others in the Open edX community looking to migrate to env.config.js?

That said, perhaps that might be worth bringing as a discussion to Frontend Working Group. Nothing to act on in this PR directly :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion! I think if we end up going with this implementation, then we should definitely consider moving it upstream. For the sake of stating the obvious and making sure I know the why, similarly with the webpack.config.js issue I already made in frontend-build, this would mean that teams wouldn't have to create a custom webpack config with this logic or modify their already-existing configs per repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I removed the custom configurations in webpack prod/dev and tested them through the various permutations of keeping/removing .env.* and env.config.js, as well as with the local production build using Tubular. All worked as before, and the only things kept in those configs now are related to handling env.config.js
Thanks for bringing these up!

Comment on lines 3 to 4
/** Uncomment the lines below if you wish to use an env.config.js in development */
// const envConfig = require('./env.config');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[curious, suggestion] If you uncomment this without an env.config.js file defined in the repository, does it fail? Assuming not due to frontend-build's moduleNameMapper configuration. I believe if it's not defined, envConfig would fallback as an empty object {}.

If that is true, in the commented out line below, e.g., could you have it such that its not necessary to have commented out code here? For example, if you do want to use env.config.js as is and I uncomment this line, I wouldn't feel I can commit it upstream since not everyone might have an env.config.js, which adds some burden to local development to ensure I don't commit these changes.

Would config.devServer.port = envConfig?.PORT || process.env.PORT || 8080; work while keeping the import uncommented?

Copy link
Contributor Author

@jsnwesson jsnwesson Feb 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll fail on this line because it'll first try to find this file before it gets to line 6 (createConfig('webpack-dev'). Unless I add some condition before to check that the file exists, then I imagine it would need to remain commented like this (but I totally agree on the local dev burden to not commit these changes) or we escalate the frontend-build ticket to handle this PORT assignment for us. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes I see what you mean...

I do imagine we could get a ✅ fairly quickly if we opened a PR in frontend-build upstream and request a review in FWG, as the ideal path forward, IMHO.

That said, an alternative shorter term idea might be to (temporarily) utilize patch-package (docs) to make a patch to your installed copy of @edx/frontend-build that is committed and applied anytime npm install is run (including in CI/GoCD).

FWIW, we also had to utilize patch-package in an enterprise MFE semi-recently at one point, given we were blocked on upgrading to latest version of Paragon but needed a fix in one of its components for the currently installed version we were stuck on (see this ADR describing the rationale).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your suggestions, I moved the webpack logic for both prod and dev to a patch file, and I also started working on a branch in frontend-build to include these upgrades.
You'll also notice in the patch file that I modified the webpack and jest configs to be able to handle a JS or JSX extension of env.config, which I know we'll want to be able to have for upcoming work.

example.env.config.js Show resolved Hide resolved
Comment on lines 17 to 19
+const envConfigPath = fs.existsSync(appEnvConfigPathJs) ? appEnvConfigPathJs
+ : fs.existsSync(appEnvConfigPathJsx) ? appEnvConfigPathJsx
+ : path.resolve(__dirname, './jest/fallback.env.config.js');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sanity check] Will this __dirname will refer to the appropriate path when used in a .patch? Presumably the patch is brought into the node_modules during the postinstall so it should be OK; just wanted to see whether that's your understanding as well.

Copy link
Contributor Author

@jsnwesson jsnwesson Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the exact same understanding! However, since I applied your ternary suggestion, this __dirname, './jest/fallback.env.config.js' logic was removed from the patch and performs just as it did when first implemented in frontend-build :D

-if (fs.existsSync(appEnvConfigPath)) {
- envConfigPath = appEnvConfigPath;
-}
+const envConfigPath = fs.existsSync(appEnvConfigPathJs) ? appEnvConfigPathJs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if the breaking up the nested ternary conditionals would improve readability a bit?

let envConfigPath = path.resolve(__dirname, './jest/fallback.env.config.js');
const appEnvConfigPathJs = path.resolve(process.cwd(), './env.config.js');
const appEnvConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx');

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree!


+const envConfigPathJs = path.resolve(process.cwd(),'./env.config.js');
+const envConfigPathJsx = path.resolve(process.cwd(), './env.config.jsx');
+const envConfig = fs.existsSync(envConfigPathJs) ? require(envConfigPathJs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment/suggestion as above regarding nested ternary.

import messages from './i18n';

jest.mock('@edx/frontend-platform/react/hooks', () => ({
...jest.requireActual('@edx/frontend-platform/react/hooks'),
useTrackColorSchemeChoice: jest.fn(),
}));

mergeConfig(envConfig);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is reasonable to have in setupTest.js. I guess one question/suggestion might be whether it could be in the base setupTest.js upstream in @edx/frontend-build so that it'll be applicable to all consuming MFEs. I imagine other consuming MFEs of env.config would otherwise need to follow this same approach? Is there an opportunity to make it part of your patch and upstream contribution?

Comment on lines +81 to +83
"@edx/frontend-build": {
"sharp": "$sharp"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[inform] It looks like the upstream revert PR to get sharp back to 0.32.6 was merged upstream in latest version of @openedx/frontend-build. Likely still need to keep this override here, unless it's possible to migrate to the @openedx/frontend-build scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw! Glad it already got addressed upstream. I think for this PR we'll want to keep the override, and depending on how urgently we want to handle the @edx --> @openedx migration we'd be able to remove this override and the patches. Arguably, it'd be a good reason to do a follow-up ticket to migrate Learner Record and any other MFEs we want to experiment JS prod builds with.

@jsnwesson
Copy link
Contributor Author

@adamstankiewicz I am now realizing that the patch-package probably won't work for our production build because frontend-build is a devDependency. https://github.com/ds300/patch-package?tab=readme-ov-file#dev-only-patches

@adamstankiewicz
Copy link
Member

adamstankiewicz commented Feb 9, 2024

@adamstankiewicz I am now realizing that the patch-package probably won't work for our production build because frontend-build is a devDependency. https://github.com/ds300/patch-package?tab=readme-ov-file#dev-only-patches

Hmm, so typically patch-package is called within an NPM postinstall script (see docs), such that it applies any relevant patches whenever npm install or npm ci is executed (locally or in CI/CD). Given the @edx/frontend-build package is only intended for use at build time, I imagine patch-package may be warning about trying to patch something that's expected to be exposed in the published NPM package, which isn't the case here.

I think patch-package is still be worth a shot; we'd be looking for the following output during the build+deploy (GoCD) steps:

> @edx/[email protected] postinstall
> patch-package

patch-package 8.0.0
Applying patches...
@edx/[email protected]

We should still see the above output in GoCD (e.g. in the stage build) given it runs (or should be running) npm ci; the same command locally does seem to apply the patch 🤔

…ork in development

* since frontend-build is a devdependency, post-package will not include its patches in prod
@jsnwesson
Copy link
Contributor Author

@adamstankiewicz ah, my problem was that I forgot to add the postinstall script before i started testing the prod build, and I only added it afterward. Peace has been restored.
The only suggestion I wasn't able to immediately address was moving the setupTest.js changes to the base setupTest.js in frontend-build due to the import issue. I'll be asking my team for a review, but since you helped so much with this I'd appreciate if you could give your approval when you feel it's ready.

.gitignore Outdated
Comment on lines 8 to 9
env.config.js
env.config.jsx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be able to tackle both of these in 1 line?

Suggested change
env.config.js
env.config.jsx
env.config.js*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm learning something new every day haha. Thanks for the suggestion!

Comment on lines 8 to 9

+console.log('If you are expecting to use an env.config.jsx config, make sure to run `npm run postinstall` first!')
Copy link
Member

@adamstankiewicz adamstankiewicz Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[clarification] I'm assuming this console.log wouldn't be in the upstream contribution to @edx/frontend-build?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes to it not being added upstream although... I'm now realizing that the postinstall script should be able to handle this when anyone runs npm install. I'll remove it!

import messages from './i18n';

jest.mock('@edx/frontend-platform/react/hooks', () => ({
...jest.requireActual('@edx/frontend-platform/react/hooks'),
useTrackColorSchemeChoice: jest.fn(),
}));

mergeConfig(envConfig);
Copy link
Member

@adamstankiewicz adamstankiewicz Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good call out. I agree we can't do much here with setupTest upstream after all. Perhaps the action item then is possibly about documentation in your upstream @edx/frontend-build contribution and/or in @edx/frontend-platform to ensure consumers know to include this in their setupTest.js files if the intent is to have those settings come through in tests.

Copy link
Member

@MaxFrank13 MaxFrank13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jsnwesson jsnwesson merged commit fc17b47 into master Feb 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants