feat: install npm private requirements during frontend build #1
Conversation
iamsobanjaved
commented
May 2, 2024
•
iamsobanjaved
commented
- Add facility to instal those requirements privately to MFEs which have only 2U-specific use-case.
- Added tooling to copy JS-based config file from edx-internal to MFE root, it will help us in adding support for Datadog without involving community.
tubular/scripts/frontend_utils.py
Outdated
| @@ -127,6 +153,17 @@ def create_version_file(self): | |||
| except IOError: | |||
| self.FAIL(1, 'Could not write to version file for app {}.'.format(self.app_name)) | |||
|
|
|||
| def copy_js_config_file_to_app_root(self, app_config, app_name): | |||
| """ Creates a copy of env.config.js(x) file to the app root """ | |||
| source = app_config['JS_CONFIG_FILEPATH'] | |||
There was a problem hiding this comment.
[suggestion] Should we only attempt to copy the JS_CONFIG_FILEPATH file if JS_CONFIG_FILEPATH exists? Otherwise, will this end up calling self.FAIL simply when an MFE does not define JS_CONFIG_FILEPATH?
We should ensure MFEs without JS_CONFIG_FILEPATH don't fail GoCD builds to be able to incrementally migrate MFEs.
tubular/scripts/frontend_utils.py
Outdated
| def copy_js_config_file_to_app_root(self, app_config, app_name): | ||
| """ Creates a copy of env.config.js(x) file to the app root """ | ||
| source = app_config['JS_CONFIG_FILEPATH'] | ||
| destination = f"{app_name}/{app_config['LOCAL_CONFIG_FILENAME']}" |
There was a problem hiding this comment.
Thanks for getting this work added in here. I'm wondering about this LOCAL_CONFIG_FILENAME field. I see it is being set to webpack.prod.config.js in the example. Does this mean the JS file will be copied to the webpack.prod.config.js file?
For JS file configuration to work properly, we would want this file to be copied to the root of the MFE. That being said, is there a desire/need to copy configuration into the webpack file?
There was a problem hiding this comment.
I agree. I think destination = self.app_name is the correct directory. We are not trying to customize webpack.prod.config.js during production.
tubular/tests/test_frontend_build.py
Outdated
| ] | ||
| assert mock_create_version.call_count == 1 | ||
| assert mock_shutil_copyfile.call_count == 1 | ||
| # Verify that source is correct and destination is rightly formatted | ||
| mock_shutil_copyfile.assert_called_with('dummy/file/path/env.config.js', 'coolfrontend/webpack.prod.config.js') |
There was a problem hiding this comment.
Adding onto Max's question above about LOCAL_CONFIG_FILENAME, the copied file should be named env.config.js within the destination path (i.e., root of the MFE). The copied file should not be the Webpack configuration.
| @@ -102,6 +120,14 @@ def get_npm_aliases_config(self): | |||
| self.LOG('No npm package aliases defined in config.') | |||
| return npm_aliases_config | |||
|
|
|||
| def get_npm_private_config(self): | |||
There was a problem hiding this comment.
this can be achieved with NPM_ALIASES. I don't think we need to add another implementation.
There was a problem hiding this comment.
FWIW, the difference between NPM_ALIASES and NPM_PRIVATE is that dependencies in NPM_PRIVATE are not aliases. If we relied on NPM_ALIASES, the syntax might be somewhat unnecessarily verbose/confusing to define an NPM alias for the same package name it already has.
NPM_ALIASES:
- "@datadog/browser-rum": "npm:@datadog/browser-rum@2"versus:
NPM_PRIVATE:
- "@datadog/browser-rum@2"
Also copy JS config file to app root in frontend build
iamsobanjaved
force-pushed
the
iamsobanjaved/npm-private
branch
from
93a867d
to
22ea9c4
Compare
|
Tests are green, coverage is failing due to rate-limiting issue of codecov. |
tubular/scripts/frontend_utils.py
Outdated
| def copy_js_config_file_to_app_root(self, app_config, app_name): | ||
| """ Creates a copy of env.config.js(x) file to the app root """ | ||
| source = app_config.get('JS_CONFIG_FILEPATH') | ||
| filename = app_config.get('LOCAL_CONFIG_FILENAME', "env.config.js") |
There was a problem hiding this comment.
[nit/suggestion] It might be worth renaming LOCAL_CONFIG_FILENAME to LOCAL_JS_CONFIG_FILENAME so the config setting is explicitly referencing the JS config (versus some other config).
tubular/scripts/frontend_utils.py
Outdated
| def copy_js_config_file_to_app_root(self, app_config, app_name): | ||
| """ Creates a copy of env.config.js(x) file to the app root """ | ||
| source = app_config.get('JS_CONFIG_FILEPATH') | ||
| filename = app_config.get('LOCAL_CONFIG_FILENAME', "env.config.js") |
There was a problem hiding this comment.
[sanity check] Will each individual MFEs be able to override the default LOCAL_JS_CONFIG_FILENAME of env.config.js, as needed?
For example, if an MFE relies on an env.stage.config.jsx file, the copied/replicated env.config file in the root of the MFE should also use the .jsx file extension. There would be a similar case if MFEs opted to use .ts or .tsx (TypeScript) extensions as well.
There was a problem hiding this comment.
Yes, they can override LOCAL_JS_CONFIG_FILENAME in their MFE's APP_CONFIG, also adjusted the test case to test overriding behaviour.
tubular/scripts/frontend_utils.py
Outdated
| # Skip if JS_CONFIG_FILEPATH not defined | ||
| if source: |
There was a problem hiding this comment.
nit: it might be slightly more readable/concise if you fail early when source is not defined, so the "happy path" logic doesn't get nested under a conditional block.
source = app_config.get('JS_CONFIG_FILEPATH')
# Skip if JS_CONFIG_FILEPATH not defined
if not source:
return
filename = app_config.get('LOCAL_CONFIG_FILENAME', "env.config.js")
destination = f"{app_name}/{filename}"
try:
shutil.copyfile(source, destination)
except FileNotFoundError:
self.FAIL(1, f"Could not find '{source}' for copying for app {app_name}.")
except OSError:
self.FAIL(1, f"Could not copy '{source}' to '{destination}', due to destination not writable.")
| if not source: | ||
| return | ||
|
|
||
| filename = app_config.get('LOCAL_JS_CONFIG_FILENAME', "env.config.js") |
There was a problem hiding this comment.
nit: is it worth having one extra check on line 161 if app_config.get('LOCAL_JS_CONFIG_FILENAME') does not exist? Though it should exist since you're defining it in a common location applicable to all MFEs, an MFE could override it to be an empty config settings.
