duongdev / phosphor-react-native

phosphor-icons for react-native. A flexible icon family for React Native
https://www.npmjs.com/package/phosphor-react-native
MIT License
186 stars 24 forks source link

Tree shaking support #61

Open mrkpatchaa opened 3 weeks ago

mrkpatchaa commented 3 weeks ago

Currently, when we import icons like this import { Horse, Heart, Cube } from 'phosphor-react-native'; it results in a big bundle size because of importing all the icons, even if the others are not used.

Expo adds experimental support for Removing unused imports and exports in SDK 52.

This issue is to track the progress around tree shaking support.

Jonnboy91 commented 3 weeks ago

Great to see that this is under investigation already 💪

I came across this issue only because API version < 31 crashes due to this issue (Pixel 4 API 28 was my emulator that I originally noticed the crash). I don't know exactly why API >= 31 works fine without crashing, maybe they handle memory optimisation or something differently. Once I deleted all icon imports the app was working again and then I imported using the individual import and the app worked, but even one import with the normal way just crashed the app.

We're now doing the individual imports, before this is fixed. Do you know the reason why 2.0.0 seemed to work, but this crashing issue came with the new 2.1.0 version (Tried the 2.2.1 as well that you have in npmjs and that crashes as well), so is it that the tree shaking stopped working in the newer version? 🤔

mrkpatchaa commented 3 weeks ago

Hmmm @Jonnboy91 I don't recall changing anything that could break/change the tree shaking aspect in 2.1. The 2.1 mainly brings more icons (aligned with phosphor 2.1), maybe it's the root cause? And yes I think the app crashes because of the large number of icons to load (1500+ x 5 weights) I started making some tests and the difference in size between the normal import and individual import is around 2 MB.

mrkpatchaa commented 3 weeks ago

Metro bundler does not support tree shaking by default.

  1. As explained above, Expo is adding an experimental support in version 52 https://github.com/facebook/metro/issues/227#issuecomment-2306946288
  2. Microsoft added a metro plugin to rnx-kit in order to tree shaking. https://github.com/facebook/metro/issues/632#issuecomment-1994077817

Other icons libraries like react-native-vector-icons uses font files + glyphmaps to render icons. I added support for ionicons v6 myself with this PR https://github.com/oblador/react-native-vector-icons/pull/1484. I don't think it can be done with phosphor icons because of duotone + it would result in a big font file, which means we are not really reducing the size of our app.

The generated index.tsx file file looks like this

/* GENERATED FILE */
export { type Icon, type IconProps, IconContext, type IconWeight } from './lib';

export { default as Acorn } from './icons/Acorn';
export { default as AddressBookTabs } from './icons/AddressBookTabs';
export { default as AddressBook } from './icons/AddressBook';
export { default as AirTrafficControl } from './icons/AirTrafficControl';
export { default as AirplaneInFlight } from './icons/AirplaneInFlight';
export { default as AirplaneLanding } from './icons/AirplaneLanding';
export { default as AirplaneTakeoff } from './icons/AirplaneTakeoff';
export { default as AirplaneTaxiing } from './icons/AirplaneTaxiing';
export { default as AirplaneTilt } from './icons/AirplaneTilt';
export { default as Airplane } from './icons/Airplane';
export { default as Airplay } from './icons/Airplay';
export { default as Alarm } from './icons/Alarm';
export { default as Alien } from './icons/Alien';
...

which bundlers should be able to three shake.

On the other hand, imagine you are only using the thin weight in your app for all your icons, and you are using individual imports. The app will still include all other weights because of how the icons are generated at this time.

Maybe in a few weeks/months, this will not be a problem anymore with the enhancements coming to metro bundler via plugins.

Jonnboy91 commented 4 days ago

I looked into this issue a bit more and I noticed that the app crashed with:

FATAL EXCEPTION: OkHttp Dispatcher
Process: com.app.dev, PID: 10267
java.lang.OutOfMemoryError: OutOfMemoryError thrown while trying to throw OutOfMemoryError; no stack trace available

Sometimes it had stack trace:

FATAL EXCEPTION: OkHttp Dispatcher
Process: com.app.dev, PID: 10684
java.lang.OutOfMemoryError: Failed to allocate a 40 byte allocation with 142264 free bytes and 138KB until OOM, max allowed footprint 50331648, growth limit 50331648; failed due to fragmentation (largest possible contiguous allocation 0 bytes)
at okhttp3.internal.ws.WebSocketReader.readHeader(WebSocketReader.kt:134)
at okhttp3.internal.ws.WebSocketReader.processNextFrame(WebSocketReader.kt:102)
at okhttp3.internal.ws.RealWebSocket.loopReader(RealWebSocket.kt:293)
at okhttp3.internal.ws.RealWebSocket$connect$1.onResponse(RealWebSocket.kt:195)
at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:519)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
at java.lang.Thread.run(Thread.java:764)

This happened with the newest version of phosphor-react-native as well, but it started with 2.1.0, since 2.0.0 works and does not crash like this. I noticed that you updated the Expo on that one as well, could it possibly be due to that somehow? 🤔 Obviously it could be something else as well, but just throwing ideas, since I found on StackOverFlow/etc that it could be due to Expo using an out-dated version of okhttp, at least I found few issues with that (older ones that they have then fixed, but maybe similar issue coming up again), but just thinking since the error is very much the same.

We're using a bare version of React Native and no Expo, so it shouldn't come with anything else and it seems to do with the version of this certain dependency 🤔 Obviously my issue is a bit weird that API 31 and over work well even with the newest version, but lower than that crash with 2.1.0 upwards (2.0.0 works).

We haven't come across this bug in production thankfully, since we use the newer version without separately importing the icons. This only seems to happen in our dev environment, maybe it uses a bit more memory/something that then causes this, but the OKHttp Dispatcher is always the error we get 🤔

Just wanted to write my thoughts and findings here, might be no help to you or just rambling, but hopefully something clicks and you can think what it could be 💪

Jonnboy91 commented 4 days ago

Maybe the Newer API version uses different version of something which causes it not to crash with the OKHttp 🤔 Found some place saying this: https://github.com/facebook/react-native/issues/31801