software-mansion / react-native-svg

SVG library for React Native, React Native Web, and plain React web projects.
MIT License
7.43k stars 1.12k forks source link

Bundle size css-tree #1264

Closed wilau2 closed 6 days ago

wilau2 commented 4 years ago

The bundle size passed from 55kb to 173kb updating the library from 9.11.1 to 11.0.1.

Should css-tree be an optional dependency?

image

wilau2 commented 4 years ago

We were doing good without css-tree.

1166

https://github.com/react-native-community/react-native-svg/commit/957914d59b27e22121d13f13cb54a051b893b446

msand commented 4 years ago

Feel free to make a pr to fix this

msand commented 4 years ago

No response, closing.

todorone commented 4 years ago

@wilau2 How did You manage to get rid of it?

@msand Just checked, css-tree adds almost 100Kb of result React Native bundle which may result in startup time degradation/bigger bundles for OTA updates. I'm ready to work on PR, just need to make sure what will be the way to rectify it...

msand commented 4 years ago

I suspect the easiest way is to use patch-package to remove/comment out these lines of code such that metro doesn't attempt to bundle them:

https://github.com/react-native-community/react-native-svg/blob/e66e87a5b5c090509d5e2127237963f83e60f1e9/src/ReactNativeSVG.ts#L27-L34

https://github.com/react-native-community/react-native-svg/blob/e66e87a5b5c090509d5e2127237963f83e60f1e9/src/ReactNativeSVG.ts#L90-L97

Alternatively, to set up some replacement for css-select and css-tree, assuming you're not using the css functionality, you should be able to replace them with no-op functions. Alternatively, it might be possible to use random access modules, but that might require additional configuration, and would thus limit usability, haven't looked into it. At least I haven't found any way to get metro to work without attempting to load those two, even if the modules that use them aren't used anywhere. Maybe it's possible to configure somehow, or maybe haul/webpack supports it.

todorone commented 4 years ago

@msand Thanks for help and quick response. Yeah, just commenting those lines with patch-package saved almost 250Kb of bundle, according to react-native-bundle-visualizer.

Bundle is 2.1 MB in size (--- has decreased with 246003 bytes since last run)

It's ridiculous, how much trash sometimes we bundle into our apps, and then claiming that react native is slow...😸

msand commented 4 years ago

Yeah, would be great to get proper tree-shaking at both development and in production builds. Would you mind looking into how to achieve this?

todorone commented 4 years ago

@msand I'll try to investigate

msand commented 4 years ago

This might be relevant https://github.com/facebook/metro/pull/511

msand commented 4 years ago

https://github.com/msand/react-native-svg-e2e/pull/31

todorone commented 4 years ago

Hey @msand Thanks for the links provided. Now metro-babel-preset allows conditional importing, but tree shaking is not implemented and not planned, unfortunately - https://github.com/facebook/metro/issues/227

The only solution for now is to migrate to monorepo and multiple packages like react-navigation did. We can have something like @react-native-svg/core and @react-native-svg/css. I know it will introduce a bit of overhead in maintaining, but it's the only viable solution for now. Otherwise currently those couple components that were introduced recently take 246003 bytes, which is 2x more than the all the rest, which is ridiculous...😆

Wdyt?

UPDATE: Oh, totally forgot about bundling separate chunks in separate files. What do we need:

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You may also mark this issue as a "discussion" and I will leave this open.

todorone commented 4 years ago

@msand So the solution will be simple(and can be reused with any of big planned features like filters):

This is common practice used by huge libraries like Material UI, the only downside is this is a breaking change, but it's not a big deal if we will document it in release note like: import { SvgCss } from 'react-native-svg' => import { SvgCss } from 'react-native-svg/css'

If You give me a green flag, I'll prepare PR...

msand commented 4 years ago

@todorone Sounds great. I haven't had time to work on open-source lately, will probably be able to again, starting july or something. Might be good to have a release in between, with a deprecation warning. At least nicer for users if the latest major version, before the breaking change, warns about upcoming api changes and ideally has a codemod available.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You may also mark this issue as a "discussion" and I will leave this open.

todorone commented 4 years ago

Thanks for ping, stale bot, i'll try to prepare PR tomorrow...

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. You may also mark this issue as a "discussion" and I will leave this open.

wilau2 commented 3 years ago

A PR is open mister stale bot !

chang-ke commented 3 years ago

tree shaking support https://github.com/facebook/metro/issues/632

bohdanprog commented 1 month ago

@wilau2 Can we close that issue?

wilau2 commented 1 month ago

Feel free to close in not working with react native anymore

bohdanprog commented 1 month ago

@wilau2, has that issue been resolved?