feathericons / react-feather

React component for Feather icons
https://npm.im/react-feather
MIT License
1.93k stars 126 forks source link

Tree-shaking seems to be broken #6

Closed austenpayan closed 5 years ago

austenpayan commented 6 years ago

Running webpack bundle analyzer, I am seeing every icon being included in my minified bundles:

screen shot 2017-11-14 at 8 11 56 am

Not sure if I am doing something wrong of if there is a problem in the package.

babelrc settings:

{
    "presets": [
        ["env", {
            "loose": true,
            "modules": false
        }], "react"
    ],
    plugins: [
        'syntax-dynamic-import',
        'transform-object-rest-spread',
        'transform-class-properties'
    ]
}
austenpayan commented 6 years ago

I am using import statements as well.

SuperOleg39 commented 6 years ago

Same problem, but i use TypeScript without Babel.

Fast fix for reduce bundle size - use full path, example: import Download from 'react-feather/dist/icons/download';

cichelero commented 6 years ago

I had this problem too, and sent a pull request #10 describing @SuperOleg39 solution in the docs.

ghost commented 6 years ago

This is still broken.

I'm using it like this:

import { Users } from "react-feather";

But in analyze I see this:

image

I'm using create-react-app latest default.

Something broken?

cichelero commented 6 years ago

This will always be the case untill this project is using babel to build ES modules into module.exports. It happens because WebPack only does tree shaking from ES modules. You can read it here.

You will need to import using the full path like described above.

ghost commented 6 years ago

Ok. I understand. Can I follow this thread for updates to when ES modules will be there? There is a PR?

Allend13 commented 6 years ago

Yo guys,

Help is here:

  1. Add babel-plugin-import
  2. Add next config to babel plugins: ["import", { "libraryName": "react-feather", "libraryDirectory": "dist/icons", "camel2DashComponentName": false }],

Profit!

austenpayan commented 6 years ago

@Allend13 tried using babel-plugin-import. The plugin does not seem to support using aliased imports, e.g.

import { MessageCircle as Compose } from 'react-feather';

Produces an error in Webpack. Not sure if this is a viable option, as I use import aliasing quite often.

RinkAttendant6 commented 6 years ago

@cichelero Would it help if the project added a "sideEffects": false to the package.json file, according to the link you posted?

alex996 commented 6 years ago

@Allend13 That config will fail at build time with Module not found: Error: Can't resolve .... You need to leave off camel2DashComponentName as true by default. This config works

"plugins": [
  [ "import", { "libraryName": "react-feather", "libraryDirectory": "dist/icons" } ]
]

@RinkAttendant6 I don't see how it will help because the library is still transpiling ES modules to CommonJS, thereby making it hard to tree shake. It's not only Webpack that suffers from this, but also Rollup, which is geared towards ES modules as well.

If we were to go with ESM, we'd not only need "sideEffects": false but also "modules": false in .babelrc. But then anyone using Node would either need to transpile ESM to CJS themselves, or introduce a bundler to their build step. Coincidentally, many people already bundle their React server-rendered code, just as they have to their front-end. So I think this change would be welcomed, and IMO this is the most viable way to go forward. @cichelero What are your thoughts?

Other prospects are dim. We could ship two bundles, both CJS and ESM, but this will double the size, and put us at risk of timeout when installing the package. We could also publish react-feather-es, just like lodash-es did, but this would inevitably lead to duplication. End users would need to alias the library or force-rewrite the import path in their bundler config, which is doable, but hacky.

Just throwing out ideas. On the same note, has anyone been able to tree shake react-feather with webpack-common-shake? I guess not, and I myself couldn't. Babel's __esModule exports probably fall under the known limitations of webpack-common-shake.

RinkAttendant6 commented 6 years ago

@alex996 What about referencing the ESM file from the module field in package.json? From https://rollupjs.org/guide/en#tree-shaking:

If your package.json file also has a module field, ES6-aware tools like Rollup and webpack 2+ will import the ES6 module version directly.

And it looks like Webpack prefers the module field over the main field in the default mainFields configuration.

I don't have experience in publishing a library to npm, just trying to throw out some ideas here. If it comes to shipping two bundles, I don't really see that as problematic as long as the size of the final output doesn't increase (it shouldn't, because either the CJS or ESM would be considered depending on the build toolchain, not both.)

lili21 commented 6 years ago

@RinkAttendant6 You're right.

notflip commented 5 years ago

Is this still an ongoing issue?

sirugh commented 5 years ago

@notflip Yes. I think there was an open PR but it hasn't been merged.

dallas commented 5 years ago

Might be worth looking at Styled Icons. They boast:

styled-icons provides the Font Awesome, Feather, Icomoon, Material Design, Octicons, Typicons, Crypto Icons and Boxicons icon packs as Styled Components, with full support for TypeScript types and tree-shaking / ES modules.

There’s also React Icons, which looks similar to Styled Icons, but has some slightly different icon sets.

carmelopullara commented 5 years ago

Version 2.0.1 is out, with all the latest icons and support for tree-shaking.

sarahdayan commented 4 years ago

@carmelopullara I'm using 2.0.8 and I still seem to have the issue.

Capture d’écran 2020-08-15 à 02 09 27

I'm using ES imports (reexports, not full paths) within a CRA app.

damianobarbati commented 4 years ago

@carmelopullara I don't see the tree-shaking working properly with es imports:

import { Download, Trash2, Copy } from 'react-feather';

All library ends up in the bundle. What's the intended way to use the lib?

nedtwigg commented 4 years ago

I believe this is still broken as of 2.0.8. I switched to react-icons, and it fixed the tree shaking. I'd rather use this library though, react-icons seems to bring more functionality than I want. EDIT the problem was using commonjs rather than esnext in my tsconfig.json, thanks @alex996 for finding the problem (see below). react-feather now shrinks down to be imperceptibly small in my build, far smaller than react-icons.

sirugh commented 4 years ago

If ya'll are experiencing a regression in functionality I would recommend using codesandbox to reproduce the issue that way you can provide a working link to show the example. Probably want to do something with webpack bundle analyzer or something.

alex996 commented 4 years ago

The basic webpack + babel (.js) or webpack + typescript (.tsx) seems to tree shake just fine. I was suspicious that the forwardRef wrapper, combined with both propTypes and displayName assignment, might be breaking compression in terser, but it's not! So, something is probably not right in your webpack config.

To lib authors: consider using oliviertassinari/babel-plugin-transform-react-remove-prop-types to strip prop types in production. It's generally a good practice in React and can make or break tree shaking (see terser/terser#293). I don't believe it's causing any issues here, but it still is a safety net.

nedtwigg commented 4 years ago

Love the icons, totally possible that my config is wrong, and I don't expect anyone to spend their time debugging my personal issue. But just switching to react-icons, with no other changes, reduced my bundle size by ~250kb (see commit above). I shrunk my build down to this reproducible example, npx webpack and you'll see this, which shows that lodash is getting tree-shook, but react-feather is not:

image

alex996 commented 4 years ago

@nedtwigg You need to change the following lines in tsconfig.json:

-"module": "commonjs",
+"module": "esnext",
+"moduleResolution": "node",

Check out the size before 409054 B and after 27987 B.

Basically, with commonjs you are telling typescript to transpile ES Modules into CommonJS require/exports syntax. CommonJS is notoriously hard to tree shake due to its dynamic nature, and most bundlers doesn't even try. If you set esnext however, this will leave import/export syntax intact so that webpack can take care of module resolution and tree shaking.

nedtwigg commented 4 years ago

Thanks very much @alex996, sorry for making noise here, hope it was at least useful to others. In the app we're using this on, changing the module to esnext basically cut our compiled size in half - improved a lot of things besides just react-feather. I'm kinda curious why lodash and react-icons managed to get some shrinking done in our badly-configured environment, but not curious enough to dig further. I'm gonna just take the solution and run!