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

Switch package management from npm to yarn #359

Merged
merged 5 commits into from May 16, 2017
Merged

Switch package management from npm to yarn #359

merged 5 commits into from May 16, 2017

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented Apr 18, 2017

This PR switches our package management from npm to Yarn. Yarn uses the same package repository as npm but offers a saner lockfile format (in yarn.lock) which is much easier to diff and merge than the huge JSON blob that npm-shrinkwrap.json is. The other main benefit for CI and local development is that package installation is much faster.

For local development it is still possible to run npm manually to install packages, but you'll only get exactly the same versions of packages that CI / prod builds use if you install with yarn.

Unlike npm, yarn is not bundled with Node so developers will need to install that separately. I have added yarn to the list of prerequisites for developing the client.

@robertknight
Copy link
Member Author

Possibly related to Travis failures: yarnpkg/yarn#1016

@codecov-io
Copy link

codecov-io commented May 4, 2017

Codecov Report

Merging #359 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #359   +/-   ##
=======================================
  Coverage   89.42%   89.42%           
=======================================
  Files         121      121           
  Lines        4785     4785           
  Branches      818      818           
=======================================
  Hits         4279     4279           
  Misses        506      506

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d04180...d8f60dc. Read the comment docs.

@seanh
Copy link
Contributor

seanh commented May 12, 2017

I've rebased this on master locally, tests pass.

If I run yarn install then it changes the yarn.lock file. This happens both before and after the rebasing onto master, same diff:

diff --git a/yarn.lock b/yarn.lock
index 491e5bb5..6c984735 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -280,10 +280,6 @@ ast-types@0.8.12:
   version "0.8.12"
   resolved "https://registry.yarnpkg.com/ast-types/-/ast-types-0.8.12.tgz#a0d90e4351bb887716c83fd637ebf818af4adfcc"
 
-ast-types@0.8.15:
-  version "0.8.15"
-  resolved "https://registry.yarnpkg.com/ast-types/-/ast-types-0.8.15.tgz#8eef0827f04dff0ec8857ba925abe3fea6194e52"
-
 ast-types@0.9.6:
   version "0.9.6"
   resolved "https://registry.yarnpkg.com/ast-types/-/ast-types-0.9.6.tgz#102c9e9e9005d3e7e3829bf0c4fa24ee862ee9b9"
@@ -3849,11 +3845,7 @@ lower-case@^1.1.0, lower-case@^1.1.1, lower-case@^1.1.2:
   version "1.1.4"
   resolved "https://registry.yarnpkg.com/lower-case/-/lower-case-1.1.4.tgz#9a2cabd1b9e8e0ae993a4bf7d5875c39c42e8eac"
 
-lru-cache@2:
-  version "2.7.3"
-  resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-2.7.3.tgz#6d4524e8b955f95d4f5b58851ce21dd72fb4e952"
-
-lru-cache@2.2.x:
+lru-cache@2, lru-cache@2.2.x:
   version "2.2.4"
   resolved "https://registry.yarnpkg.com/lru-cache/-/lru-cache-2.2.4.tgz#6c658619becf14031d0d0b594b16042ce4dc063d"
 
@@ -4895,7 +4887,7 @@ readline2@^1.0.1:
     is-fullwidth-code-point "^1.0.0"
     mute-stream "0.0.5"
 
-recast@0.10.33:
+recast@0.10.33, recast@^0.10.10:
   version "0.10.33"
   resolved "https://registry.yarnpkg.com/recast/-/recast-0.10.33.tgz#942808f7aa016f1fa7142c461d7e5704aaa8d697"
   dependencies:
@@ -4904,15 +4896,6 @@ recast@0.10.33:
     private "~0.1.5"
     source-map "~0.5.0"
 
-recast@^0.10.10:
-  version "0.10.43"
-  resolved "https://registry.yarnpkg.com/recast/-/recast-0.10.43.tgz#b95d50f6d60761a5f6252e15d80678168491ce7f"
-  dependencies:
-    ast-types "0.8.15"
-    esprima-fb "~15001.1001.0-dev-harmony-fb"
-    private "~0.1.5"
-    source-map "~0.5.0"
-
 recast@^0.11.17:
   version "0.11.23"
   resolved "https://registry.yarnpkg.com/recast/-/recast-0.11.23.tgz#451fd3004ab1e4df9b4e4b66376b2a21912462d3"

Should these changes be committed?

@robertknight
Copy link
Member Author

It looks to me like what has happened in that diff is that there was a dependency on (1) recast@0.10.33 (exact version) and (2) recast@^0.10.10 (any 0.10.x version). In the initial yarn.lock recast@0.10.43 was installed to satisfy (2), although (1) matches the version constraint. After your local install, it appears that (2) has been merged into (1) so there will be one less version of recast on disk.

This merge seems like a reasonable thing to do in response to some explicit command, but I wouldn't expect it to happen during an install if there is an existing lockfile since the whole point of a lockfile is to install exactly the described versions. 🤔

I'll investigate.

@seanh
Copy link
Contributor

seanh commented May 12, 2017

Yeah it confused me too, on a brief read of the yarn docs it seems odd for yarn install to modify the lock file. See if you can reproduce it, maybe it's a problem on my end

@seanh
Copy link
Contributor

seanh commented May 12, 2017

Hmmm yarnpkg/yarn#570

@seanh
Copy link
Contributor

seanh commented May 12, 2017

yarn install --pure-lockfile seems to do what we expected yarn install to do (so maybe adding that to Makefile is enough?)

@sean-roberts
Copy link
Contributor

I just read something about a new NPM version supporting most of what yarn offers... I can't find it though

@robertknight
Copy link
Member Author

http://blog.npmjs.org/post/154473364440/npm5-specifications-and-our-rfc-process specifically mentions that npm 5 aims to improve performance and the lockfile format, which are two of the biggest pain points that yarn solves, though far from the only ones. There is a beta which can be installed side-by-side with npm stable. Good to see it happening, though in some light testing the experience (with the client repo at least) is still nowhere near as good as yarn.

Yarn provides much faster package installation, better resilience to npm
registry connection issues and most importantly for us, a much better
lockfile format.

This commit:

1. Replaces the npm shrinkwrap with the yarn lockfile using the
   following steps:

   1. Checkout latest version of master

   2. Remove node_modules folder and re-run `npm install`

   3. Run `yarn import` to generate a lockfile

2. Modifies the Makefile, Jenkinsfile, package.json and .travis.yml
   scripts to run `yarn` instead of `npm`.
There were a couple of issues with the initial Yarn lockfile created via
`yarn import`:

1. `yarn check` reported several inconsistencies.

2. `phantomjs-prebuilt` needed to be upgraded to resolve a compatibility
   issue with Yarn.

This commit resolves these issues by just removing and recreating the
Yarn lockfile. In the process all dependencies have been upgraded to the
latest versions compatible with the constraints in package.json.
Package installation fails with Node v6.2 due to an incompatible
"check-dependencies" package. See
https://travis-ci.org/hypothesis/client/jobs/228937886
Update lockfile with result of running `yarn install` with yarn v0.23.2.
@robertknight
Copy link
Member Author

From that lengthy thread above, I think yarnpkg/yarn#570 (comment) is the best explanation of the intended behaviour.

The TL;DR is that conceptually, yarn install is like a memoized function that takes package.json as input and generates node_modules as output, caching the result in yarn.lock. On that basis, yarn.lock should only be modified during yarn install if package.json has changed since the lockfile was last built. That thread documents a few examples where that didn't happen at the time. In this case, I think the issue we're seeing is yarnpkg/yarn#3315

I ran yarn again locally and got the same output as seanh above. This is a mild annoyance but I don't think it shows a fundamental problem with the tool or negates the benefits of the change.

@seanh
Copy link
Contributor

seanh commented May 16, 2017

Alright, lets give it a try and see how it goes

@seanh seanh merged commit 5e284a9 into master May 16, 2017
@seanh seanh deleted the yarn branch May 16, 2017 16:45
@seanh
Copy link
Contributor

seanh commented May 16, 2017

@robertknight Would you mind sending an email to the team, informing of this change and what they need to do to their dev envs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants