Patternfly-react: react-icons build optimization

[Hyperkid123]

Hello. We are building our docs page and we have serious issues with the react-icons bundle size.

We are using server side rendering and there is 8 chunks that all have additional 1.8mb bundle size caused just in @patternfly/react-icons. In our project we are using only 5 distinct icons, but the build includes all of them. We have tree shaking enabled but with the way how the icons are exposed it will never mark them as unreachable and it will not exclude them from the build.

We could theoretically mark each icon file in node modules as a side effect but that would mean marking each and every single icon file separately which is not really. It is much simpler to take these 5 svgs and store them in our code base and drop the dependency all together (that is what will do for now).

Here you have a screenshot of bundle sizes of every page created for SSR chunks:

Here is comparision of chunk sizes before (with react-icons) and after (without, moved svgs to code base)

Before

After

You can see that react-icons/dist/ems is in all of the biggest chunks. Each having 1.8 MB. That means that 8 of our SSR pages has over 2.2 MB of size which is not acceptable for SRR and completely defeats the purpose of this technology.

Can you please look at this issue? We are not able to use PF4 efficiently and we have to use different means (like duplicating components and having them in our code base).

[martinpovolny]

@dlabaj, @tlabaj : any updates?

Whom should we ping regarding this?

[karelhala]

This was first opened in https://github.com/patternfly/patternfly-react/issues/2533 without any resolution.

@rachael-phillips this has been observed from many teams, we see similiar issue in cloud services as well and our bundle is getting bigger with each icons bump. We were told that treeshaking will take care of unused icons. Turns out not to be the case and all icons are included multiple times if you create multiple bundles.

I would mark this as high priority, because it bloats our resources and either example how to properly bundle icons or some work that enables tree shaking.

[evwilkin]

@Hyperkid123 we were able to reproduce the issue and are seeing the full set of icons included in Dev builds as you described, but unused icons are excluded as expected through tree shaking in production build. Note that this is how webpack defaults the tree shaking:

• https://webpack.js.org/guides/tree-shaking/#minify-the-output

You can set optimization.sideEffects to true within your webpack.config.js to force the tree shaking in dev:

• https://webpack.js.org/configuration/optimization/#optimizationsideeffects
• https://github.com/webpack/webpack/tree/master/examples/side-effects

Using this optimization.sideEffects setting in our webpack config fixed the tree shaking discrepancy for us when building in dev:

[martinpovolny]

@evwilkin : I think that closing the issue is rather premature. I am pretty sure that @Hyperkid123 and @karelhala where experimenting with production build.

Can you share your demo/test project where things worked for you? To compare and to speed up the resolution...

[Hyperkid123]

@evwilkin Thanks for responding. What you are saying is true for any environment capable of using your esm dist version of icons. But I reported this issue specifically because of SSR(server side rendering) which runs on node which cannot use esm just yet.

optimization.sideEffects is set to true by default in webpack when used mode: production so it wont help on ssr either since the build requirements are a bit different.

You can simulate this issue even in client rendered app importing directly from dist/js or dist/umd.

import { CloseIcon, SearchIcon } from '@patternfly/react-icons/dist/umd'

And your bundle will bloat about 2MiB

I also discovered why is this happening. When you check the index.js in node_modules/@patternfly/react-icons/dist/umd/index.js you can see that every single icon is wrapped in generated immediately invoked function, which receives every single icon as an argument:

(function (global, factory) {
  if (typeof define === "function" && define.amd) {
    define(["exports", "./common", "./icons/ad-icon", "./icons/address-book-icon", "./icons/address-card-icon", "./icons/adjust-icon", "./icons/air-freshener-icon",...);
  } else {
    var mod = {
      exports: {}
    };
    factory(mod.exports, global.common, global.adIcon, global.addressBookIcon, global.addressCardIcon, global.adjustIcon, global.airFreshenerIcon,...);
    global.undefined = mod.exports;
  }
})(this, function (exports, _common, _adIcon, _alignCenterIcon,.....

This code is referenced as 1.js in webpack output log after production build. Look at the file size in logs below.

import React, { useState } from 'react';
import { CloseIcon, SearchIcon } from '@patternfly/react-icons/dist/umd'

const EmptyPage = () => {
    const [show, setShow] = useState(false)
    return (
        <div>
            Empty page
            <button onClick={() => setShow(show => !show)}>foo</button>
            {show ? <CloseIcon /> : <SearchIcon />}
        </div>
    )
}

export default EmptyPage;

Resulting bundle sizes are:

Hash: a564e2f48e38c8188253
Version: webpack 4.41.2
Time: 8219ms
Built at: 11/23/2019 5:24:07 PM
       Asset       Size  Chunks                    Chunk Names
./index.html  187 bytes          [emitted]
        1.js   2.12 MiB       1  [emitted]  [big]
        2.js  917 bytes       2  [emitted]
        3.js   2.87 KiB       3  [emitted]
     main.js    152 KiB       0  [emitted]         main
Entrypoint main = main.js
  [16] (webpack)/buildin/global.js 472 bytes {0} [built]
  [19] ./src/index.js + 11 modules 72 KiB {0} [built]
       | ./src/index.js 167 bytes [built]
       | ./src/app.js 1.03 KiB [built]
       |     + 10 hidden modules
[1685] ./src/empty-page.js 1.42 KiB {2} [built]
[1686] ./src/lazy-loaded-chunk.js + 3 modules 5.14 KiB {3} [built]
       | ./src/lazy-loaded-chunk.js 264 bytes [built]
       |     + 3 hidden modules
    + 3343 hidden modules

Webpack (or any other code bundler) cannot ever tree shake them since its actually being used. On top of that all these icons are added to global scope.

This is actually fixable directly in the node_modules file. If you delete the wrapping function and run build, the bundle size shrinks from 2.12Mib to 317Kib (which is still way to much for 2 icons but i just went a deleted some code from node modules.)

Hash: 86ee6bbbdee03b5adfb5
Version: webpack 4.41.2
Time: 6918ms
Built at: 11/23/2019 5:37:09 PM
       Asset       Size  Chunks                    Chunk Names
./index.html  187 bytes          [emitted]
        1.js    373 KiB       1  [emitted]  [big]
        2.js  915 bytes       2  [emitted]
        3.js   2.87 KiB       3  [emitted]
     main.js    152 KiB       0  [emitted]         main
Entrypoint main = main.js
[16] (webpack)/buildin/global.js 472 bytes {0} [built]
[19] ./src/index.js + 11 modules 72 KiB {0} [built]
     | ./src/index.js 167 bytes [built]
     | ./src/app.js 1.03 KiB [built]
     |     + 10 hidden modules
[22] ./src/empty-page.js 1.42 KiB {2} [built]
[23] ./src/lazy-loaded-chunk.js + 3 modules 5.14 KiB {3} [built]
     | ./src/lazy-loaded-chunk.js 264 bytes [built]
     |     + 3 hidden modules
    + 1680 hidden modules

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets:
  1.js (373 KiB)

How should the umd/commoj js build look like? I suggest looking at @material-ui/icons package because there is no issue with including all the icons. I will paste a sample which sets the exact pattern for the thousands of icons that this package provides:

"use strict";

var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefault");

Object.defineProperty(exports, "__esModule", {
  value: true
});
Object.defineProperty(exports, "AccessAlarm", {
  enumerable: true,
  get: function get() {
    return _AccessAlarm.default;
  }
});

var _AccessAlarm = _interopRequireDefault(require("./AccessAlarm"));

All other icons follow the same exact pattern. There is no factory which would cause all modules being bundled as one. Every single item has its own export. Icons are not even in dev build without forcing tree shaking.

By the way, the same is happening in react-core but it is not so prominent due to its smaller size.

Can you please take a second look and consider commonjs and umd environments as well?

I would also appreciate if we can have a bit more discussion in the future on issues like this before closing it immediately after first round of investigation.

[martinpovolny]

Thanks @Hyperkid123 for the detailed analysis.

[Hyperkid123]

@evwilkin I prepared repository which reproduces this issue: https://github.com/Hyperkid123/pf-icons-build

Feel free to play around with. You will see that cjs/umd packages will create large vendor chunks. Even with side effect "turned on" (its turned on by default in webpack in production mode), the result is the same.

[karelhala]

@evwilkin turning sideEffects on doesn't solve this issue. We are experiencing this in both webpack and parcel environment. If you want minimal light configuration you can reproduce this rather easilly by including any icon, you don't even need react-dom or anything and you'll be able to see it.

import React from 'react'
import { CloseIcon } from '@patternfly/react-icons';

export default () => (
  <div>
    <CloseIcon />
  </div>
);

And running npx parcel build index.js

To address this more deeply, we are not doing any strange things in our builds, we are using webpack with codesplitting all deps into vendor. You can check the config here https://github.com/RedHatInsights/frontend-components/blob/master/packages/config/src/config.js I don't think there's anything strange setup.

Based on @Hyperkid123 thorough investigation (kudos for that!), it looks like tree shaking is possible only for certain environment.

[evwilkin]

@Hyperkid123 @karelhala @martinpovolny thank you for the detailed feedback, we'll continue to look into this issue based on your comments and ensure a solution is working for you before closing out.

[evwilkin]

@Hyperkid123 thank you for sending the sample repo. @karelhala it does appear that tree shaking as a feature only works with ESM modules, so if you're unable to use ESM modules due to the SSR nature of your app then the alternate approach below is needed.

Material-UI and PatternFly have very similar icon implementations - you can see the side-by-side here:

@Hyperkid123 it looks like your project is importing each icon from Material separately, and as you mentioned the PatternFly icons are being hosted locally and imported individually:

In place of using ESM modules, you can utilize the same individual imports from PatternFly with CJS or UMD, just ensure that the import paths for each icon are pointing to that icon file within the directory you need (@patternfly/react-icons/dist/_{esm/js/umd}_/icons/_icon-name_) - note that material-ui exports each CJS icon file from @material-ui/icons with a separate directory at @material-ui/icons/esm for ESM modules, whereas PatternFly splits out each module option into directories within @patternfly/react-icons/dist:

I've created a PR for your sample repo here with the updated CJS and UMD paths:

• https://github.com/Hyperkid123/pf-icons-build/pull/1

@redallen also put in a PR for your project to help update these paths and avoid the react-icons unnecessary bloat:

• https://github.com/data-driven-forms/react-forms/pull/248

Should ESM modules become an option in this or another project then tree shaking should be a viable solution, but making these changes to the icon imports should be the best solution in the meantime.

[Hyperkid123]

@evwilkin

@Hyperkid123 it looks like your project is importing each icon from Material separately, and as you mentioned the PatternFly icons are being hosted locally and imported individually:

This is true but it works even when importing from index using import { Foo } from '@material-ui/core'

Thanks for the PR. I was aware of this option but here lies the problem. People will use import { x } from 'y' simply because they are used to it. The PR you created for data driven forms is appreciated but I would consider this a hot-fix. We might as well go and do the same for every single UI in cloud services for instance. The issue will still be a problem in any consumer app if they do not follow the same rule. We do not bundle PF. Its marked as external dependency.

There are two options I think.

• disable the index.js in non esm builds to prevent people from even using it since it will increase their builds and make it very clear in the docs (i don't like this one bit)
• or make sure that the build generates the index.js properly

By the way. The exact same issue applies to @patternfly/react-core.

Here is a example PR: https://github.com/Hyperkid123/pf-icons-build/pull/2

When importing from dist/umd/index.js will result in 5mb bundle size. I am generally not a lazy person but i really don't want to change all imports in around 6 cloud services repositories to which I am actively contributing.

I would prefer fixing this at the source.

No matter which way you will decide to go, i would definitely add very clear optimization section to the react docs so people are aware that they can reduce their bundles by a considerable amount.

[karelhala]

@evwilkin this looks more like a workaround instead of a solution. Can we look at proper solution to this? We are using quite a lot of PF components in approximately 30 applications (speaking of cloud-services) there's no time on our hands to go trough all of them and using this approach. I hope that PF team will come with proper solution to this so eveybody in future can use seamlessly this library.

If we'd were to look at material design (linked by Martin) I am not experiencing such issues might want to be good to go trough how they are doing it so we can replicate it here as well.

[martinpovolny]

If we'd were to look at material design (linked by Martin) I am not experiencing such issues might want to be good to go trough how they are doing it so we can replicate it here as well.

Can PF use the same approach? Is there a reason not to?

[karelhala]

Another thing to note - parcel is using ESM modules, but tree shaking is slightly different there so it will not remove these components as well. I don't think locking consumers to webpack in specific configuration is a good approach.

[redallen]

Can you share a minimal reproduction where importing from _the same module type_ of @material-ui/icons (or whatever icons package) gets tree shaken properly by a bundler but doesn't using @patternfly/react-icons?

I'm unable to reproduce this using react-renderer-demo inside https://github.com/data-driven-forms/react-forms using next or rollup or using the minimal case of https://github.com/Hyperkid123/pf-icons-build. In fact, @material-ui/icons looks worse than @patternfly/react-icons in the minimal reproduction of pf-icons-build!

Using the ESM version of import { AccessAlarm } from '@material-ui/icons'; translates into 160KB for one icon:

Whereas the ESM version of import { CloseIcon } from '@patternfly/react-icons'; is a more reasonable 3.7KB:

And using the CJS version of @material-ui/icons/index.js also bloats the bundle, adding 5MB+:

And yes, indeed, using @patternfly/react-icons/dist/js/index.js does add 3MB just like you reported:

Our @patternfly/react-icons/dist/js/index.js is a Babel'ed file:

"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
Object.defineProperty(exports, "IconSize", {
  enumerable: true,
  get: function get() {
    return _common.IconSize;
  }
});
Object.defineProperty(exports, "AdIcon", {
  enumerable: true,
  get: function get() {
    return _adIcon["default"];
  }
});
Object.defineProperty(exports, "AdIconConfig", {
  enumerable: true,
  get: function get() {
    return _adIcon.AdIconConfig;
  }
});

Just like Google's @material-ui/icons/index.js:

"use strict";

var _interopRequireDefault = require("@babel/runtime/helpers/interopRequireDefault");

Object.defineProperty(exports, "__esModule", {
  value: true
});
Object.defineProperty(exports, "AccessAlarm", {
  enumerable: true,
  get: function get() {
    return _AccessAlarm.default;
  }
});

The only noticeable difference seems to be how the _interopRequireDefault shim is defined. Our package.json files are similar with what they contain for bundlers:

@patternfly/react-icons

  "main": "dist/js/index.js",
  "module": "dist/esm/index.js",
  "types": "dist/js/index.d.ts",
  "sideEffects": false,

@material-ui/icons

  "main": "./index.js",
  "module": "./esm/index.js",
  "typings": "./index.d.ts"
  "sideEffects": false,

This leaves me with the question: how is our package different from @material-ui/icons? The only obvious thing is they include all the CJS under the root directory instead of putting it under /dist/js like we do, but this should not impact bundle size.

[Hyperkid123]

@redallen there is an official guide how to do it for mui: https://material-ui.com/guides/minimizing-bundle-size/#minimizing-bundle-size

I might have a couple of minutes later today to show it. Or tomorrow morning.

[Hyperkid123]

@evwilkin @redallen

I have used the MUI guide to PF and results are great check out this PR: https://github.com/Hyperkid123/pf-icons-build/pull/3 to see the code changes.

$ webpack --mode production
Webpack Bundle Analyzer is started at http://127.0.0.1:8888
Use Ctrl+C to close it
Hash: 7cbdeb72078e2dd9b8be
Version: webpack 4.41.2
Time: 1257ms
Built at: 11/27/2019 5:30:14 PM
                      Asset       Size  Chunks             Chunk Names
               ./index.html  159 bytes          [emitted]  
        cjs-chunk.bundle.js   5.89 KiB       0  [emitted]  cjs-chunk
             main.bundle.js    130 KiB       1  [emitted]  main
        mui-chunk.bundle.js  214 bytes       2  [emitted]  mui-chunk
vendors~mui-chunk.bundle.js   55.4 KiB       3  [emitted]  vendors~mui-chunk
Entrypoint main = main.bundle.js
 [3] ./src/index.js 757 bytes {1} [built]
 [8] ./src/cjs.js 266 bytes {0} [built]
 [9] ./src/mui.js 225 bytes {2} [built]
[21] ./node_modules/@material-ui/icons/esm/Close.js + 61 modules 162 KiB {3} [built]
     |    62 modules
    + 46 hidden modules
Child html-webpack-plugin for "index.html":
     1 asset
    Entrypoint undefined = ./index.html
    [0] ./node_modules/html-webpack-plugin/lib/loader.js!./src/index.html 301 bytes {0} [built]
    [2] (webpack)/buildin/global.js 472 bytes {0} [built]
    [3] (webpack)/buildin/module.js 497 bytes {0} [built]
        + 1 hidden module

[karelhala]

Thank you @Hyperkid123! I tried your approach and went from ~7MB to >3MB, now I can't see any react-icons and it looks really good. I've also tried this with react-core, but failed because all components are not bundled into one folder. Can we also please bundle all component and layouts to one folder so this approach will work?

[karelhala]

I would expect from this issue that this approach would be documented in components/icons readme so consumers know what to do and how to set their build process.

On other hand we still need a bit change for react-core components build. @redallen @evwilkin do you want me to open new issue to adress how components are exported? To fully enable tree shaking and easy adoption of this approach we need the build folder to look like

/dist/${esm|js|umd}/${componentName|layoutName}.js

So basically a flat structure of each component and layout (one file per component or layout without extra named exports). It's rather critical issue for us and it will help cloud-services a lot! It will shave off like half of our dependencies and make our builds much faster.

[evwilkin]

@Hyperkid123 thank you for linking the Material UI documentation and the results you found from implementing these changes - it's great to see that this did bring down the bundle size while still using the original import paths you had in place.

@karelhala if you can go ahead and create an issue for react-core that would be great, we may need more discussion around that file structure change to ensure that it is not breaking any existing import paths for users. Thanks for giving some context on how this will help out cloud-services

[Hyperkid123]

@evwilkin can we add the babel config snippet to PF icons docs? All examples you have there now are using the shorthand imports. I would expect that anybody using icons right now will be needing that if they want to improve their bundle sizes.

[evwilkin]

@Hyperkid123 working up something now for our current sprint, thanks for staying on this!

[evwilkin]

@Hyperkid123 we have a PR up to add this documentation here:
https://github.com/patternfly/patternfly-react/pull/3410

Note that we're suggesting using the 2nd plugin mentioned in the link you shared above - this solution is a bit more flexible and allows for a similar solution when using CJS modules within react-core as well.

[Hyperkid123]

@evwilkin thanks i will go trough it and try it out on some project to see the results. But looking at the config example I believe it should work. Will give you a status update in a bit.

[Hyperkid123]

@evwilkin so i started playing with the configs and here are my notes.

1. The config you showed will not work in 100% of use cases. Take a ActionGroup component.

The resolved path is @patternfly/react-core/dist/js/components/ActionGroup/ActionGroup.js but it should be node_modules/@patternfly/react-core/dist/js/components/Form/ActionGroup.js

1. If you have an external configured in your app for react-core or icons you have to create new external config. If you are using rollup for instance, it looks something likes this:

import { createFilter } from 'rollup-pluginutils';

const pf4Externals = createFilter([
  'react',
  'react-dom',
  'prop-types',
  '@data-driven-forms/react-form-renderer',
  '@patternfly/react-core/**',
  '@patternfly/react-icons/**',
], null, { resolve: false });

1. If you want to apply this config you also have to mark icons in react-core build as externals because its bundle with it. See picture bellow. Even if we use icons from JS build it still includes ESM ones from PF react core into our bundles. Or use direct paths in react core for icons to avoid this.

[evwilkin]

@Hyperkid123 thank you for the feedback and detailed notes - I'm currently looking at a potential solution for your example of the ActionGroup component & other components with different parent directories, and am working to build on what's in place to adapt for these file paths. I'll need to look into your externals example a bit more, but will post back here with what we find.

[evwilkin]

@Hyperkid123 are you able to provide steps to reproduce these issues in the react-forms project you're working in? We've been working on a solution and are hoping to be able to test in your environment and foresee any other issues/solutions as well.

[Hyperkid123]

@evwilkin the easiest way would be to clone it and play with the configs there. But steps to reproduce are fairly easy.

1. Create a library that uses PF react-core and icons but mark the as externals so they won't be in the library build.

2. Create some app that consumes the first one, add pf dependencies to it and use some. then run build.

You can try it by cloning https://github.com/data-driven-forms/react-forms. It is a monorepo where pf4-component-mapper is an example of the first library with pf as an external dependency and the package react-renderer-demo is the SSR package that consumes pf4-component-mapper.

Granted this repo is a bit overkill for testing so I will probably create some which is much more basic but should have the same issues.

[Hyperkid123]

@evwilkin I created simplest example possible which has the issue here:

https://github.com/Hyperkid123/pf-core-icons-deps

Steps to reproduce are in the readme

[evwilkin]

@Hyperkid123 thanks for the quick feedback, and for explaining the monorepo that you're working in.

As we look into these projects can you confirm if this is blocking, and if it is what is your timeframe around this issue? Thanks again for all the collaboration with us to get this figured out.

[Hyperkid123]

@evwilkin well this is not just problem with data driven forms. I am using it as an example because it is my project and I have full control over it and test all the changes that we came up with.

This issue mostly hinders cloud services projects. Every single UI which uses https://github.com/RedHatInsights/frontend-components (i believe that almost 100% of UI bundles depends on it) is affected by the same problems. Data driven forms are also being adopted quite a lot and are even a part of the frontend-components repo.

From my experience and based on the scope of these bundles, their size should be around 500kb, 1mb in some specific cases. But because of these duplications they are now somewhere around 5mb. Considering that the platform is not SPA and the bundle sizes, the time to first render is considerably longer than they should be, even on networks with high bandwidth. That negatively affects the whole UX on the whole platform.

So is this blocking us? Well not really. But personally I would say that it is quite critical.

If you don't have the resources to tackle this, I am willing to help. I mean changing the imports of icons in react-core repo is not exactly rocket science, it just takes some time. I don't want to force any major changes like starting using rollup for your builds etc, but I would like to resolve this ASAP.

[evwilkin]

Thanks for that context @Hyperkid123 - to catch you up, we'd been working on a few different approaches to getting the imports working using different Babel plugins while keeping the file structure as-is, in order to fix the issue you're seeing but also not break imports for other users or make large variations to the file structure that's in place.

A few solutions got us close but not quite there for all components, so we're looking at other options now and will update you as this progresses - thank you again for the valuable feedback.

[Hyperkid123]

@evwilkin I tried to apply the transform plugin to the react-core repo to use full imports on react icons and the results are great:

Compiled successfully.

Automatically optimizing pages  

Page                    Size
┌ ○ /                   27 kB
└   /_app               1.11 kB
+ shared by all         71.8 kB
  ├ chunks/commons.js   65.7 kB
  ├ runtime/main.js     5.26 kB
  └ runtime/webpack.js  812 B

λ  (Server)  server-side renders at runtime (uses getInitialProps or getServerProps)
○  (Static)  automatically rendered as static HTML (uses no initial props)
●  (SSG)     automatically generated as static HTML + JSON (uses getStaticProps)

Done in 4.31s.

The bundle now contains only the components that it should. I will create PR with the changes to the react-core repo.

Edit: PR with the plugin config for react-core can be found here: https://github.com/patternfly/patternfly-react/pull/3448

If this is something we can agree on, the only thing remaining would be a similar solution to the react-core repo and we are done here.

[evwilkin]

@Hyperkid123 thank you for the help on this issue!

Resolved in #3448

[evwilkin]

Keeping #3155 open to continue work on tree shaking components from react-core.