Skip to content

Replace npm with yarn#11332

Merged
rsimha merged 9 commits intoampproject:masterfrom
danielrozenberg:re-yarn
Sep 20, 2017
Merged

Replace npm with yarn#11332
rsimha merged 9 commits intoampproject:masterfrom
danielrozenberg:re-yarn

Conversation

@danielrozenberg
Copy link
Copy Markdown
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
"engines": {
"node": "^4.7.0",
"npm": "^5.0.0"
"node": ">=4.8.0",
Copy link
Copy Markdown
Contributor

@rsimha rsimha Sep 19, 2017

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
Copy Markdown
Contributor

@rsimha rsimha Sep 19, 2017

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
Copy Markdown
Member Author

@danielrozenberg danielrozenberg Sep 19, 2017

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

danielrozenberg commented Sep 20, 2017

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
Copy Markdown
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
Copy Markdown
Member Author

danielrozenberg commented Sep 20, 2017

Done 1 & 2, PTAL

rsimha
rsimha approved these changes Sep 20, 2017
View reviewed changes
Copy link
Copy Markdown
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
- node 6+
- gulp (installed globally)
- java 8
- yarn 1.0.2+ (see https://yarnpkg.com/)
Copy link
Copy Markdown
Contributor

@rsimha rsimha Sep 20, 2017

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
Copy Markdown
Member Author

@danielrozenberg danielrozenberg Sep 20, 2017

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
# Checkout a tag
git checkout 123456789
npm install
yarn
Copy link
Copy Markdown
Contributor

@rsimha rsimha Sep 20, 2017

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
Copy Markdown
Member Author

@danielrozenberg danielrozenberg Sep 20, 2017

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
Copy Markdown
Contributor

@rsimha rsimha Sep 20, 2017

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
* In your local repository directory (e.g. `~/src/ampproject/amphtml`), install the packages that AMP uses by running
```
npm install
yarn
Copy link
Copy Markdown
Contributor

@rsimha rsimha Sep 20, 2017

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
Copy Markdown
Member Author

@danielrozenberg danielrozenberg Sep 20, 2017

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
Copy Markdown
Member

erwinmombay commented Sep 27, 2017

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
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

1 more reviewer

@rsimha rsimha rsimha approved these changes

Reviewers whose approvals may not affect merge requirements

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