Skip to content
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

Revisting: Remove Haste from Libraries/ and convert to relative requires to shrink bundle sizes #18598

Closed
rdy opened this issue Mar 27, 2018 · 11 comments
Labels
JavaScript Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.

Comments

@rdy
Copy link
Contributor

rdy commented Mar 27, 2018

Hi, I would like to revisit the issue of removing the use of haste modules as described by issue #14196.

I have been able to get a bundle size reduction of react-native using webpack with haul. My approach relies on the modules not using haste so that webpack can properly identify dead code.

I've put together a code example using create-react-native-app ejected you can see it here:
https://github.com/rdy/tree-shaking-example/tree/tree-shaking. I've chosen to use the android platform since I had the build tools on my machine already. You can build the bundles from the package.json scripts bundle:metro and bundle:haul.

The master branch contains a version where I've optimized the bundle size with haul slightly by tweaking the babel output instead of relying on react-native-preset.
Metro bundle: 505K
Haul bundle: 538K

The tree-shaking branch shows the changes from that where I've used my fork of react-native that removes haste as well as haul that doesn't use haste anymore. I've provided a transform for babel-plugin-transform-imports. I'm basically looking at the original main for react-native and supplying the correct import path, this allows webpack to be able to tree shake out the dead code.

Haul bundle with tree shaking: 254K

I've created a series of PDF using source-map-explorer to illustrate that pieces of the bundle are actually being remove correctly by webpack.

metro.pdf
haul-pre-shake.pdf
haul-shaken.pdf

I believe that I could fix the problem with a babel-plugin to rewrite the haste modules to relative requires if necessary but it would be better to not have to maintain a fork or have to transform react-native. Is there a compelling reason to keeping haste? The bundle savings show in my example are pretty compelling.

I was able to do everything with off the shelf components. The only changes I've needed to make was removing haste from react-native and haul so that webpack can trim the dead code.

@rdy rdy changed the title Revisting: Remove Haste from Libraries/ and convert to relative requires Revisting: Remove Haste from Libraries/ and convert to relative requires to shrink bundle sizes Mar 27, 2018
@react-native-bot
Copy link
Collaborator

Thanks for posting this! It looks like your issue may be incomplete. Are all the fields required by the Issue Template filled out?

If you believe your issue contains all the relevant information, let us know in order to have a maintainer remove the No Template label. Thank you for your contributions.

How to ContributeWhat to Expect from Maintainers

@matthargett
Copy link
Contributor

Cc @skevy @rozele @grabbou
A similar change was merged into Expo here: expo#10

@grabbou
Copy link
Contributor

grabbou commented Mar 28, 2018

cc: @thymikee @satya164

@anp anp added JavaScript Type: Discussion Long running discussion. and removed 📋No Template labels Mar 28, 2018
@vincentriemer
Copy link
Contributor

vincentriemer commented Mar 28, 2018

Has there been any progress on the action items at the end of #14196? It still seems like this would break react-native-windows.

@kelset
Copy link
Contributor

kelset commented Mar 29, 2018

Thanks for sharing these informations btw - really looking forward about where removing haste could take RN.

(cc @skevy btw which I seem to recall did some work around this)

@rdy
Copy link
Contributor Author

rdy commented Mar 29, 2018

@vincentriemer I've thought about the comments on how it would work for react-native-windows. In my case I am using webpack so I can do platform mapping directly using alias instead @providesModule. If we wanted we could change it so that when we detect a haste module by its name for example Text could strip out the relative path to Text and perform the same lookup via haste. We'd probably need to make the bundler aware of this difference but it would allow us to mix the relative requires with haste as way to move forward.

@rdy
Copy link
Contributor Author

rdy commented Mar 29, 2018

@kelset yes, this is a continuation of the work that @skevy already did. I used his script to modify a version of react-native so I could rely on webpack to do the dead code elimination with uglify. The proof of concept works so I thought I would get the discussion going again. Since react has moved away from haste I was wondering if react-native could do the same, provided we still maintained a way for out of tree platforms like windows to still work.

@stale
Copy link

stale bot commented Feb 2, 2019

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 2, 2019
@vovkasm
Copy link
Contributor

vovkasm commented Feb 2, 2019

I think someone can create issue and move this discussion to https://github.com/react-native-community/discussions-and-proposals.
Somewhat related discussion is here react-native-community/discussions-and-proposals#49

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 2, 2019
@kelset
Copy link
Contributor

kelset commented Feb 5, 2019

Yeah I feel that you are right @vovkasm - we should move this conversation over there.

@ide
Copy link
Contributor

ide commented Jun 5, 2019

The last of the commits to replace Haste have landed and some follow-up commits to slim down the .flowconfig and remove providesModuleNodeModules from Metro are in flight for RN 0.61.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 5, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Feb 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
JavaScript Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.
Projects
None yet
Development

No branches or pull requests

9 participants