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

Closed
wants to merge 4 commits into from

Conversation

johnvente
Copy link

@johnvente johnvente commented Jun 26, 2023

Most MFEs are using @edx/frontend-build to build their static files for both production and development configurations. However, this setup only works effectively for production because it ignores certain Webpack plugins that are unnecessary in a production environment.

This proposed change aims to enhance the build time for production by optimizing the Webpack cache configuration. It results in a significant improvement, with build times becoming more than 80% faster.

Issue related: overhangio/tutor-mfe#88

No cache
webpack-build-no-cache

Cached
webpack-build-cache

No cache Cached
ms seconds minutes ms seconds minutes
109849 109,849 1,83 3988 3,988 0,06

Note: We may need to add a volume in tutor to the .cache folder for each MFE and introduce an additional configuration to enable or disable this feature within tutor
Certainly! Here's the corrected version of the text:

Example of mfe authn previous to the change

mfe-auth-no-cached

How to test it:

  1. Clone the repository using the feat-build-time branch:

    git clone -b feat-build-time https://github.com/johnvente/frontend-build.git
    
  2. Install the necessary dependencies:

    npm install
    
  3. Clone a Micro Frontend (MFE) inside the frontend-build folder that builds its assets using @edx/frontend-build. This means that the webpack.prod.config.js file of the MFE should have the following configuration:

    const { createConfig } = require('@edx/frontend-build');

    Example MFE repositories with this requirement:

  4. Add this variable to .env file:

 ENABLE_WEBPACK_CACHE=''  
  1. Run npm install inside each cloned MFE.

  2. Update the webpack.prod.config.js file of the cloned MFE(s) to use the following import statement:

    const { createConfig } = require('../index');
  3. Run the build command:

    npm run build
    

    Note: The first time you run the build command, you may not notice any difference because the .cache folder has not been created yet.

  4. Run the build command again:

    npm run build
    

Please make sure to review and adapt the instructions based on your specific setup and requirements.

Note:

To implement this feature we need to add .cache folder to the mfe that will be using this and add the following to the .env file

 ENABLE_WEBPACK_CACHE=''

@openedx-webhooks
Copy link

openedx-webhooks commented Jun 26, 2023

Thanks for the pull request, @johnvente! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 26, 2023
@johnvente johnvente closed this Jun 26, 2023
@johnvente johnvente deleted the feat-build-time branch June 26, 2023 16:46
@johnvente johnvente restored the feat-build-time branch June 26, 2023 16:46
@johnvente johnvente reopened this Jun 26, 2023
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

Hi @johnvente! Thanks for the contribution. I left some clarifying questions and other comments. The added cache configuration to the production Webpack configuration definitely improves build times quite a bit! Very cool 🚀 🥇 Appreciate the performance benchmarks in the PR description.

Out of curiosity, is it possible and/or does it make sense to have similar caching when running MFEs with the Webpack Dev Server (i.e., webpack.dev.config.js)? 🤔

lib/createConfig.js Outdated Show resolved Hide resolved

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

> frontend-app-learner-portal-enterprise@0.1.0 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!

lib/createConfig.js Outdated Show resolved Hide resolved
lib/createConfig.js Outdated Show resolved Hide resolved
lib/ConfigPreset.js Outdated Show resolved Hide resolved
lib/createConfig.js Show resolved Hide resolved
lib/createConfig.js Show resolved Hide resolved
@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jun 27, 2023
@adamstankiewicz adamstankiewicz removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jun 27, 2023
@johnvente
Copy link
Author

Hi @johnvente! Thanks for the contribution. I left some clarifying questions and other comments. The added cache configuration to the production Webpack configuration definitely improves build times quite a bit! Very cool rocket 1st_place_medal Appreciate the performance benchmarks in the PR description.

Out of curiosity, is it possible and/or does it make sense to have similar caching when running MFEs with the Webpack Dev Server (i.e., webpack.dev.config.js)? thinking

I was thinking only for production environment but it could work for development environment as well. I think this feat doen't need to ignore those plugins

@adamstankiewicz
Copy link
Member

I was thinking only for production environment but it could work for development environment as well.

Got it, let's maybe defer on the development environment for now and treat that as a separate, future PR primarily to keep this PR's scope more narrow.

@johnvente johnvente force-pushed the feat-build-time branch 2 times, most recently from 158230d to f4a5ebb Compare June 27, 2023 20:57
@itsjeyd itsjeyd added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 5, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jul 5, 2023
@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jul 11, 2023
@itsjeyd
Copy link

itsjeyd commented Jul 18, 2023

Hey @johnvente, a friendly reminder to follow up on @adamstankiewicz's latest comment.

@itsjeyd
Copy link

itsjeyd commented Aug 15, 2023

Hi @johnvente, just checking in to see if you're planning to continue working on this PR?

@itsjeyd
Copy link

itsjeyd commented Sep 13, 2023

Hi @johnvente! Just a quick update that I'm closing this PR now because it is stale. You may reopen this pull request, or open a new one, when you have time to come back to this work. Thanks!

@itsjeyd itsjeyd closed this Sep 13, 2023
@openedx-webhooks
Copy link

@johnvente Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

1 similar comment
@openedx-webhooks
Copy link

@johnvente Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@itsjeyd itsjeyd added closed inactivity PR was closed because the author abandoned it and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed inactivity PR was closed because the author abandoned it open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants