grommet / grommet-icons

Iconography for Grommet and React.js
https://icons.grommet.io
Apache License 2.0
334 stars 65 forks source link

Grommet-icons does not support webpack tree shaking #83

Open shai32 opened 5 years ago

shai32 commented 5 years ago

I Only imported 5 icons, but it adds 500kb to my bundle size

the package should support tree shaking. so only the imported icons will be added to the bundle. notice that I am importing only the icons I am using e.g: import { AddCircle } from 'grommet-icons/icons/AddCircle';

image

ericsoderberghp commented 5 years ago

Hmm... I know we've tested tree shaking via: `import { AddCircle } from 'grommet-icons'. Have you tried that?

shai32 commented 5 years ago

yes also tested with `import { AddCircle } from 'grommet-icons' same issue

shai32 commented 5 years ago

I Only need 3 icons, adding 500kb just for that, seems to much

atanasster commented 5 years ago

@ericsoderberghp @ShimiSun if a project is not set up for tree shaking, grommet itself forces importing all icons from grommet-icons. As the screenshot above suggests, both grommet and grommet-icons are not 'tree-shaken' - for my nextjs apps even with webpack 4, grommet and grommet-icons are not automatically tree shaken.

I think this problem will come about more often as more sophisticated users start using v2 and check what is being packaged into their apps.

To fix this for grommet-icons I would suggest importing all icons from their respective files: 1. import { Actions } from 'grommet-icons/icons/Actions'; https://github.com/grommet/grommet/blob/9c5af4e6ec036cdfbcd5c9de69437e502a50d84b/src/js/themes/base.js#L4

  1. import { FormSearch } from 'grommet-icons/icons/FormSearch'; https://github.com/grommet/grommet/blob/9c5af4e6ec036cdfbcd5c9de69437e502a50d84b/src/js/components/DataTable/Searcher.js#L7

To fix similar issues with grommet itself, I would suggest to document importing components from their folders if tree shaking is not enabled (currently all documentation, examples etc. assumes tree shaking being automatically enabled): import { Box } from 'grommet/components/Box';

atanasster commented 5 years ago

Just to make it clear - even when @shai32 correctly used import { AddCircle } from 'grommet-icons/icons/AddCircle';, the full grommet-icons is being imported because of the above issues in grommet itself

alexeikaratai commented 5 years ago

the same problem. I don't import no one icon, but all icons are in bunlde js. How to remove it from my bundle ? Can i install grommet without icons?

ShimiSun commented 5 years ago

from #general(Slack), there are still some issues with tree-shaking with webpack 4, its including both the es6 and js versions of all files in the /utils and /themes folders. You can duplicate with a small project like slide and https://facebook.github.io/create-react-app/docs/analyzing-the-bundle-size.

atanasster commented 5 years ago

@ShimiSun - would you also consider adding some documentation on the grommet usage without tree shaking set up?

ShimiSun commented 5 years ago

@atanasster where exactly do you suggest adding it, just so we'll be on the same page

atanasster commented 5 years ago

Possibly in the section ‘Start coding’ on the home page have a note on importing components from their own folder if tree shaking is not enabled with a small code snippet?

ShimiSun commented 5 years ago

not sure it will fit on the home page, I couldn't find import refs there to potentially add a note, and we are trying to keep the page light for both design and dev, while this info may be too technical. It might be a better option to expose in our Github readme pages like https://github.com/grommet/grommet-starter-new-app.

atanasster commented 5 years ago

Yes, that would be good too, just someplace to point users to

oorestisime commented 5 years ago

So to recap here:

Tree shaking support was added with latest release and it greatly improves previous state of things. There's still one issue (duplicated icons in bundle) left and this occurs when user imports icons that are also imported by the grommet core package itself. This happens for a handful of icons and I am inclined to say it is not of a critical issue (still a bug though). To workaround this issue user can import those icons directly from grommet-icons/icons/IconName.

This should get documented somewhere along with the general state of things tree shaking and grommet. An initial idea would be the wiki while waiting for the grommet-site to support different kind of pages.

Is anyone feeling doing this? I could draft something but i only learned about tree shaking just recently so not sure i am the most appropriate person to do this!

atanasster commented 5 years ago

Its almost correct - v2 had what is called webpack 4?tree shaking support by having the setting sideEffects turned off in package.json. Thats about it, othing special.

The problem arose because grommet 2 assumes everyone is using webpack 4, which the users reporting thise bugs did not. What was fixed was importing gromet icons to avoid loading all of the icons if webpack 4 is NOT used. What is nit fixex, is documenting how to make imports when tree shaking is not enabled. I have published my take and sample projects on tree shaking already https://grommet-nextjs.herokuapp.com/tree-shaking

atanasster commented 5 years ago

The little bit of documentation that i thought should be on the grommet site is this clarification on imports https://grommet-nextjs.herokuapp.com/get-started

Clearly the original posters are also importing the entire grommet library because they are following the grommet site/ samoles but are not on webpack 4 |do not have tree shaking enabled

ShimiSun commented 5 years ago

@oorestisime were you able to document the best practice? anything else left before closing this issue?

oorestisime commented 5 years ago

No i completely forgot to do this.