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

Font Awesome 5.0/5.1, webpack and tree shaking not working correctly #11311

Closed
5 tasks done
tommymcgahee opened this issue May 30, 2018 · 10 comments
Closed
5 tasks done

Comments

@tommymcgahee
Copy link

tommymcgahee commented May 30, 2018

Expected Behavior

When running gulp build --production with font awesome version 5.0/5.1 installed and properly imported in app.js, webpack should properly "tree shake" and include in the final build ONLY the fonts that are imported and NOT the entire library(ies).

I know this is not foundation-sites specific, but after trying to troubleshoot at FortAwesome/Font-Awesome#13232 I'm asking here for your gulp / webpack / uglifyjs expertise since gulp is handling webpack and uglifyjs (from what I can understand) in this project.

Current Behavior

The whole font awesome style library is being included in the final app.js build regardless of the environment flag, ballooning my app.min.js file size to ~700 kbs.

Possible Solution

After reading webpack/webpack#2867 and mishoo/UglifyJS#1261 and FortAwesome/Font-Awesome#12429 (comment) I have tried upgrading the following packages to these versions:

"gulp-uglify": "^3.0.0",
"webpack": "^4.10.2",
"webpack-stream": "^4.0.3",
"uglify-js": "^3.3.28"

I have also tried adding "sideEffects": false, to my project's package.json file and mode: "production" to my webpackConfig variable inside of gulpfile.babel.js. Neither fix the issue.

My builds are successful, but the file size is still ballooning:

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

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  app (779 KiB)
      app.js


WARNING in webpack performance recommendations:
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/

Test Case and/or Steps to Reproduce (for bugs)

How to reproduce:

Font Awesome 5.1 (prerelease) is where they are focusing their tree shaking fix development efforts, so those steps are:

  1. Install the styles
npm i --save @fortawesome/fontawesome-svg-core@prerelease
npm i --save @fortawesome/free-solid-svg-icons@prerelease

  1. Add this to /src/assets/js/app.js
import { library, dom } from '@fortawesome/fontawesome-svg-core';
import { faUser } from '@fortawesome/free-solid-svg-icons';
library.add( faUser );
dom.watch()

  1. Run gulp build --production

Docs: https://fontawesome.com/preview/how-to-use/use-with-node-js

Context

I cannot migrate to Font Awesome 5.x until this is fixed.

Your Environment

  • Foundation version(s) used: 6.4.3
  • Browser(s) name and version(s): All of them
  • Operating System and version (desktop or mobile): All of them
  • Link to your project: NA

Checklist (all required):

  • I have read and follow the CONTRIBUTING document.
  • This is a bug report or a feature request.
  • There are no other issues similar to this one.
  • The issue title is descriptive.
  • The template is fully and correctly filled.
@ncoden ncoden changed the title Font Awesome 5.0/5.1, webpack and tree shaking not working correctly (I explain in the post) Font Awesome 5.0/5.1, webpack and tree shaking not working correctly May 30, 2018
@DanielRuf
Copy link
Contributor

Treeshaking is just (available) for JS. Unused css can be removed with solutions like nanocss, uncss, purgecss and so on.

@DanielRuf
Copy link
Contributor

DanielRuf commented May 30, 2018

SplitChunks, AggressiveSplitting and other plugins can be also used. Or try the minicss plugin to extract the styles to a css file.

@DanielRuf
Copy link
Contributor

DanielRuf commented May 30, 2018

Which webpack and gulp version do you use? Is the filesize on the filesystem the same?

@tommymcgahee
Copy link
Author

The size of dist/assets/js/app.min.js on the filesystem is 824 KB, and webpack says this during the build process:

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

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  app (781 KiB)
      app.js

Here is a paste of my package.json file:

{
  "name": "foundation-zurb-template",
  "version": "1.0.0",
  "description": "Official ZURB Template for Foundation for Sites.",
  "main": "gulpfile.babel.js",
  "scripts": {
    "start": "gulp",
    "build": "gulp build --production"
  },
  "author": "ZURB <[email protected]>",
  "license": "MIT",
  "dependencies": {
    "@fortawesome/fontawesome-svg-core": "^1.2.0-14",
    "@fortawesome/free-regular-svg-icons": "^5.1.0-11",
    "@fortawesome/free-solid-svg-icons": "^5.1.0-11",
    "a11y-dialog": "^5.0.2",
    "datatables.net": "^1.10.16",
    "foundation-sites": "~6.4.1",
    "jquery": ">=3.0.0",
    "motion-ui": "~1.2.2",
    "npm": "^5.10.0",
    "owl.carousel": "^2.3.4",
    "uglify-js": "^3.3.28",
    "url-search-params": "^0.6.1",
    "what-input": "^4.1.3"
  },
  "devDependencies": {
    "babel-core": "^6.26.3",
    "babel-loader": "^7.1.4",
    "babel-preset-env": "^1.7.0",
    "babel-register": "^6.7.2",
    "browser-sync": "^2.24.4",
    "gulp": "github:gulpjs/gulp#4.0",
    "gulp-autoprefixer": "^3.1.0",
    "gulp-babel": "^6.1.3",
    "gulp-clean-css": "^3.9.4",
    "gulp-cli": "^1.2.1",
    "gulp-concat": "^2.5.2",
    "gulp-extname": "^0.2.0",
    "gulp-if": "^2.0.0",
    "gulp-imagemin": "^2.2.1",
    "gulp-load-plugins": "^1.1.0",
    "gulp-rename": "^1.2.3",
    "gulp-sass": "^2.1.0",
    "gulp-sourcemaps": "^1.6.0",
    "gulp-uglify": "^3.0.0",
    "gulp-uncss": "^1.0.1",
    "js-yaml": "^3.11.0",
    "panini": "^1.6.2",
    "rimraf": "^2.4.3",
    "style-sherpa": "^1.0.0",
    "vinyl-named": "^1.1.0",
    "webpack": "^4.10.2",
    "webpack-stream": "^4.0.3",
    "yargs": "^3.8.0"
  },
  "repository": {
    "type": "git",
    "url": "https://github.com/zurb/foundation-zurb-template.git"
  },
  "bugs": {
    "url": "https://github.com/zurb/foundation-sites/issues",
    "email": "[email protected]"
  },
  "babel": {
    "presets": [
      "env"
    ]
  },
  "private": true
}

@DanielRuf
Copy link
Contributor

And which commits of the fontawesome packages are in the lockfile? Because this is clearly an issue with the fontawesome package and not us or webpack in general imho.

Treeshaking is normally just for JS classes and methods. Treeshaking fonts for user facing solutions where they can enter text is not a good idea in general.

So I think this is just an issue with fontawesome and their require's.

@tommymcgahee
Copy link
Author

Here is a paste of the relevant portion of the lock file. Can you elaborate on what you mean by it not being a good idea? This is my first experience with tree shaking so I'm learning as I troubleshoot this.

"@fortawesome/fontawesome-common-types": {
   "version": "0.2.0-9",
   "resolved": "https://registry.npmjs.org/@fortawesome/fontawesome-common-types/-/fontawesome-common-types-0.2.0-9.tgz",
   "integrity": "sha512-V+PyKLFoFvfg9hj4weCjWVYPKvUI5hFLlPxPzb/SIrI9zhlOibHKtVpOy577fIXV7HFTy+FpeSlMDrhkV76dfA=="
 },
 "@fortawesome/fontawesome-svg-core": {
   "version": "1.2.0-14",
   "resolved": "https://registry.npmjs.org/@fortawesome/fontawesome-svg-core/-/fontawesome-svg-core-1.2.0-14.tgz",
   "integrity": "sha512-V13Ou3UZ+Y0J5WTnhHDUmZFsz8ycxAyGaowrrPu7uuZXVT78/l4OCrOP6y5U6PdZoy3VJbZvkxCXZyTA6C32fA==",
   "requires": {
     "@fortawesome/fontawesome-common-types": "0.2.0-9"
   }
 },
 "@fortawesome/free-regular-svg-icons": {
   "version": "5.1.0-11",
   "resolved": "https://registry.npmjs.org/@fortawesome/free-regular-svg-icons/-/free-regular-svg-icons-5.1.0-11.tgz",
   "integrity": "sha512-Oj9on7JlLF/TgJjuYl76EHLCCL5MydcPCAD4U0GZe0GeqAyDijiT11tDJb7d4XFjg9/EOl/PlFG49KTqRY+jdQ==",
   "requires": {
     "@fortawesome/fontawesome-common-types": "0.2.0-9"
   }
 },
 "@fortawesome/free-solid-svg-icons": {
   "version": "5.1.0-11",
   "resolved": "https://registry.npmjs.org/@fortawesome/free-solid-svg-icons/-/free-solid-svg-icons-5.1.0-11.tgz",
   "integrity": "sha512-Dc5EaDbQryTRSt96NzxEnENX+bcKXun3mpvdPt8rSkx3Kf8Kyda60qaA3Or6AKfNYcxLup5Q6zwfWdhdhTNrpQ==",
   "requires": {
     "@fortawesome/fontawesome-common-types": "0.2.0-9"
   }
 },

@DanielRuf
Copy link
Contributor

Because it would remove unused icons which users might need in dynamically added content.

@DanielRuf
Copy link
Contributor

Well, in general we use webpack-stream which uses webpack under the hood. There is a normal webpack config used in our templates.

Is somewhere the whole repository with the problematic code so we can clone and test with it?

Per se the issue is not foundation-sites but the usage of webpack.

I would say we close this issue here as this is more an issue with fontawesome and webpack.

@tommymcgahee
Copy link
Author

I hear you on tree shaking with dynamically added content. In our case we're not going to run into that issue, but I can see where it could be a problem.

Unfortunately there isn't anything public that I can show you to test out. I am able to import the icons without tree shaking, so I will close this out, but I would request that you guys keep an eye on this issue since fontawesome 5.x is now live and popular, and webpack looks to be the future direction of both yours, and their, project.

@DanielRuf
Copy link
Contributor

but I would request that you guys keep an eye on this issue since fontawesome 5.x is now live and popular, and webpack looks to be the future direction of both yours, and their, project.

It is not an issue with foundation sites imho ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants