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.

Sign up for GitHub

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

Jump to bottom

Replace npm with yarn #11332

Merged
merged 9 commits into from
Sep 20, 2017
Merged

Replace npm with yarn #11332

merged 9 commits into from
Sep 20, 2017

Conversation

danielrozenberg
Copy link
Member

@danielrozenberg danielrozenberg commented Sep 19, 2017 •
edited by rsimha
Loading

Partially fixes #11303
Fixes #11278

rsimha
rsimha reviewed Sep 19, 2017
View reviewed changes
package.json Outdated
@@ -5,8 +5,8 @@
"description": "Fastish HTML",
"main": "index.js",
"engines": {
"node": "^4.7.0",
"npm": "^5.0.0"
"node": ">=4.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you should remove the engines section from this file. We specify the node version that Travis must use via .travis.yml, and Travis' built in version of yarn used to work fine as it was.

Copy link
Contributor

Choose a reason for hiding this comment

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

See travis-ci/travis-ci#4653 (comment) for an explanation of why Travis ignores the versions of node and npm specified in package.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

No change, Travis still fails on Yarn

@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017 •
edited
Loading

@danielrozenberg, looks like the error in your latest commit is due to yarn complaining that it needs node 6 in order to install punycode 2.0. It turns out that punycode 2.0 was introduced as a dependency of browserify, which was upgraded as part of #9775.

And as luck would have it, @erwinmombay merged #9775 just after you had removed yarn, so this problem became yours instead of his :/

I encountered a similar version incompatibility in #10671, and at that time, I could get away with it. This time, I'd imagine you'd need to downgrade browserify to a version that uses a version of punycode that is compatible with node 4, or bite the bullet and upgrade to node 6 on Travis.

Dependencies are hard :(

@rsimha
Copy link
Contributor

rsimha commented Sep 19, 2017 •
edited
Loading

Update after discussion with the team: Let's move to node 6 on Travis. I'll follow up this PR with one that introduces a check to make sure yarn is used by all committers to install dependencies, and not npm.

@rsimha rsimha mentioned this pull request Sep 20, 2017
Closed
@rsimha
Copy link
Contributor

rsimha commented Sep 20, 2017

@danielrozenberg FYI, I just tested your PR with node 6, and the yarn install was successful.

@danielrozenberg
Copy link
Member Author

Aren't we forcing everyone to update to node 6 then? If yarn can't build amphtml with node 4.8, then changing the .travis.yml isn't going to be enough

@rsimha
Copy link
Contributor

rsimha commented Sep 20, 2017

Yes, everyone should upgrade to node 6. From yesterday's discussion, here's what needs to happen:

  1. Update the version of node used on Travis from 4.8 to 6 (this PR)
  2. Update the instructions to reflect that everyone must now use node 6 for local development (this PR)
  3. Add a check to make sure that those who freshly clone the AMP repo use yarn to install their dependencies, and not npm (I'll send out a separate PR)
  4. Send out a PSA to the team

@rsimha rsimha mentioned this pull request Sep 20, 2017
Closed
@danielrozenberg
Copy link
Member Author

Done 1 & 2, PTAL

rsimha
rsimha approved these changes Sep 20, 2017
View reviewed changes
Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix!

build-system/SERVING.md Outdated
- gulp (installed globally)
- java 8
- yarn 1.0.2+ (see https://yarnpkg.com/)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Move this up the list, just below node, to reflect the order in which they are likely to be installed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

build-system/SERVING.md

## Steps
```bash
git clone https://github.com/ampproject/amphtml.git
cd amphtml
# Checkout a tag
git checkout 123456789
npm install
yarn
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be yarn install? Or is yarn sufficient if there's a yarn.lock?

Copy link
Member Author

Choose a reason for hiding this comment

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

"yarn" == "yarn install", regardless of yarn.lock or not

https://yarnpkg.com/lang/en/docs/cli/install/

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks for checking!

rsimha
rsimha reviewed Sep 20, 2017
View reviewed changes
contributing/getting-started-e2e.md
```

The preceding command might require elevated privileges using `sudo` on some platforms.

* In your local repository directory (e.g. `~/src/ampproject/amphtml`), install the packages that AMP uses by running
```
npm install
yarn
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here, re: yarn vs. yarn install.

Copy link
Member Author

Choose a reason for hiding this comment

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

ACK

@rsimha rsimha mentioned this pull request Sep 20, 2017
Merged
@rsimha rsimha merged commit fe4c45b into ampproject:master Sep 20, 2017
@rsimha rsimha changed the title Replace npm with yarn #11278 Replace npm with yarn Sep 20, 2017
@danielrozenberg danielrozenberg deleted the re-yarn branch September 20, 2017 20:44
@rsimha rsimha mentioned this pull request Sep 21, 2017
Closed
@erwinmombay
Copy link
Member

fyi @dknecht we bumped node requirement to >6 and that yarn should be used to install node deps

dmvjs pushed a commit to dmvjs/amphtml that referenced this pull request Jan 31, 2018
@danielrozenberg @dmvjs
Replace npm with yarn ampproject#11278 (ampproject#11332)
4d413ea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsimha rsimha rsimha approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Several AMP dev dependencies are deprecated Use one package manager, not two
4 participants
@danielrozenberg @rsimha @erwinmombay @googlebot