Skip to content

🏗 Migrate amphtml package management from yarn to npm#30694

Merged
rsimha merged 3 commits intoampproject:masterfrom
rsimha:2020-10-02-YarnNpm
Oct 19, 2020
Merged

🏗 Migrate amphtml package management from yarn to npm#30694
rsimha merged 3 commits intoampproject:masterfrom
rsimha:2020-10-02-YarnNpm

Conversation

@rsimha
Copy link
Copy Markdown
Contributor

@rsimha rsimha commented Oct 16, 2020 •
edited
Loading

This PR changes AMP's package manager from yarn to npm. For a detailed discussion of why, see #30518.

PR highlights:

  • Replaces all yarn.lock files in the repo with package-lock.json
  • Updates Travis and GH Actions config files to no longer install / use / cache yarn
  • Updates check-package-manager.js to error out when yarn is used to install dependencies
  • Replaces the yarn integrity check for a stale node_modules with check-dependencies, which runs in O(milliseconds)
  • Updates PR check jobs to error out in the presence of uncommitted changes to package-lock.json
  • Updates validator/ scripts to use npm install instead of yarn
  • Updates all amphtml developer docs

Tested the following workflows:

  • Clean install (works)
  • In-place install after syncing branch (works)
  • Clean / in-place install when required tools are missing (both work, with expected warnings + steps)
  • Install on different OSs (lockfile stable across Linux, macOS, and Windows)
  • Run gulp task without installing packages (prompted to run npm install, yarn used to error out)
  • Spot check changes between lockfiles (caret semver resolution bug present in yarn has been fixed by npm)
  • CI checks (sent out 🏗 Pre-emptive lint fixes + clean up #30700 to pre-emptively fix some lint errors)

Notes for developers (after syncing past this PR):

  • Manually running yarn will now result in an error. Use npm install instead. All other dev commands have been updated, there's nothing new to do.
  • It is recommend to sync your branch and manually run npm install once. (An in-place reinstall is sufficient, no need to delete node_modules/.)
  • For in-flight changes that add / delete / modify a package, rebase and resolve conflicts by deleting yarn.lock and updating package-lock.json (by invoking npm install)
  • On macOS machines running Catalina, node-gyp (part of nodejs) may print verbose logs with expected error messages during first-time install. For amphtml development, they are harmless and can be ignored.

Fixes #30518

@rsimha rsimha self-assigned this Oct 16, 2020
@google-cla google-cla bot added the cla: yes label Oct 16, 2020
@rsimha rsimha marked this pull request as ready for review October 16, 2020 16:53
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Oct 16, 2020

Hey @estherkim! These files were changed:

build-system/tasks/e2e/package-lock.json
build-system/tasks/e2e/yarn.lock

Hey @wassgha! These files were changed:

build-system/tasks/storybook/.gitignore
build-system/tasks/storybook/package-lock.json

Hey @danielrozenberg! These files were changed:

build-system/tasks/visual-diff/index.js
build-system/tasks/visual-diff/package-lock.json
build-system/tasks/visual-diff/yarn.lock

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 16, 2020

This PR is now ready for preliminary review. Once reviewed and approved, I'll coordinate the merging of this PR with internal release system changes.

@rsimha rsimha mentioned this pull request Oct 16, 2020
Merged
rcebulko
rcebulko reviewed Oct 16, 2020
View reviewed changes
kristoferbaxter
kristoferbaxter approved these changes Oct 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@kristoferbaxter kristoferbaxter left a comment

Choose a reason for hiding this comment

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

A few minor comments, but overall looks great!

rsimha
rsimha commented Oct 16, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

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

Excellent comments all around! All addressed, except for the python deprecation and Java removal changes, which are out of scope here.

@MichaelRybak MichaelRybak removed their request for review October 16, 2020 21:43
@rsimha rsimha mentioned this pull request Oct 16, 2020
Merged
1 task
rcebulko
rcebulko approved these changes Oct 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Updated comment re: flamegraph gitignore

@rsimha rsimha requested review from honeybadgerdontcare and jridgewell and removed request for dvoytenko and mrjoro October 17, 2020 00:14
@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 19, 2020

@honeybadgerdontcare for owners approval of changes in validator/
@jridgewell for owners approval of root-level OWNERS and package-lock.json files

@kristoferbaxter
Copy link
Copy Markdown
Contributor

kristoferbaxter commented Oct 19, 2020

https://codelabs.developers.google.com/codelabs/creating-your-first-amp-component/index.html (404) Looks like a failing URL. Checking master.

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 19, 2020 •
edited
Loading

https://codelabs.developers.google.com/codelabs/creating-your-first-amp-component/index.html (404) Looks like a failing URL. Checking master.

We don't run the link-checker for all files, all the time on master because our docs contain thousands of links and it's virtually impossible to check them hundreds of times a day and get all green results. Therefore, we only re-check a documentation file when a PR modifies it.

amphtml/build-system/pr-check/checks.js

Lines 87 to 90 in 4ef897f

// Check document links only for PR builds.
if (buildTargets.has('DOCS')) {
timedExecOrDie('gulp check-links --local_changes');
}

I can fix this URL though. Would https://amp.dev/documentation/guides-and-tutorials/start/create work as a reasonable substitute?

Edit: This guide is a better option. I've sent out #30732 with a fix and will rebase this PR once it's merged.

@rsimha
Copy link
Copy Markdown
Contributor Author

rsimha commented Oct 19, 2020 •
edited
Loading

Latest version of this PR has been verified on Linux, macOS, and Windows. Internal release code has been updated to work through the transition from yarn to npm. Merging this now 🤞.

Edit: So far so good. Release system has been verified to work through this transition. Renovate PRs are updating the correct lockfile.

@rsimha rsimha merged commit b3f7798 into ampproject:master Oct 19, 2020
@rsimha rsimha deleted the 2020-10-02-YarnNpm branch October 19, 2020 21:33
@rsimha rsimha mentioned this pull request Oct 19, 2020
Closed
7 tasks
@samouri
Copy link
Copy Markdown
Member

samouri commented Oct 20, 2020

Congrats on getting this through so quickly! ⭐

@rsimha rsimha mentioned this pull request Oct 27, 2020
Open
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
@rsimha @ed-bird
🏗 Migrate amphtml package management from yarn to npm (ampproje…
47f55e7 
…ct#30694)
@rsimha rsimha mentioned this pull request Dec 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jridgewell jridgewell jridgewell approved these changes

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@estherkim estherkim Awaiting requested review from estherkim

@danielrozenberg danielrozenberg Awaiting requested review from danielrozenberg

+2 more reviewers

@kristoferbaxter kristoferbaxter kristoferbaxter approved these changes

@rcebulko rcebulko rcebulko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rsimha rsimha

Labels

cla: yes PR use: In Beta / Experimental WG: infra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

I2I: Consider switching AMP package management from Yarn to NPM

8 participants

@rsimha @kristoferbaxter @samouri @jridgewell @honeybadgerdontcare @danielrozenberg @rcebulko @ampprojectbot