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

Use direct imports of PF4 components #814

Merged
merged 1 commit into from
May 21, 2020

Conversation

Hyperkid123
Copy link
Contributor

This changes how we are importing PF components to improve the build size of JS chunks

I have also locked the PF version to 3.141.4 due to a bug withing the pagination component which causes an additional 0.5MB of react-tokens assets to be bundled with the component and it was not fixed yet. (issue here: patternfly/patternfly-react#4184). We could ever that I have not found any critical piece of code in the newer version that would affect these bundles.

I have tried and use babel transform plugin to do this import transformation while building, but parcel does not support babel.config.js files, because PF requires some fs searching to actually find correct files and that cannot be done in .babelrd or babel.json :(

There is an eslint plugin from PF that checks the imports, but as I tried to add it to this project it completely messed up the linter and I did not want to change every single file in order to fix this issue (plugin: https://github.com/patternfly/patternfly-react/tree/master/packages/eslint-plugin-patternfly-react).

Before (only including the JS and CSS files because its relevant don't care about map files and images)

build/js/chrome.js                                           ⚠️  4.59 MB    52.46s
build/js/frontend-components-inventory.e3560e4d.js            892.99 KB    47.50s
build/js/chrome.css                                           439.84 KB    28.85s
build/js/frontend-components-remediations.a65e56a1.js         101.55 KB    23.39s
build/js/App.eeab7546.js                                       44.59 KB    26.24s
build/js/inventoryStyles.dee582d6.js                             1.4 KB     5.64s
build/js/remediationsStyles.83d6d9d2.js                         1.35 KB     3.33s
build/js/remediationsStyles.a24650e6.css                        1.22 KB     1.75s
build/js/App.30fd3cd2.css                                         942 B    13.39s

After

build/js/chrome.js                                            ⚠️  1.07 MB    30.50s
build/js/frontend-components-inventory.b5cf168a.js            ⚠️  1.01 MB    26.47s
build/js/frontend-components-inventory.b0938f5f.css            142.91 KB     5.58s
build/js/chrome.css                                            121.74 KB    11.41s
build/js/App.3f6cf847.js                                       106.81 KB    18.22s
build/js/frontend-components-remediations.ec1c6932.js           102.2 KB    15.39s
build/js/App.5da2d433.css                                       40.92 KB     6.28s
build/js/inventoryStyles.e68b22c8.css                            27.6 KB     6.74s
build/js/frontend-components-remediations.7be3c16c.css            6.7 KB     2.53s
build/js/inventoryStyles.dee582d6.js                              1.4 KB     8.20s
build/js/remediationsStyles.83d6d9d2.js                          1.35 KB     3.86s
build/js/remediationsStyles.a24650e6.css                         1.22 KB     2.16s

cc @karelhala @ryelo

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2020

Codecov Report

Merging #814 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #814   +/-   ##
=======================================
  Coverage   61.05%   61.05%           
=======================================
  Files          45       45           
  Lines         873      873           
  Branches      169      169           
=======================================
  Hits          533      533           
  Misses        277      277           
  Partials       63       63           
Impacted Files Coverage Δ
src/js/App/Header/Brand.js 85.71% <ø> (ø)
src/js/App/Header/InsightsAbout.js 75.00% <ø> (ø)
src/js/App/Header/Login.js 100.00% <ø> (ø)
src/js/App/Header/LogoutAlert.js 66.66% <ø> (ø)
src/js/App/Header/ToolbarToggle.js 52.94% <ø> (ø)
src/js/App/Header/Tools.js 70.00% <ø> (ø)
src/js/App/Header/UserIcon.js 83.33% <ø> (ø)
src/js/App/Header/UserToggle.js 86.66% <ø> (ø)
src/js/App/NoAccess.js 40.00% <ø> (ø)
src/js/App/Sidenav/Navigation.js 87.23% <ø> (ø)
... and 2 more

Copy link
Contributor

@karelhala karelhala left a comment

Choose a reason for hiding this comment

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

This looks really aweseom! Kudos to you @Hyperkid123 for taking the time to look into this.

@@ -113,7 +114,7 @@
"@babel/polyfill": "^7.8.7",
"@babel/runtime": "^7.9.6",
"@patternfly/patternfly": "^2.71.5",
"@patternfly/react-core": "^3.153.13",
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to use at least version 3.153.7 due to this bug fix patternfly/patternfly-react#3990 I think we can be fine with the extra bundle size for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😢 ok I will put It back. Hopefully they will fix it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i used the latest version of PF. That added around 0.5MB to the inventory frontend

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's in inventory frontend then it's no big deal.

@karelhala karelhala merged commit 34c5710 into RedHatInsights:master May 21, 2020
@Hyperkid123 Hyperkid123 deleted the minimize-build branch May 21, 2020 14:49
ryelo added a commit that referenced this pull request Jun 11, 2020
* Update all deps (#795)

* Update PF and FEC deps to newest stable versions

* Update all other deps to latest stable version

* Fix linting errors

* update case and split logout (#799)

Co-authored-by: Karel Hala <[email protected]>

* use a skeleton instead of logging in text (#792)

* add en-us to automation platform link (#808)

per james bailey on the customer portal side, this addition should resolve the issue with doc link in automation platform going to dead page.

Co-authored-by: Ryan Long <[email protected]>

* Bump @redhat-cloud-services/frontend-components-inventory (#809)

* update sentry and add more apps (#802)

* update sentry and add more apps

* lint

* Use direct imports of PF4 components (#814)

* Bump @redhat-cloud-services/frontend-components from 1.0.24 to 1.0.28 (#815)

* Add Catalog, Approval and Sources to about modal app list. (#834)

* Update inv package to newest version 33 (#835)

* Fix highlight of duplicate sub-nav items (#836)

* Fix highlight of duplicate sub-nav items

* Add parentId verification to highlight condition

* Add appNavClick - parentId param to docs

Co-authored-by: Ryan Long <[email protected]>
Co-authored-by: Chris Budzilowicz <[email protected]>
Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Martin Maroši <[email protected]>
Co-authored-by: Filip Hlavac <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants