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

tests: use npm ci #4051

Closed
wants to merge 2 commits into from
Closed

tests: use npm ci #4051

wants to merge 2 commits into from

Conversation

DanielRuf
Copy link

npm ci is much faster and is meant for CI. This PR is meant for evaluating the new ci command and testig the performance and compare the logs of Travis CI.

https://docs.npmjs.com/cli/ci
http://blog.npmjs.org/post/171556855892/introducing-npm-ci-for-faster-more-reliable

@mgol
Copy link
Member

mgol commented Apr 20, 2018

Thanks for the PR. However, there are a few problems with this approach:

  1. We removed the lockfile on purpose in Remove package-lock.json? #3792 due to issues with it inside of npm.
  2. We intentionally don't upgrade npm in CI to make sure our builds pass with npm versions included by default in respective Node.js versions as that's what usually people use.
  3. This change doesn't speed up the build which is the main supposed advantage of npm ci vs npm install; compare https://travis-ci.org/jquery/jquery/builds/369211845 & https://travis-ci.org/jquery/jquery/builds/367872474

In light of the above, I don't think we'll want to switch to npm ci.

@mgol mgol closed this Apr 20, 2018
@DanielRuf
Copy link
Author

Right. We just have to trigger a build to compare the results.

Thanks for the feedback.

@DanielRuf DanielRuf deleted the tests/use-npm-ci branch April 21, 2018 08:06
@DanielRuf
Copy link
Author

DanielRuf commented Apr 21, 2018

We removed the lockfile on purpose in #3792 due to issues with it inside of npm.

Which was fixen in npm 5.6 ;-) npm/npm#18135 (comment)
Also see https://yarnpkg.com/blog/2016/11/24/lockfiles-for-all/
https://medium.com/@Quigley_Ja/everything-you-wanted-to-know-about-package-lock-json-b81911aa8ab8

@mgol
Copy link
Member

mgol commented Apr 23, 2018

We're aware of the supposed fixes for that in npm 5.6 but #17722 is still open and there are comments indicating this is still an issue when working cross-platform.

Besides, we haven't had many problems with sub-dependencies misbehaving so it seems our contributors would suffer more from lockfile issues rather than dependencies changing under us.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants