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

Only import the expand icon #109

Merged

Conversation

rileyhilliard
Copy link
Contributor

Only import font-awesome expand icon instead of the entire react-icons library.
This optimization reduces the production build output by 374k, a 68% reduction in asset size.

How to test this:

  1. checkout the branch locally
  2. install dependency changes via yarn or npm install
  3. run yarn example and verify everything looks normal
  4. merge Adding webpack-build-analyzer to react-digraph #107 and run yarn analyze-bundle to see prod build stats.

Visualized improvements via webpack-build-analyzer:

screen shot 2019-02-24 at 8 35 41 am

screen shot 2019-02-24 at 8 29 26 am

Resolved #108

@rileyhilliard
Copy link
Contributor Author

@ajbogh / @chiwoojo
This will reduce the production build asset size of react-digraph by 68% ☝️

@ajbogh
Copy link
Contributor

ajbogh commented Feb 25, 2019

There are 4 failing tests with the following error:

 FAIL  __tests__/components/graph-controls.test.js
  ● Test suite failed to run

    TypeError: First argument must be a string

      at HTMLReactParser (node_modules/html-react-parser/index.js:17:11)
      at Object.<anonymous> (src/components/graph-controls.js:768:177)
      at Object.<anonymous> (__tests__/components/graph-controls.test.js:9:22)

Log out faExpand:

import faExpand from '@fortawesome/fontawesome-free/svgs/solid/expand.svg';
const steps = 100; // Slider steps
console.log(faExpand);

console.log result:

console.log src/components/graph-controls.js:26
      {}

Just noticed that you used @fortawesome. Not sure if the namespace should be an awesome fort, or an awesome font.

It still didn't work with the namespace changed to @fontawesome.

@rileyhilliard
Copy link
Contributor Author

@ajbogh I was confused by the same @FortAwesome thing, but it appears that that's fontawesome's parent branding.

Weird, I thought I had the tests working, but it looks like that might have been before I was explicitly importing the svg asset. It looks like the issue is actually just jest not knowing how to import the asset. I added an icon mock that fixes the test issue 👇.
screen shot 2019-02-26 at 6 52 41 am
As a secondary precaution, I manually greped dist/main.min.js for the svg expand icon asset. It's forsure included properly, but jest seems to not be great at knowing how to handle certain types of assets.

@ajbogh
Copy link
Contributor

ajbogh commented Feb 26, 2019

Great work on this. I checked it out and it looks good. Thanks!

@ajbogh ajbogh merged commit 8e60f76 into uber:master Feb 26, 2019
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.

2 participants