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.

Sign up for GitHub

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: reduce build time with cache #406

Closed
wants to merge 4 commits into from
Closed

Feat: reduce build time with cache #406

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
514a199
feat: reduce build time with cache
johnvente Jun 24, 2023
103788c
fix: remove unnecessary code
johnvente Jun 26, 2023
f4a5ebb
fix: enable feature since .env file and remove ignoring plugins for w…
johnvente Jun 27, 2023
28037f6
feat: adding ADR doc
johnvente Jun 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions lib/ConfigPreset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ function ConfigPreset({
return {
defaultFilename,
getDefault: () => require(require.resolve(`./${defaultFilename}`, { paths: [defaultDir] })),
getMfePath: () => searchFilepaths[0],
johnvente marked this conversation as resolved.
Show resolved Hide resolved
get defaultFilepath() {
console.log('getting default filepath', defaultFilename, defaultDir);
return require.resolve(`./${defaultFilename}`, { paths: [defaultDir] });
Expand Down
21 changes: 21 additions & 0 deletions lib/createConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,28 @@
const { merge } = require('webpack-merge');
const path = require('path');
const getBaseConfig = require('./getBaseConfig');
const presets = require('./presets');

module.exports = (commandName, configFragment = {}) => {
const baseConfig = getBaseConfig(commandName);

if (commandName === 'webpack-prod') {
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 curious on the decision to update the production webpack config here versus updating the default webpack.prod.config.js file with similar changes. As is, only consumers who rely on createConfig will benefit from the caching; consumers who rely on the default webpack.prod.config.js provided by @edx/frontend-build would not get the added benefit of caching.

Ideally, even consumers who don't explicitly define a webpack.prod.config.js file should also benefit from the added caching.

Copy link
Author

Choose a reason for hiding this comment

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

It's because some MFE's have webpack.prod.config.js if I make the change in that file I would have to rewrite that file per MFE. Maybe I could be wrong I'm not sure. For the MFE's that don't have implemented @edx/frontend-build are a quite different because they use a make file any advise for that?

Copy link
Member

Choose a reason for hiding this comment

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

The repositories that have a custom webpack.prod.config.js still rely on the base config defined in @edx/frontend-build's webpack.prod.config.js file here: https://github.com/openedx/frontend-build/blob/master/config/webpack.prod.config.js

For example, frontend-app-learner-portal-enterprise, doesn't specify a webpack.prod.config.js file; instead, it relies on the default Webpack configuration provided by @edx/frontend-build as shown here:

❯ npm run build

> [email protected] build
> fedx-scripts webpack

Running with resolved config:
/Users/astankiewicz/Desktop/edx/frontend-app-learner-portal-enterprise/node_modules/@edx/frontend-build/config/webpack.prod.config.js

The createConfig function used in those custom webpack.prod.config.js files extend/override the webpack.prod.config.js from @edx/frontend-build. If you change the Webpack configuration file itself, createConfig would still inherit the change unless consumers explicitly override/replace the updated cache configuration you're adding.

The benefit being that consumers of @edx/frontend-build that don't explicitly use createConfig (like frontend-app-learner-portal-enterprise) would still get the significant performance improvements from the cache addition.

For the MFE's that don't have implemented @edx/frontend-build are a quite different because they use a make file any advise for that?

[clarification] What MFEs in Open edX don't rely on @edx/frontend-build? @edx/frontend-build is the standard for Open edX MFE tooling & build scripts. Are you referring to non-MFE repositories that still rely on Webpack?

Copy link
Member

Choose a reason for hiding this comment

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

@johnvente Bump on this comment thread 😄 I still feel we should be modifying the base webpack.prod.config.js file, not the createConfig function.

Copy link
Member

Choose a reason for hiding this comment

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

@johnvente Just wanted to check in with you on this PR comment again. Thanks!

const cacheFolderPath = presets[commandName].getMfePath();
baseConfig.cache = {
johnvente marked this conversation as resolved.
Show resolved Hide resolved
type: 'filesystem',
cacheDirectory: path.resolve(cacheFolderPath, '.cache'),
johnvente marked this conversation as resolved.
Show resolved Hide resolved
};

const ignorePlugins = ['NewRelicPlugin', 'HtmlWebpackNewRelicPlugin', 'BundleAnalyzerPlugin'];
johnvente marked this conversation as resolved.
Show resolved Hide resolved
johnvente marked this conversation as resolved.
Show resolved Hide resolved

const { plugins } = baseConfig;

baseConfig.plugins = plugins.filter((plugin) => !ignorePlugins.includes(plugin.constructor.name));

baseConfig.devtool = false;
johnvente marked this conversation as resolved.
Show resolved Hide resolved

return merge(baseConfig, configFragment);
}

return merge(baseConfig, configFragment);
};