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

fix: make icons package tree-shakeable #2441

Merged
merged 1 commit into from
Jul 16, 2023

Conversation

viktorrusakov
Copy link
Contributor

@viktorrusakov viktorrusakov commented Jul 14, 2023

Description

Currently, all of the Paragon icons are included into the bundle if at least one icon is used in the project. This PR adds tree-shaking support to include only used icons into the bundle.

Previously, using SearchField component (that uses couple of icons internally) would result in following Paragon bundle, which included all of the icons
image

this PR reduced the bundle to this
Screenshot 2023-07-14 at 11 19 10

Deploy Preview

Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks
Copy link

Thanks for the pull request, @viktorrusakov! 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 Jul 14, 2023
@netlify
Copy link

netlify bot commented Jul 14, 2023

Deploy Preview for paragon-openedx ready!

Name Link
🔨 Latest commit c6bcc26
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/64b10c0b3f77700008d067d4
😎 Deploy Preview https://deploy-preview-2441--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@adamstankiewicz
Copy link
Member

This solution definitely helps with the resulting bundle's raw "stat" size; I believe it's still an open question on why the parsed/gzipped size doesn't seem to be changing for a test case that uses the Container and a handful of the Form.* sub-components, e.g.:

Without sideEffects: false

image

With sideEffects: false

image

I'd recommend we still merge/release this sideEffects: false change and update any JS libraries that include Paragon in its own bundle (e.g., frontend-component-header). It might still be worth digging into understanding why there's no change for parsed/gzipped sizes, though?

@viktorrusakov
Copy link
Contributor Author

@adamstankiewicz I'm pretty sure parsed and gzipped sizes don't change because webpack itself detects that most of the icons are not used and removes them during the minimization process. I've tested this with our example app that uses frontend-build's webpack config and looking at it I see that it uses default minimization plugin which defaults to terser-webpack-plugin, Terser in turn knows how to remove dead code from the bundle (judging by the fact that it has dead_code and side_effects options which are both set to true by default, meaning Terser will try to remove all unreachable code it can find).

Removing terser plugin (i.e. ... line) from webpack config results in different bundle sizes depending on sideEffects option

Without sideEffects
image

With sideEffects
image

Sooo I'm 99,9999% sure that currently even though all of the icons are present in the initial bundle of the MFEs that use Paragon, they are removed in the final bundle and don't cause bundle size / performance issues. I also validated it manually with our example app by checking resulting bundle manually and I couldn't find redundant icons in it even without sideEffects: false.

I guess this PR only ensures that icons don't get into they initial bundle, even though they would get removed by terser plugin anyway. Still this adds additional guarantee that we don't include all of the icons in the bundle 🙂.

@viktorrusakov viktorrusakov merged commit bcdbff9 into master Jul 16, 2023
9 checks passed
@viktorrusakov viktorrusakov deleted the rusakov/fix-treeshaking-with-icons branch July 16, 2023 09:02
@openedx-webhooks
Copy link

@viktorrusakov 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 20.45.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@adamstankiewicz
Copy link
Member

@viktorrusakov Thanks for doing a deeper dive in the bundle assets! Interesting find. So, it seems the output shown by Webpack Bundle Analyzer (i.e., report.html) may be a bit misleading by seemingly including all 2300+ icons, even though in reality Terser strips them? I wonder if there's a way to delay the BundleAnalyzerPlugin to run after Terser does its thing through minification (assuming it's generating the report before the minification). Food for thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U released
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants