Fix #154 by allowing absolute imports #340
Conversation
|
@kamijin-fanta any feedback on this? |
|
@kamijin-fanta it seems like this could potentially be really helpful. I understand if you're not interested in merging for any number of reasons, but could you comment? I've had the behavior of imports bouncing back and forth between versions. With some versions it would import the icons I specify and with others it would import all the icons of any set I specify (eg, I use one FontAwesome icon and it imports them all) As it stands right now I'm not sure what the behavior is - but this PR seems beneficial. |
|
I've forked the repo here until we get some traction https://github.com/aliogaili/meronex-icons |
|
@aliogaili @slapbox I am sorry for not replying to this PR for a while. Placing a large number of files can increase build time and I want to be careful. (ex: GameIcon contains 3000 files...) Specifically, could you tell me the case where TreeShaking cannot be done with the current approach? |
|
Good to hear from you @kamijin-fanta ! Some build systems such MeteorJS and (others referenced in that thread) still doesn't support tree shaking and shipping a large incon bundle to clients is simply not acceptable. So we need to be careful about what do optimize, could you please elaborate which build process are you referring to specifically? |
|
@kamijin-fanta I'll have to get back to you some time in the next few weeks when I rebuild our project. I'll investigate whether the tree shaking is working as intended at present. With differing versions of React-Icons I've had different results, sometimes getting just the icons I import, and other times getting all the icons from any subset I import, i.e. including one Font Awesome icon includes them all. |
|
@aliogaili Thank you. I understood that it means Parcel, MeteorJS, Webpacker, etc. I have only tested CreateReactApp and plain Webpack+Babel. I haven't tested any other build systems, so I think it's necessary to do this. What does this mean? English is not a native language. sorry.
@slapbox Thank you! I would like to merge the results into this project. I contacted npm yesterday. "Make me the owner of the unused @react-icons organization." This inquiry has been accepted and the package can now be published under @react-icons/. For example, I can publish packages like @react-icons/all, @react-icons/font-awesome_splitted. I'm thinking of using a single project to manage multiple packages with different file styles. |
|
@kamijin-fanta splitting the pancakes and publishing different packages is a good a idea, I was planning to do the same. And yes, we need to make sure it works beyond webpack. We can't depend only on the build system tree shaking. You said build process will take longer, I am not sure which build process? I know the NPM package gets larger with many icons but creating files I'm still not sure what is the issue here and why we can't merge this. |
|
@aliogaili I believe that packages with a large number of files will take a very long time to install. Prepare an empty repository, clear the yarn cache, and then verify. Summary of yarn install time.
Results for Windows 10.Results on Linux.Expect tar.gz extraction to take a lot of time. When react-icons@2, a lot of files were causing slow package installation. I want to avoid this problem in CI. So I reduced the number of files. I thought this should work if Webpack is properly configured. Build time does not increase. |
|
Got it, thanks for clarifying this, and yes it is a tradeoff 🤷♂️ and I understand where you coming from. I think it is important we resolve the bundling to the client issue until tree-shakig is a known to be the build standard for all systems (it is slowly getting there). I'm also trying to get our build systems to adopt tree shaking, but right now we can't assume all the bundlers in the ecosystem supports tree shaking. The build taking a little extra time is one thing but clients and customers loading the site/apps few seconds more because of the icons is another thing. If we need to pick, clients user experience is, needless to stay, more important than the 34 seconds extra CI time. As it stands we simply can't use react-icons, adding MBs to client bundle is not acceptable unfortunately. I will be happy if there is another way to resolve this. Perhaps we can split it into smaller packages if build time is a concern, or have two versions published on NPM, one with files and one without? Or we can keep the fork for those who still build systems with tree shaking? I'm ok with any direction as long as this issue is resolved, my goal is make react-icons accessible to everyone, I will leave it up to you to decide the best path forward 🙂. Thanks @kamijin-fanta ! |
|
I have just tried it with MeteorJS. Certainly, the bundle size increases. I'll explore other build systems when I have time. Is there a good naming when publishing another package? I think about making another package based on this PR. If TreeShaking is the goal, |
|
Thanks for testing it with MeteorJS @kamijin-fanta! by the way Meteor is adding tree shaking soon but until then we have to use absolute imports. I was thinking to name it react-icons/all or react-icons/absolute or react-icons/files, I wouldn't use the word legacy myself because absolute imports is part of the modern JS syntax, and some people might still prefer it, and those bundlers (webpack, rollup, etc) are doing the extra logic with tree-shakig. |
|
I finally got around to investigating the behavior of the current version and I can confirm that, at least in our setup, it tree shakes exactly as expected, leaving only the imported icons from each set. |
|
What is your current setup? Which build system are you using? |
|
Using Webpack with the default |
|
I see but the issue is not with webpack since it does support tree shaking...the issue is with build systems that don't support that feature. |
|
That's a valid concern. There were sporadic versions where it was an issue even with Webpack which is why I wanted to report that the current version works properly. |
|
I'm working on building |
|
Webpack 5 has fixed this issue but everyone that's still on webpack 4 has this issue. Treeshaking works but if you have 10 pages and use 1 icon on every page, you'll end up with 10 icons on every page. See webpack/webpack#4453. What's necessary to get this merged? |
|
published @react-icons/all-files in v4.0.0. try it! https://github.com/react-icons/react-icons/releases/tag/v4.0.0 |
Fix #154 by giving developers the option of either using absolute imports or destructuring.
It generates a unique file for each icon.
This will eliminate the assumption that the build systems are configured for tree shaking.