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

Enforce hoisting of some packages to fix giant disk size with Yarn #1397

Closed
gaearon opened this issue Jan 16, 2017 · 18 comments
Closed

Enforce hoisting of some packages to fix giant disk size with Yarn #1397

gaearon opened this issue Jan 16, 2017 · 18 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Jan 16, 2017

Yarn has known issues deduplicating dependencies, sometimes failing to hoist dependencies shared between a ton of packages (such as babel-runtime) and producing installs 5 times bigger than npm: yarnpkg/yarn#2306.

While we could disable Yarn integration (#1390), I'd like to explore an alternative approach. We should be able to determine which package causes most duplication, and then hoist it manually by making it an explicit dependency of react-scripts. This would ensure Yarn deduplicates it. This is a temporary workaround until Yarn actually fixes the algorithm (tracked in yarnpkg/yarn#2306).

Please don't hesitate to help with this. You would need to:

  1. Reproduce the issue with giant (~1GB) install locally with Yarn.
  2. Figure out which dependencies get duplicated. (I don't know the easiest way but you could probably look at Babel packages and see which dependencies end up in node_modules of each of them.)
  3. Add the duplicated dependencies to react-scripts itself to hoist them.
  4. Verify that the issue is resolved.
@droidenator
Copy link

I've been looking to get more involved in open source work. I'd be glad to look into this and see if I can fix it.

@sambanks
Copy link

Yeah me too. I have created a list of duplicate modules installed by create-react-app, but am not sure what to do next.
Can I run create-react-app locally from a cloned copy?
Would it be just a matter of adding the duplicates to the Package.json in react-scripts?

@gaearon
Copy link
Contributor Author

gaearon commented Jan 16, 2017

Can I run create-react-app locally from a cloned copy?

Yes, see instructions here.

Would it be just a matter of adding the duplicates to the Package.json in react-scripts?

Should be.

@sambanks
Copy link

Great I'll give it a crack thanks

@timoxley
Copy link

timoxley commented Jan 16, 2017

According to pkgcount --du --sort size, after a fresh create-react-app, there's 54 copies of core-js@2.4.1 coming in at 373.36 MB:

PACKAGE                                                         #   SIZE
…
json5@0.4.0                                                     1   618.5 kB
acorn@3.3.0                                                     1   659.46 kB
acorn@4.0.4                                                     1   659.46 kB
async@2.1.4                                                     1   679.94 kB
postcss@5.2.10                                                  1   700.42 kB
react@15.4.2                                                    1   708.61 kB
bluebird@3.4.7                                                  1   712.7 kB
pako@0.2.9                                                      1   794.62 kB
source-map@0.5.6                                                1   798.72 kB
hawk@3.1.3                                                      2   868.35 kB
escope@3.6.0                                                    1   909.31 kB
acorn@2.7.0                                                     2   933.89 kB
babel-runtime@6.11.6                                            1   950.27 kB
fbjs@0.8.8                                                      1   983.04 kB
eslint-plugin-jsx-a11y@2.2.3                                    1   1.01 MB
webpack@1.14.0                                                  1   1.15 MB
crypto-browserify@3.3.0                                         1   1.44 MB
sha.js@2.2.6                                                    1   1.54 MB
node-notifier@4.6.1                                             1   1.81 MB
cssstyle@0.2.37                                                 1   1.88 MB
jsdom@9.9.1                                                     1   1.95 MB
regenerator-runtime@0.10.1                                      53  2.17 MB
ajv@4.10.4                                                      1   2.42 MB
eslint@3.8.1                                                    1   2.54 MB
react-dom@15.4.2                                                1   2.56 MB
handlebars@4.0.6                                                1   2.75 MB
es5-ext@0.10.12                                                 1   3.28 MB
core-js@1.2.7                                                   1   4.4 MB
lodash@4.17.4                                                   1   5.08 MB
caniuse-db@1.0.30000611                                         1   8.4 MB
babel-runtime@6.20.0                                            53  52.32 MB
core-js@2.4.1                                                   54  373.36 MB

Other duplicates are far less significant, so deduping core-js should be the key priority.

@hzoo
Copy link

hzoo commented Jan 16, 2017

FYI this is the same issue as npm 2 (why we didn't suggest using npm 2 for Babel 6).

Luckily after dropping Node < 4 we should be able to remove babel-runtime (core-js/regenerator) from babel itself after some work in babel/babel#5118. Otherwise workaround is to make babel-runtime an explicit dependency for now.

(This is because almost all the babel plugin packages have babel-runtime (since we use https://babeljs.io/docs/plugins/transform-runtime/) to support Symbol, Promise, Map/Set, etc which are in Node 4.)

And as for a timeline we are just going to do another 6.x + fixes and 7.x work is already starting https://github.com/babel/babel/wiki/Babel-7 thanks to contributors! The scope of the changes isn't huge so it should be an easy upgrade (at least I intend it to be)

@omrigilad
Copy link

I don't mean to imply that I'm somehow more qualified to prioritize tasks than anyone else, especially @gaearon, but I fail to see why this issue is a big deal.

At the end of the day, disk space is cheap, this has no impact on build size or, if I understand correctly, build time. Its just feels to me like complaints about the size of node_modules are almost like saying "back in my day we used to walk uphill both ways to hand pick minified jQuery plugins and lovingly lay them down in the vendor directory."

Certainly this duplication bug is a problem that yarn should strive to fix, but I don't really see how this matters enough to warrant temporary workarounds in CRA.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 16, 2017

Disk space may be cheaper but it's not universal truth especially for older machines running out of space. If you work on many projects it's annoyingly each each of them takes a whole gig.

It also impacts install times negatively because the linking step takes a long time.

@FWeinb
Copy link

FWeinb commented Jan 16, 2017

I was just tracking down where the core-js@1.2.7 module comes from. It looks like fbjs@0.8.8 referenced by react@15.4.2 is using that. All other package are using core-js@^2.4.0.

I have no idea how feasible it would be to update react to use the next version of fbjs (which is in 0.9.0-alpha.1)

@omrigilad
Copy link

I can't categorically disagree with anything you've said, but I just don't think that annoyance is enough of a reason to "dirty" react-scripts' package.json with incorrect dependencies, let alone disable yarn integration entirely.

@tbillington
Copy link

@mmgm The link time alone is a bit of a killer for me, taking up to 2 minutes for an app that really isn't so big. I'm on a not so powerful laptop but it's still a little crazy.

@hzoo
Copy link

hzoo commented Jan 16, 2017

@FWeinb oh interesting. Sounds like a similar issue with when Babel 6 was still being compiled with Babel 5 causing core-js@1 and core-js@2 deps and fixed with babel/babel#3438 (to have the same core-js/babel-runtime version)

Also FYI: babel/babel#3340 for core-js@1 to core-js@2 update in Babel a while ago.

@FWeinb
Copy link

FWeinb commented Jan 16, 2017

@hzoo
Yes. Having the same babel-runtime should be the second goal. core-js@2 is responsible for a major chunk of memory. So trying to remove that should benefit create-react-app greatly.

@jkimbo
Copy link
Contributor

jkimbo commented Jan 18, 2017

So I've done some investigation into deduping some of the dependencies:

Baseline:

--- /create-react-app/test-1 ---------------------------------------------------------------------------------------
  543.5 MiB [##########] /node_modules
  188.0 KiB [          ]  yarn.lock
   60.0 KiB [          ]  README.md
   32.0 KiB [          ] /public
   24.0 KiB [          ] /src
    4.0 KiB [          ]  package.json
    4.0 KiB [          ]  .gitignore

Adding core-js as a top level dependency reduces the size significantly:

--- a/packages/react-scripts/package.json
+++ b/packages/react-scripts/package.json
@@ -28,6 +28,7 @@
     "babel-eslint": "7.1.1",
     "babel-jest": "18.0.0",
     "babel-loader": "6.2.10",
+    "core-js": "2.4.1",
     "babel-preset-react-app": "^2.0.1",
     "case-sensitive-paths-webpack-plugin": "1.1.4",
     "chalk": "1.1.3",
--- /create-react-app/test-2 ---------------------------------------------------------------------------------------
  194.0 MiB [##########] /node_modules
  188.0 KiB [          ]  yarn.lock
   60.0 KiB [          ]  README.md
   32.0 KiB [          ] /public
   24.0 KiB [          ] /src
    4.0 KiB [          ]  package.json
    4.0 KiB [          ]  .gitignore

Also adding babel-runtime reduces the size even further:

--- a/packages/react-scripts/package.json
+++ b/packages/react-scripts/package.json
@@ -28,6 +28,8 @@
     "babel-eslint": "7.1.1",
     "babel-jest": "18.0.0",
     "babel-loader": "6.2.10",
+    "babel-runtime": "6.20.0",
+    "core-js": "2.4.1",
     "babel-preset-react-app": "^2.0.1",
     "case-sensitive-paths-webpack-plugin": "1.1.4",
     "chalk": "1.1.3",
--- /create-react-app/test-3 ---------------------------------------------------------------------------------------
  142.3 MiB [##########] /node_modules
  188.0 KiB [          ]  yarn.lock
   60.0 KiB [          ]  README.md
   32.0 KiB [          ] /public
   24.0 KiB [          ] /src
    4.0 KiB [          ]  package.json
    4.0 KiB [          ]  .gitignore

Lastly if just babel-runtime (with a carat range) is defined then you get most of the benefits:

--- a/packages/react-scripts/package.json
+++ b/packages/react-scripts/package.json
@@ -28,6 +28,7 @@
     "babel-eslint": "7.1.1",
     "babel-jest": "18.0.0",
     "babel-loader": "6.2.10",
+    "babel-runtime": "^6.20.0",
     "babel-preset-react-app": "^2.0.1",
     "case-sensitive-paths-webpack-plugin": "1.1.4",
     "chalk": "1.1.3",
--- /Users/kimjona/code/create-react-app/test-4 --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

  148.7 MiB [##########] /node_modules
  188.0 KiB [          ]  yarn.lock
   60.0 KiB [          ]  README.md
   32.0 KiB [          ] /public
   24.0 KiB [          ] /src
    4.0 KiB [          ]  package.json
    4.0 KiB [          ]  .gitignore

From this is looks like the quickest solution would be add both babel-runtime and core-js to react-scripts. From this is looks like the quickest solution would be add just babel-runtime to react-scripts. What do you think @gaearon ?

Node version: v6.9.0
Yarn version: v0.18.1
Operating system: Mac OS X 10.11.6

@hzoo
Copy link

hzoo commented Jan 18, 2017

babel-runtime@6.20.0 contains core-js@^2.4.0 so dono if there are other babel-runtime versions as well? regenerator-runtime may help as well but probably not as much (also the other dep in babel-runtime).

Either way we will try to fix this soon for Babel 7.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 23, 2017

From this is looks like the quickest solution would be add just babel-runtime to react-scripts. What do you think @gaearon ?

Sounds awesome, send a PR please.

@jkimbo
Copy link
Contributor

jkimbo commented Jan 24, 2017

@gaearon #1441

@gaearon
Copy link
Contributor Author

gaearon commented Jan 24, 2017

Fixed by #1441, will be out in 0.9.0.

@gaearon gaearon closed this as completed Jan 24, 2017
@gaearon gaearon mentioned this issue Jan 29, 2017
11 tasks
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants