grommet-icons icon indicating copy to clipboard operation
grommet-icons copied to clipboard

Grommet-icons does not support webpack tree shaking

Open shai32 opened this issue 6 years ago • 17 comments

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

shai32 avatar Jan 04 '19 19:01 shai32

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

ericsoderberghp avatar Jan 04 '19 19:01 ericsoderberghp

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

shai32 avatar Jan 05 '19 18:01 shai32

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

shai32 avatar Feb 04 '19 08:02 shai32

@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

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 avatar Feb 04 '19 14:02 atanasster

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

atanasster avatar Feb 04 '19 14:02 atanasster

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?

alexeikaratai avatar Feb 05 '19 08:02 alexeikaratai

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.

ShimiSun avatar Feb 06 '19 03:02 ShimiSun

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

atanasster avatar Feb 06 '19 03:02 atanasster

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

ShimiSun avatar Feb 06 '19 03:02 ShimiSun

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?

atanasster avatar Feb 06 '19 03:02 atanasster

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.

ShimiSun avatar Feb 06 '19 03:02 ShimiSun

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

atanasster avatar Feb 06 '19 03:02 atanasster

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!

oorestisime avatar Feb 13 '19 22:02 oorestisime

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 avatar Feb 13 '19 23:02 atanasster

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

atanasster avatar Feb 13 '19 23:02 atanasster

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

ShimiSun avatar Apr 10 '19 03:04 ShimiSun

No i completely forgot to do this.

oorestisime avatar Apr 10 '19 05:04 oorestisime