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

Enable package lock to reduce duration & CPU usage of "npm install" 🏎🌱 #2558

Merged
merged 3 commits into from
Mar 8, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Mar 8, 2019

Historically, we were avoiding package locks to ensure our CI builds are using the latest version of all dependencies, the same versions that our consumers would receive when running npm install in their projects.

Now that we have Greenkeeper enabled to detect new versions in our dependency tree and semi-automatically update our package.json & package-lock.json files, I feel it's safe to use package-lock feature.

This pull request enables package-locks in the entire monorepo in the first commit, and adds individual package-lock.json files in the second commit - I hope this will make the change easier to review. The third commit removes Node.js version 10.0.0 from the build matrix on Windows, because npm ci is not available there. (We cannot upgrade to a newer 10.x version - see #1431.)

Note: To fully leverage the power of package locks, the CI is using npm ci instead of npm install now. Similarly, lerna is configured to use npm ci when installing dependencies in individual monorepo packages.

As a result, the time to install dependencies was cut by half - from 1m44s to 0m52s on my machine. Not only the installation is faster, it's also burning less CPU cycles (and energy).

before

# clean all dependencies
$ npm run clean:lerna && rm -rf node_modules 
# measure how long it takes to install them
$ time npm i

real  1m43.710s
user  6m44.097s
sys   1m37.536s

after

# clean all dependencies
$ npm run clean:lerna && rm -rf node_modules 
# measure how long it takes to install them using "npm ci"
$ time npm ci

real  0m51.985s
user  3m56.915s
sys   1m18.982s

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@bajtos bajtos added the Internal Tooling Issues related to our tooling and monorepo infrastructore label Mar 8, 2019
@bajtos bajtos added this to the March 2019 milestone milestone Mar 8, 2019
@bajtos bajtos self-assigned this Mar 8, 2019
@bajtos bajtos requested review from raymondfeng and a team March 8, 2019 08:31
@@ -1 +1 @@
package-lock=false
package-lock=true
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am intentionally keeping .npmrc and forcing package-lock on all loopback-next developers.

Personally, I have a global .npmrc file setting package-lock to false, because that's what I need when working on LB3 repositories. If there was no .npmrc file in loopback-next, then I would not be using package-lock functionality because of my global config.

IMO, it's best to not make any assumptions about the global npm configuration and explicitly tell npm that we want to use package-lock files.

@bajtos
Copy link
Member Author

bajtos commented Mar 8, 2019

Bummer, Node.js 10.0.0 used on AppVeyor does not support npm ci. I think it's best to remove 10.0.0 from the build matrix on Windows. The version 10.0.0 is very outdated when compared to current 10.x LTS versions.

@bajtos
Copy link
Member Author

bajtos commented Mar 8, 2019

@raymondfeng if the patch LGTY and there are no objections from other reviewers, then feel free to land it yourself. The sooner we speed up our CI builds, the better for us and the environment too.

Copy link
Contributor

@marioestradarosa marioestradarosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 .
@bajtos , as a side note, npm ci automatically do the rm -rf node_modules for us. According to its documentation. Also it mentions that it is faster because it skips certain user-oriented features.

@bajtos bajtos added the feature label Mar 8, 2019
@bajtos bajtos changed the title Enable package lock Enable package lock 🏎🌱 Mar 8, 2019
@bajtos bajtos changed the title Enable package lock 🏎🌱 Enable package lock to reduce duration & CPU usage of "npm install" 🏎🌱 Mar 8, 2019
@raymondfeng
Copy link
Contributor

We need to try out a few things:

  1. Make sure package-lock.json works nicely with lerna including our release process.
  2. Figure out the impact of released @loopback/* modules with package-lock.json, such as:

@raymondfeng
Copy link
Contributor

raymondfeng commented Mar 8, 2019

I'm seeing a diff in package-lock.json such as:

-          "resolved": "https://registry.npmjs.org/callsites/-/callsites-2.0.0.tgz",
+          "resolved": "http://registry.npmjs.org/callsites/-/callsites-2.0.0.tgz",

npm config get registry prints https://registry.npmjs.org/.

I had to do the following to resolve the problem:

  1. npm cache clean --force
  2. rm -rf node_modules
  3. npm i

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👏

@@ -0,0 +1,361 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's surprising that I don't see entries for @loopback/* dependencies in the generated package-lock.json. Is it by design as they are symbolically linked? If so, what's the impact for published modules?

@raymondfeng
Copy link
Contributor

Figure out the impact of released @loopback/* modules with package-lock.json

I just realized that package-lock.json won't be published to npm.

  • package-lock.json is never published to npm, whereas npm-shrinkwrap is by default
  • package-lock.json files that are not in the top-level package are ignored, but shrinkwrap files belonging to dependencies are respected

Basically, package-lock.json is used to ensure repeatable build while shrinkwrap.json is used for repeatable installation/runtime.

bajtos added 3 commits March 8, 2019 09:09
Turn on package-lock, we will rely on Greenkeeper to let us know
when a new version of our (deep) dependency was published.

Update CI configuration to run `npm ci` instead of `npm i`.

This change speeds up the installation from approx. 1m44s to 0m52s,
i.e down to a half.
The version 10.0.0 is way too outdated, it makes little sense to test
it. Unfortunately we cannot upgrade to a newer 10.x version because
the CLI tests freeze on Windows :(

Also, Node.js 10.0.0 is shipping an outdated npm version that does not
support `npm ci` command that we are using to speed up install times.
@raymondfeng raymondfeng force-pushed the enable-package-lock branch from 74a812a to 5b3757f Compare March 8, 2019 17:11
@raymondfeng raymondfeng merged commit d4eb70c into master Mar 8, 2019
@raymondfeng raymondfeng deleted the enable-package-lock branch March 8, 2019 17:35
@bajtos
Copy link
Member Author

bajtos commented Mar 11, 2019

Basically, package-lock.json is used to ensure repeatable build while shrinkwrap.json is used for repeatable installation/runtime.

Exactly 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants