Enforce the use of yarn for package management by rsimha · Pull Request #11368 · ampproject/amphtml

rsimha · 2017-09-21T03:46:43Z

rsimha · 2017-09-21T13:22:45Z

rsimha · 2017-09-21T13:34:06Z

danielrozenberg · 2017-09-21T14:43:59Z

.travis.yml

  webhooks:
    - http://savage.nonblocking.io:8080/savage/travis
 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash


Not a huge fan of abusing npm to install yarn? 😆

I'm still playing around with this. Now that we recommend the use of yarn 1.0.2 (latest stable) for development, I thought we should use the same versions on Travis too. Let me know if you can think of a more efficient way to install the correct version.

This is one of the recommended ways to install the latest version of Yarn from yarkpkg.com, so might as well stick with it. I'm not advocating for one or the other.

I think the "better alternative" that yarkpkg.com recommends is to use the Debian packages, but it might slow down the installation (because it needs to add the ppa, update the apt packages list, and download and install yarn - as opposed to what you chose which just downloads a simple .sh file and installs it)

I agree that we should choose the fastest install method for Travis, while recommending the "best" install method for local development. It's fine if the two aren't the same.

Any issues with Travis's built-in version of yarn? How long does this take to install?

It's prehistoric (0.27 I think)

Travis uses yarn 0.27.5. Our packages install fine with that version. I thought we should eventually get to the point where everyone writing AMP code should use the same tool chain, but I'll do that in a separate PR.

I've reverted the changes in this file.

danielrozenberg · 2017-09-21T14:44:54Z

build-system/check-package-manager.js

+
+  // If npm is being run, print a message and cause 'npm install' to fail.
+  if (process.env.npm_execpath.indexOf('yarn') === -1) {
+    console/*OK*/.log(red(


Curious, what does the /*OK*/ comment do?

We have a presubmit check that forbids the use of console.log in the AMP runtime. The /*OK*/ bypasses that check, since we know that the build tools are only going to be run in the presence of a console.

danielrozenberg · 2017-09-21T14:46:49Z

build-system/check-package-manager.js

+  // If npm is being run, print a message and cause 'npm install' to fail.
+  if (process.env.npm_execpath.indexOf('yarn') === -1) {
+    console/*OK*/.log(red(
+        '*** The AMP project now uses yarn for package management ***'), '\n');


Remove the "now", this message is gonna stay here forever and the "now-ness" of it will lose relevancy within a couple of weeks

Good point. Removed.

danielrozenberg · 2017-09-21T14:48:24Z

build-system/check-package-manager.js

+    console/*OK*/.log(red(
+        '*** The AMP project now uses yarn for package management ***'), '\n');
+    console/*OK*/.log(yellow('To install all packages:'));
+    console/*OK*/.log(cyan('$'), 'yarn', '\n');


Add a space after the $ (here and below)

console.log automatically adds a space between comma separated arguments.

danielrozenberg · 2017-09-21T14:53:20Z

contributing/getting-started-e2e.md

   ```
-
-   You should see a progress indicator and some messages scrolling by.  You may see some warnings about optional dependencies that are generally safe to ignore.
+   You will see a progress indicator and some messages scrolling by.  You may see some warnings about optional dependencies, which are generally safe to ignore.


nit: one space after the period

This file was originally written with double spacing, so I'm just sticking to the original convention. (We're only changing one word in this line.)

danielrozenberg · 2017-09-21T16:23:36Z

.travis.yml

  webhooks:
    - http://savage.nonblocking.io:8080/savage/travis
 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash


This is one of the recommended ways to install the latest version of Yarn from yarkpkg.com, so might as well stick with it. I'm not advocating for one or the other.

I think the "better alternative" that yarkpkg.com recommends is to use the Debian packages, but it might slow down the installation (because it needs to add the ppa, update the apt packages list, and download and install yarn - as opposed to what you chose which just downloads a simple .sh file and installs it)

danielrozenberg · 2017-09-21T16:25:08Z

contributing/getting-started-e2e.md

-* Install [NodeJS](https://nodejs.org/) version >= 6 (which includes npm)
+* Install the LTS version of [NodeJS](https://nodejs.org/)

-* Install [Yarn](https://yarnpkg.com/) version >= 1.0.2, follow the instructions on the website or install it with npm:


Come to think of it, I think it's important that you leave this comment ("follow the instructions on the website") because each platform has its own "right way" to install Yarn. Using curl should be the last alternative

Fair point. If there's no outright best way to install yarn, we should merely point people to the website. Paging @erwinmombay and @choumx for their opinions.

ok - LGTM on my end, once you get a consensus on this one. Don't forget to update both getting-started-*.md files :)

agree with @danielrozenberg

Fixed. We now just point users to the website.

danielrozenberg · 2017-09-21T16:25:25Z

contributing/getting-started-quick.md

-* Install [NodeJS](https://nodejs.org/) version >= 6
+* Install the LTS version of [NodeJS](https://nodejs.org/)

-* Install [Yarn](https://yarnpkg.com/) version >= 1.0.2, follow the instructions on the website or install it with npm: `npm install -g yarn@latest` (this command might require elevated privileges using `sudo` on some platforms)


Same as above

danielrozenberg · 2017-09-21T16:45:32Z

.travis.yml

  webhooks:
    - http://savage.nonblocking.io:8080/savage/travis
 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash


danielrozenberg · 2017-09-21T16:46:22Z

contributing/getting-started-e2e.md

-* Install [NodeJS](https://nodejs.org/) version >= 6 (which includes npm)
+* Install the LTS version of [NodeJS](https://nodejs.org/)

-* Install [Yarn](https://yarnpkg.com/) version >= 1.0.2, follow the instructions on the website or install it with npm:


ok - LGTM on my end, once you get a consensus on this one. Don't forget to update both getting-started-*.md files :)

dreamofabear · 2017-09-21T16:55:17Z

contributing/getting-started-e2e.md

 amphtml uses Node.js, the Yarn package manager and the Gulp build system to build amphtml and start up a local server that lets you try out your changes.  Installing these and getting amphtml built is straightforward:

-* Install [NodeJS](https://nodejs.org/) version >= 6 (which includes npm)
+* Install the LTS version of [NodeJS](https://nodejs.org/)


Nit: "active LTS" vs "current LTS" (https://github.com/nodejs/Release#release-schedule1) though we should probably just specify Node 6+. Node 8 will be "active LTS" next month.

dreamofabear · 2017-09-21T16:55:33Z

contributing/getting-started-quick.md

 * [Install and set up Git](https://help.github.com/articles/set-up-git/); in the "Authenticating" step of that page use SSH instead of HTTPS

-* Install [NodeJS](https://nodejs.org/) version >= 6
+* Install the LTS version of [NodeJS](https://nodejs.org/)


dreamofabear · 2017-09-21T16:56:11Z

.travis.yml

  webhooks:
    - http://savage.nonblocking.io:8080/savage/travis
 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash


Any issues with Travis's built-in version of yarn? How long does this take to install?

dreamofabear · 2017-09-21T16:56:36Z

.travis.yml

 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash
+  - export PATH="$HOME/.yarn/bin:$PATH"
+  - yarn --version


Should we re-enable yarn caching?

I already did in a previous PR

danielrozenberg · 2017-09-21T17:02:07Z

.travis.yml

  webhooks:
    - http://savage.nonblocking.io:8080/savage/travis
 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash


It's prehistoric (0.27 I think)

danielrozenberg · 2017-09-21T17:02:28Z

.travis.yml

 before_install:
+  - curl -o- -L https://yarnpkg.com/install.sh | bash
+  - export PATH="$HOME/.yarn/bin:$PATH"
+  - yarn --version


I already did in a previous PR

jridgewell · 2017-09-21T21:04:55Z

build-system/check-package-manager.js

+
+  // If yarn is being run, perform a version check and proceed with the install.
+  const yarnVersion = getStdout('yarn --version').trim();
+  if (yarnVersion.startsWith('0.')) {


Is this really necessary?

I'll leave this out for now, and see about doing it at a later point in a different PR. Reverted.

jridgewell · 2017-09-21T21:05:22Z

build-system/check-package-manager.js

+    console/*OK*/.log(cyan('$'), 'yarn remove [package_name]', '\n');
+    console/*OK*/.log(yellow('For detailed instructions, see'),
+        cyan(setupInstructionsUrl), '\n');
+    return -1;


Aren't status codes supposed to be positive?

rsimha · 2017-09-21T21:38:54Z

All comments addressed. PTAL @choumx @jridgewell

jridgewell · 2017-09-21T22:45:25Z

build-system/check-package-manager.js

+  }
+
+  // If yarn is being run, proceed with the install.
+  console/*OK*/.log(green('Detected yarn. Installing packages...'));


No need to log.

I added the logging because it's confusing when preinstall in package.json prints the script name and there's no output to follow.

googlebot added the cla: yes label Sep 21, 2017

rsimha requested review from dreamofabear and jridgewell September 21, 2017 04:54

rsimha self-assigned this Sep 21, 2017

rsimha requested a review from danielrozenberg September 21, 2017 13:33

danielrozenberg reviewed Sep 21, 2017

View reviewed changes

rsimha requested a review from erwinmombay September 21, 2017 16:36

danielrozenberg approved these changes Sep 21, 2017

View reviewed changes

dreamofabear reviewed Sep 21, 2017

View reviewed changes

rsimha added 2 commits September 21, 2017 12:59

Enforce the use of yarn for package management

f1f0968

Address feedback

39ee8c9

danielrozenberg approved these changes Sep 21, 2017

View reviewed changes

Don't install yarn

694ebaf

jridgewell reviewed Sep 21, 2017

View reviewed changes

Address more comments

4a25bf0

rsimha added 2 commits September 21, 2017 18:30

Fix yarn integrity check

2c30407

Fix gulp in pr-check.js

e45cc0d

jridgewell approved these changes Sep 21, 2017

View reviewed changes

rsimha merged commit 4710863 into ampproject:master Sep 21, 2017

rsimha deleted the 2017-09-20-EnforceYarn branch September 21, 2017 22:50

This was referenced Sep 21, 2017

Several AMP dev dependencies are deprecated #11303

Closed

Weekly Status 2017-09-25 #11409

Closed

rsimha mentioned this pull request Dec 21, 2017

Move react-dates, etc. from devDependencies to dependencies #12571

Merged

dmvjs pushed a commit to dmvjs/amphtml that referenced this pull request Jan 31, 2018

Enforce the use of yarn for package management (ampproject#11368)

16bd2d8

Conversation

rsimha commented Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rsimha commented Sep 21, 2017

Uh oh!

rsimha commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danielrozenberg Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsimha commented Sep 21, 2017 •

edited

Loading

danielrozenberg Sep 21, 2017 •

edited

Loading