Remove dependencies on yarn by danielrozenberg · Pull Request #11279 · ampproject/amphtml

danielrozenberg · 2017-09-12T20:23:40Z

dreamofabear

Let's also add the equivalent package-lock.json file with the same versions in the current yarn.lock.

dreamofabear · 2017-09-12T20:30:36Z

Related: #2771

rsimha · 2017-09-12T21:15:46Z

build-system/pr-check.js

    process.exit(1);
  }

-  // Make sure changes to package.json also update yarn.lock.


Instead of removing this check, maybe repurpose it to make sure that package.json and package-lock.json are updated in lock step? (Heh!)

Fair enough, PTAL

rsimha · 2017-09-12T21:17:10Z

contributing/getting-started-e2e.md


   ```
-   yarn global add gulp
+   sudo npm install -g gulp


Is sudo required on all platforms? If not, don't include it here.

Removed sudo from the command and added a comment below to clarify

rsimha · 2017-09-12T21:17:56Z

contributing/getting-started-quick.md


 # Build AMP & run a local server
-* Make sure you have the latest packages (after you pull): `yarn`
+* Make sure you have the latest packages (after you pull): `npm install`


Another useful command is to run npm prune to make sure unnecessary dependencies are removed.

Added it below in the "Create commits to contain your changes" section

rsimha · 2017-09-12T21:18:47Z

contributing/getting-started-quick.md

+* Install [npm](https://www.npmjs.com/get-npm)`

-* Install Gulp by running `yarn global add gulp`
+* Install Gulp by running `sudo npm install -g gulp`


Same comment re: sudo. Many of our developers are on Mac OS and probably don't need to use it.

rsimha · 2017-09-12T21:23:28Z

contributing/getting-started-e2e.md

-* Install [yarn](https://yarnpkg.com/en/docs/install)
+* Install [npm](https://www.npmjs.com/get-npm)
+
+* In your local repository directory (e.g. `~/src/ampproject/amphtml`), install the packages that AMP uses by running `npm install`


Make this consistent with what's below re: sudo. Also, for commands that will likely be copy-pasted by new developers, let's put them on a separate line, like it previously was.

…pr-check.js

erwinmombay · 2017-09-13T15:29:58Z

contributing/getting-started-quick.md

 # Create commits to contain your changes

 * Edit files in your favorite editor
+* If you edited `package.json` run `npm prune && npm install` to generated an updated `package-lock.json` file


to generate*

erwinmombay · 2017-09-13T15:30:13Z

contributing/getting-started-quick.md

+* Install [npm](https://www.npmjs.com/get-npm)`

-* Install Gulp by running `yarn global add gulp`
+* Install Gulp by running `npm install -g gulp` (on some platforms this command might required elevated privileges using `sudo`)


might require*

Damn these fingers and their muscle memory 😝

rsimha

LGTM after the typos pointed out by Erwin are fixed.

dreamofabear · 2017-09-13T15:39:08Z

Per the group chat, let's also bump our min node version in package.json to 4.7 and document somewhere that NPM >= 5 is required (perhaps as part of the warning in pr-check.js).

rsimha · 2017-09-13T15:41:31Z

If you look at the Travis raw logs from your PR checks, you'll see:

$ node --version
v4.8.4
$ npm --version
2.15.11
$ nvm --version
0.33.4

This can be updated via the mechanism described in travis-ci/travis-ci#4653

rsimha · 2017-09-13T15:54:24Z

With the latest changes in commit 4dd5998, I still see an old version of npm being used. https://travis-ci.org/ampproject/amphtml/jobs/275099764

I don't think adding an npm version to engines in .travis.yml is meant to work. I was referring to the workaround in travis-ci/travis-ci#4653 (comment).

I'll let @choumx comment on whether that will address the versioning concern he raised.

dreamofabear

I haven't checked that the versions in the new package-lock.json are the same as the versions in the deleted yarn.lock, but I assume this is ok. Right, @erwinmombay? 😄

dreamofabear · 2017-09-13T15:58:46Z

package.json

  "engines": {
-    "node": "^4.0.0"
+    "node": "^4.7.0",
+    "npm": "^5"


Nit: 5.0.0 to be consistent with semantic versioning.

dreamofabear · 2017-09-13T15:58:50Z

contributing/getting-started-e2e.md

+* Install [NodeJS](https://nodejs.org/) version >= 4.7 (which includes npm)

-* Install [npm](https://www.npmjs.com/get-npm)
+* Install [npm](https://www.npmjs.com/get-npm) version >= 5


Nit: Since installing node also installs npm, this should probably be "upgrade npm to version >= 5".

dreamofabear · 2017-09-13T16:07:34Z

The last thing per our conversation with Raghu is to compare build timings for npm 5 vs yarn on Travis. Hopefully it's a negligible delta (or faster!).

…nd *upgrade* npm, since npm comes with NodeJS

… semantic versioning

danielrozenberg

Doesn't seem to have an effect on the running time of Travis

danielrozenberg · 2017-09-13T16:54:13Z

contributing/getting-started-e2e.md

+* Install [NodeJS](https://nodejs.org/) version >= 4.7 (which includes npm)

-* Install [npm](https://www.npmjs.com/get-npm)
+* Install [npm](https://www.npmjs.com/get-npm) version >= 5


danielrozenberg · 2017-09-13T16:54:22Z

package.json

  "engines": {
-    "node": "^4.0.0"
+    "node": "^4.7.0",
+    "npm": "^5"


rsimha · 2017-09-13T17:05:47Z

contributing/getting-started-quick.md

+* Install [NodeJS](https://nodejs.org/) version >= 4.7

-* Install [yarn](https://yarnpkg.com/en/docs/install)
+* If the version of [npm](https://www.npmjs.com/)` that was installed with NodeJS is lower than version 5, upgrade it by running `npm install -g npm@latest` (on some platforms this command might require elevated privileges using `sudo`)


This line is badly formatted due to a stray backtick. (You can click the "view" button above this file in the "files changed" view to see what it will actually look like.)

rsimha · 2017-09-13T17:06:20Z

contributing/getting-started-quick.md

+* Install [NodeJS](https://nodejs.org/) version >= 4.7

-* Install [yarn](https://yarnpkg.com/en/docs/install)
+* If the version of [npm](https://www.npmjs.com/)` that was installed with NodeJS is lower than version 5, upgrade it by running `npm install -g npm@latest` (on some platforms this command might require elevated privileges using `sudo`)


nit: Add a comma after "on some platforms"

Changed the voice of this sentence:

on some platforms this command might require elevated privileges using sudo

this command might require elevated privileges using sudo on some platforms

rsimha · 2017-09-13T17:07:02Z

contributing/getting-started-quick.md

 # Create commits to contain your changes

 * Edit files in your favorite editor
+* If you edited `package.json` run `npm prune && npm install` to generate an updated `package-lock.json` file


nit: Add a comma after "If you edited package.json"

rsimha · 2017-09-13T17:15:54Z

Re: build times, yarn used to take ~8 s, while npm install took ~16 s on the Travis run for this PR. This shouldn't be of huge concern, besides I'm sure it'll get faster in later runs with caching.

dreamofabear · 2017-09-13T17:18:04Z

👍 I was about to post that it's about 17 seconds slower including npm upgrade.

Travis had yarn caching, let's investigate separately how to do that for npm.

rsimha · 2017-09-13T17:21:51Z

@choumx, we already cache node_modules. However, https://stackoverflow.com/questions/42521884/should-i-have-travis-cache-node-modules-or-home-npm recommends caching $HOME/.npm instead of node_modules.

@danielrozenberg, this can go in as a separate PR if you'd like :)

danielrozenberg · 2017-09-13T17:23:52Z

Re: yarn vs npm caching - the Travis documentation you linked to says that the caching only applies if you set node version > 6, so the difference in timing might be unrelated. The node_modules is cached in .travis.yml, not sure how much it affects future PRs

Remove dependencies on yarn

676795d

googlebot added the cla: yes label Sep 12, 2017

dreamofabear requested changes Sep 12, 2017

View reviewed changes

dreamofabear requested a review from rsimha September 12, 2017 20:28

ampprojectbot added this to the Pending Triage milestone Sep 12, 2017

Add autogenerated file package-lock.json

5a162a5

rsimha suggested changes Sep 12, 2017

View reviewed changes

Daniel Rozenberg added 2 commits September 13, 2017 11:18

Fix for comments by @rsimha-amp

be35d96

Add comment to the updated package.json → package-lock.json check in …

3381e8c

…pr-check.js

erwinmombay reviewed Sep 13, 2017

View reviewed changes

rsimha approved these changes Sep 13, 2017

View reviewed changes

:%s/(generate|require)d/\1/g

b480f71

Specify minimum required versions of node and npm

4dd5998

dreamofabear approved these changes Sep 13, 2017

View reviewed changes

Daniel Rozenberg added 3 commits September 13, 2017 12:13

Set minimum versions of node_js and npm in .travis.yml

eda21e9

Detailed the instructions to explain that one should install NodeJS a…

6ba5d10

…nd *upgrade* npm, since npm comes with NodeJS

Set minimum npm version to ^5.0.0 instead of ^5 to be consistent with…

155e3f5

… semantic versioning

danielrozenberg commented Sep 13, 2017

View reviewed changes

rsimha suggested changes Sep 13, 2017

View reviewed changes

Fine tune documentation

ff540b9

Fine tune documentation

16e96d3

rsimha approved these changes Sep 13, 2017

View reviewed changes

rsimha merged commit 2ebe918 into ampproject:master Sep 13, 2017

danielrozenberg deleted the un-yarn branch September 13, 2017 18:48

This was referenced Sep 15, 2017

upgrade to babel 6 #9775

Merged

Delete yarn.lock and update package-lock.json #11302

Merged

Conversation

danielrozenberg commented Sep 12, 2017

Uh oh!

dreamofabear left a comment

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Sep 12, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Sep 13, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

dreamofabear left a comment

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!

dreamofabear commented Sep 13, 2017

Uh oh!

danielrozenberg left a comment

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 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

rsimha commented Sep 13, 2017 •

edited

Loading

rsimha commented Sep 13, 2017 •

edited

Loading

rsimha commented Sep 13, 2017 •

edited

Loading