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

Try using bundledDependencies #41

Merged
merged 1 commit into from
Jul 20, 2016
Merged

Try using bundledDependencies #41

merged 1 commit into from
Jul 20, 2016

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jul 20, 2016

Alternative approach to shrinkwrap, as suggested in #14 (comment) and ionic-team/ionic-cli#636.

@ghost ghost added the CLA Signed label Jul 20, 2016
@gaearon
Copy link
Contributor Author

gaearon commented Jul 20, 2016

Numbers on my machine:

type npm2 (cold) npm2 (warm) npm3 (cold) npm3 (warm)
no bundle 386s 113s 113s 93s
bundle 63s 41s 96s 77s

Bundled deps win in all cases, sometimes significantly (cold npm@2).
I’m going to err on the side of getting it in.

If we have any issues we’ll revert this.

@gaearon gaearon merged commit c11b5c2 into master Jul 20, 2016
This was referenced Jul 20, 2016
@gaearon gaearon deleted the bundled-deps branch July 20, 2016 16:23
@just-boris
Copy link
Contributor

Hello! I have checked bundledDependencies thing again and got quite different results:

React-scripts@0.6.1 installation time

node@v6.7.0 + npm@3.10.3: 99s (99s warm)
node@v4.6.0 + npm@2.15.9: 51s (40s warm)

Then I created a fork, @just-boris/react-scripts, with only one change - no bundled deps, and checked its installation time

node@v6.7.0 + npm@3.10.3: 78s (36s warm)
node@v4.6.0 + npm@2.15.9: 132s (110s warm)

The installation with npm2 is faster with bundled deps, but not for npm3.
Maybe something was changed for the last 3 months, but now bundledDependencies slow down the installation on npm3 for sure

How to reproduce

I have used official node.js docker images for node6+npm3 and node4+npm2. Every command was run 3 times on clean image (no cache).

The command was the following:

time npm install react-scripts --quiet

I have a fork of react-scrips@0.6.1 with removed bundled dependencies. You can find it there. The installation command is the same:

time npm install @just-boris/react-scripts --quiet

Warm time: to find out warm time I installed package one time to fill cache with data, then removed local node_modules folder and run install command again:

time npm install react-scripts --quiet --cache-min=999999

Note that I used cache-min flag to force using the cache as much as possible.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 9, 2016

The main point was to speed up cold on npm 2 because that's going to be first people's experience with CRA for the currently stable Node version.

@just-boris
Copy link
Contributor

Good point but now it seems unfair, when people who want to use the latest version have to suffer, because of some people who still use the previous version and there some tricks for them.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 9, 2016

We plan to drop npm 2 support in the future. When we do so, we can re-evaluate this choice.

@marvinhagemeister
Copy link

Node 6 will be released as the next LTS on October 18 if everything goes to plan. It ships with npm3 by default, so it seems reasonable to focus more on performance with npm3.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 9, 2016

Oh, that's awesome. We'll be happy to switch rightafter that.

@kitze
Copy link

kitze commented Oct 10, 2016

@gaearon will that 👆 fix this 👉 kitze#13 ? As explained here it's a problem with bundledDependencies.

@just-boris
Copy link
Contributor

just-boris commented Oct 11, 2016

Hot off the press: tested with yarn.

react-scripts: 30s
my fork without bundledDependencies: 28s

These numbers are so small, so now I don't actually care about bundledDependencies tradeoffs, I am going to just use Yarn.

@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

Successfully merging this pull request may close these issues.

4 participants