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

Upgrade Webpack from v4 to v5 #3091

Merged
merged 4 commits into from
Aug 26, 2024
Merged

Conversation

mudassir-hafeez
Copy link
Contributor

@mudassir-hafeez mudassir-hafeez commented Aug 7, 2024

What are the relevant tickets?

#5077

Description (What does it do?)

This PR upgrades dependencies related to webpack upgrade 5 to support python 3.12.x later.

Screenshots (if appropriate):

Before:
image

after:
image

How can this be tested?

  1. Run docker-compose up. Your watch container will automatically generate the Webpack bundles for you.
  2. Verify that the Webpack build is generated successfully with a webpack-stats.json file and that all xPro pages are working correctly.
  3. Also, verify/test with the production configuration if possible.

How to generate the bundles?

  1. Run docker-compose up. Your watch container will automatically generate the Webpack bundles for you.
  2. To verify Webpack bundles for production, go to the watch container's shell and manually run node node_modules/webpack/bin/webpack.js --config webpack.config.prod.js --bail. This will generate the production bundles for you.
  3. Use the report.html file to compare the differences visually.

Performance evalution:

  • Switch between the master and this branch for testing.
  • Generate the Webpack bundles on the master branch and note the bundle size.
  • Generate the Webpack bundles on this branch and note the bundle size again.

Additional Context

  • After upgrading to Webpack 5, the common bundle along with a new one will automatically get pulled in with an additional render_bundle in the appropriate views.

@odlbot odlbot temporarily deployed to xpro-ci-pr-3091 August 7, 2024 13:07 Inactive
@mudassir-hafeez mudassir-hafeez marked this pull request as ready for review August 7, 2024 16:36
@mudassir-hafeez mudassir-hafeez force-pushed the mudassir/webpack-upgrade branch from 1f13377 to cae1936 Compare August 8, 2024 12:00
@odlbot odlbot temporarily deployed to xpro-ci-pr-3091 August 8, 2024 12:00 Inactive
@mudassir-hafeez
Copy link
Contributor Author

mudassir-hafeez commented Aug 8, 2024

I've rebased/retested this branch as there was a conflict in a lock file after some PR's merged. It should be good to go with a review now.

@arslanashraf7
Copy link
Contributor

@mudassir-hafeez Could you rebase this PR?

@mudassir-hafeez mudassir-hafeez force-pushed the mudassir/webpack-upgrade branch from a01f856 to d1ded4e Compare August 16, 2024 09:39
@mudassir-hafeez
Copy link
Contributor Author

@mudassir-hafeez Could you rebase this PR?

I've retested some pages after the rebase, and it should be good to proceed.

Copy link
Contributor

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

The PR needs a rebase.

Other than that, The testing looks fine but I think there are a few issues with the generated bundles. Although this has stopped generating header-root bundle but its generating a new one with a random name which is not getting pulled on all the pages. For this I think we should combine all these in the header bundle to reduce confusion.

Also, The generated bundle size is more than what it was on master.

Also, The performance stats of all the pages have reduced by ~10%.

@mudassir-hafeez mudassir-hafeez force-pushed the mudassir/webpack-upgrade branch 2 times, most recently from 26be88a to 47eb339 Compare August 22, 2024 12:06
@mudassir-hafeez mudassir-hafeez force-pushed the mudassir/webpack-upgrade branch from 5f0a8f0 to 5632121 Compare August 23, 2024 12:59
@mudassir-hafeez
Copy link
Contributor Author

The PR needs a rebase.

This PR is rebased now.

Other than that, The testing looks fine but I think there are a few issues with the generated bundles. Although this has stopped generating header-root bundle but its generating a new one with a random name which is not getting pulled on all the pages. For this I think we should combine all these in the header bundle to reduce confusion.

Also, The generated bundle size is more than what it was on master.

Also, The performance stats of all the pages have reduced by ~10%.

I have made changes in this regard and combined assets into one, except those specifically mentioned in the entry for dev/prod config, similar to MITxOnline. And re-verified the generated bundle size for both the Webpack upgrade and master branches locally. The overall chunk size still appears slightly better on the Webpack upgrade branch compared to master. And the performance stats seem slightly equal what I discussed earliar, but yeah it has some fluctuation.

Copy link
Contributor

@arslanashraf7 arslanashraf7 left a comment

Choose a reason for hiding this comment

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

👍

@mudassir-hafeez mudassir-hafeez merged commit 6d33732 into master Aug 26, 2024
7 checks passed
@mudassir-hafeez mudassir-hafeez deleted the mudassir/webpack-upgrade branch August 26, 2024 13:12
@odlbot odlbot mentioned this pull request Aug 27, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants