-
-
Notifications
You must be signed in to change notification settings - Fork 743
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
Resolved tree shaking issue mentioned in issues #154 #727
base: master
Are you sure you want to change the base?
Conversation
This PR should resolve tree shaking issue mentioned in react-icons#154 without using all-files. It also resolves typescript issue when you try to import esm file directly.
When someone imports files from this library by using import statement, it will automatically use Currently typescript complains about the type If you directly import esm files which can be solved by specifying types. |
It will also resolve few tasks from #634
|
Thank you for this fix. Upvote! The tree shaking issue is enormous for us. React-icons is most of our bundle size in out nextjs app. |
Any idea what the hold up is for this to be reviewed? This would greatly help our project |
Big issue for us as well, our pages are extremely laggy from this, and all-files is missing a ton of icons we need. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@kamijin-fanta @gorangajic @jayehmke @JeffersonFilho Are you going to maintain this library in future or planning to archive it? |
@oztek22 Sorry for the delay in replying. Thanks for your excellent contribution. Is it possible to add a test to this PR? Currently there is code in Webpack4 that tests that Tree-Shaking is done correctly. https://github.com/react-icons/react-icons/tree/master/packages/webpack4-test |
CI step terminated abnormally. You need to run the code formatter. |
@kamijin-fanta I fixed the pipeline but unfortunately I cannot run https://github.com/react-icons/react-icons/tree/master/packages/webpack4-test locally as it gives me following error:
|
Both my environment and the CI environment are working fine... check the yarn version and |
Okay it worked after running the shell script. Would that be suffice? |
Please add a test that fails if you do not apply this PR change. |
@kamijin-fanta I will try it, if anyone else has time for it, feel free to add a commit on my pr. |
I spent a while on this only to realize source-maps were being inlined because of my webpack config. I also tested this pull request before and after applying the patch and noticed no change in output size. Both about 8kB. Both had tree shaking working. |
039502a
to
399b45c
Compare
Hi there! Thanks for your efforts. Any news about this PR? Would it make |
This PR should resolve tree shaking issue mentioned in #154 without using all-files.
It also resolves typescript issue when you try to import esm file directly.