Yarn & Babel 7 #753

jimbo · 2019-01-14T09:45:52Z

This PR is a:

Summary

When this pull request is merged, it will migrate from NPM to Yarn. In the process, it will also upgrade the repo to Babel 7.2.0.

Additional Information

Getting Started

# install yarn via homebrew
brew install yarn

# install packages
yarn

# build peregrine and pwa-buildpack
yarn build:dev

# watch venia and start dev server
yarn watch:venia

Motivation

Package management is currently one of our biggest pain points. Installing and building packages is slow, and our current workflow requires us to reinstall node_modules and rebuild every package on a regular basis. These operations should be faster and used less frequently.

Furthermore, as a monorepo, pwa-studio has a complex dependency tree. Our packages share a number of dependencies, and some of them depend on other packages within the monorepo. We currently use lerna to hoist dependencies into the monorepo root, but lerna isn't particularly fast. Worse, each package in this repository should list its own dependencies accurately, but we've blurred the lines between dependencies and devDependencies just to enable lerna to inform us about mismatched dependencies. As a result, most of our package.json files are missing dependencies, and our root package.json file contains unused dependencies.

Yarn

Fortunately, yarn can help us solve both problems.

Hoisting

Yarn Workspaces directly address our monorepo problems. When pwa-studio is configured as a monorepo with workspaces, running yarn install from the root will install, hoist, and dedupe the dependencies listed in each workspace's package.json file, plus any dependencies listed in the root package.json file. We no longer need to manually update the root when a dependency changes within a workspace.

Workspaces also simplify our package scripts:

Command	Effect
`yarn workspaces run clean`	run the `clean` script within each package
`yarn workspace @magento/venia-concept run watch`	run the `watch` script within `packages/venia-concept`

Speed

But most importantly, yarn is fast.

Operation	npm	yarn
`clean`	0:27	0:03
`install`	0:51	0:16
`build`	0:25	0:06
Total	1:43	0:25

vercel · 2019-01-14T09:45:55Z

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

zetlen · 2019-01-14T15:45:36Z

Regarding CircleCI config: https://circleci.com/docs/2.0/yarn/ seems pretty straightforward. I know that the cache parts are pretty weird, but fortunately this Yarn doc has some stuff you can cut and paste right over what we have today.

zetlen

I am so impressed at how small this changeset is, given the nature of the change. Could you please post some stats on the difference in final bundle sizes, build times, etc?

packages/peregrine/package.json

packages/venia-concept/webpack.config.js

packages/pwa-buildpack/src/ExtensionComponentWrap/.babelrc.js

sharkySharks

Initial once-over review. Have a few questions about all the dependencies being removed. Happy to see them go if they are not needed but wanted to get some more context on the changes.

Also can we add a .yarnrc file? I think the yarn-path would be good to set

but that ^ brings up a question, what is the preferred way of installing yarn? will we run into yarn dependency differences if we all have different local versions?

i'd recommend the yarn version be set in the engine field so we at least have a place to start from when debugging.

package.json

packages/venia-concept/yarn-error.log

sharkySharks · 2019-01-14T16:13:08Z

package.json

-    "clean:dist": "npx lerna run clean",
+    "build": "yarn workspaces run build",
+    "build:dev": "yarn workspaces run build:dev",
+    "clean:all": "yarn workspaces run clean && rimraf ./node_modules",


would we also want to clear the yarn cache?

I think I have more questions around when to use yarn workspaces and when to leverage our use of lerna bootstrap to install all of our package dependencies, and running commands like lerna run clean to remove all the package node_modules folders for us. Isn't lerna our monorepo package manager? 🤔

A main objective of this change is to stop using lerna for most of our monorepo tasks—especially for install, build, and clean operations. The goal is to have each package describe its own dependencies accurately.

Run yarn from the root directory and it will install all dependencies for the monorepo. Similarly, yarn build:dev and yarn:clean from the root will use workspaces to run that command in each workspace. It should be faster.

sounds good, i think from our discussion I'd like to remove lerna as a tool then if we are going to switch to using these workspaces and clear it out of our dependencies.

sharkySharks · 2019-01-14T16:23:01Z

Getting a few warnings when running yarn install:

packages/peregrine/package.json

babel.config.js

package.json

sirugh · 2019-01-14T16:30:42Z

I left a few comments but I was also unable to run the watch:venia command I get this output after first running yarn install:

❯ yarn run watch:venia
yarn run v1.13.0
$ cd packages/venia-concept && yarn run watch; cd - >/dev/null
$ webpack-dev-server --progress --color --env.mode development
Using environment variables from .env
internal/modules/cjs/loader.js:605
    throw err;
    ^

Error: Cannot find module 'babel-helper-module-imports'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:603:15)
    at Function.Module._load (internal/modules/cjs/loader.js:529:25)
    at Module.require (internal/modules/cjs/loader.js:657:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/Users/rugh/code/pwa-studio/packages/pwa-buildpack/dist/magento-layout-loader/babel-plugin-magento-layout.js:4:24)
    at Module._compile (internal/modules/cjs/loader.js:721:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:732:10)
    at Module.load (internal/modules/cjs/loader.js:620:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:560:12)
    at Function.Module._load (internal/modules/cjs/loader.js:552:3)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
✨  Done in 1.26s.

Do I need to do anything other than yarn install in root?

sirugh · 2019-01-14T16:43:25Z

There are still quite a few references to npm in docs and scripts. Can you update these accordingly?

jimbo · 2019-01-14T17:42:40Z

Do I need to do anything other than yarn install in root?

Yes, you need to build peregrine and pwa-buildpack (same as you do already). I've added steps to the PR description.

sharkySharks · 2019-01-17T17:02:45Z

lerna.json

-    ],
-    "npmClient": "npm"
+    "npmClient": "yarn",
+    "useWorkspaces": true


can we just remove this file now that we are going to use yarn workspaces?

looks like lerna.json is obsolete now, same question as above, can we remove this file now? It looks like you removed lerna from everywhere so far but this file. We probably want to make that change as well.

sharkySharks

I left a couple more comments. I am ready to sign off on this when the comments I left are addressed and the now.sh is fixed. For the now.sh deploy error, a nuclear fix may be to add yarn --ignore-engines but I think that's not ideal. I didn't see a good fix for this online as of yet.

jimbo · 2019-01-23T21:32:53Z

@sharkySharks Thanks for taking another look. Here are remaining tasks:

fix now.sh (blocked)
remove lerna.json
restore the hook
update docs

According to this issue, now.sh supports Yarn Workspaces as of version 2.0. Once @zetlen upgrades us, it should work, but it may also require some additional configuration in now.json.

I don't want to block the review process. My comments have been addressed, now.sh just needs to be debugged.

supernova-at

This works great for me and +15,301 −39,645 💪

supernova-at · 2019-01-25T22:51:51Z

README.md

-* Don't check in a big change to `package-lock.json`, and don't check in any `package-lock.json` files but the root one.
-* Make sure to run `npm run prettier` and `npm run lint` before any commit you intend to push. You may want to set up a [Git hook] for this.
+* Don't run `npm install`! Since this project has been configured with [Yarn Workspaces][], run `yarn install` to properly install, hoist, and cross-link dependencies.
+* Don't check in `package-lock.json`. There is only one lockfile, `yarn.lock`, and it's in the root directory.


Description =========== When migrating from npm to yarn, we tried to spread devDependencies across the packages instead of centralizing them at root. Unfortunately, we omitted a dependency that _needs_ to be at root so its CLI is available in scripts, [coveralls](https://github.com/nickmerwin/node-coveralls#readme). Missing this dependency caused every build after the merge of #753 and all we need to do is add it back: `yarn add -W --dev coveralls`. DangerCI has a transitive dependency on an outdated version of its GitHub client library, but the semver dependency in DangerCI allows updates, so we ran `yarn upgrade danger` and saw an Octokit update appear in yarn.lock.

zetlen reviewed Jan 14, 2019

View reviewed changes

sharkySharks previously requested changes Jan 14, 2019

View reviewed changes

package.json Show resolved Hide resolved

package.json Show resolved Hide resolved

packages/venia-concept/yarn-error.log Outdated Show resolved Hide resolved

sharkySharks reviewed Jan 14, 2019

View reviewed changes

sirugh reviewed Jan 14, 2019

View reviewed changes

packages/peregrine/package.json Outdated Show resolved Hide resolved

babel.config.js Outdated Show resolved Hide resolved

package.json Show resolved Hide resolved

sharkySharks reviewed Jan 17, 2019

View reviewed changes

sharkySharks reviewed Jan 23, 2019

View reviewed changes

cherdman added the 2.0 release label Jan 24, 2019

jimbo force-pushed the jimbo/yarn branch from 32341c1 to 940fd7f Compare January 24, 2019 23:59

Switch to Yarn and Babel 7

ba50e75

jimbo force-pushed the jimbo/yarn branch from 940fd7f to ba50e75 Compare January 25, 2019 00:27

jimbo added 12 commits January 24, 2019 17:35

Add upward-spec as a workspace

606a2d1

Remove package-lock.json

6c4bf14

Remove webpack-babel-env-deps

923e0b0

Remove references to package-lock

7748f44

Fix unmerged Circle config

c4155b3

Update Circle config

d3915a3

Update root readme

ee8356b

Update root readme

fcb1d7b

Update peregrine readme

6e01122

Update setup docs

43f39bf

Replace npm with yarn across docs

b39a89a

Restore lerna and re-enable watch:all

dbdd579

Merge branch 'release/2.0' into jimbo/yarn

262ec4d

supernova-at approved these changes Jan 25, 2019

View reviewed changes

jimbo merged commit 3f7bae7 into release/2.0 Jan 25, 2019

zetlen mentioned this pull request Feb 11, 2019

[BUG] (dev) Coveralls and Danger no longer work in CI #883

Closed

zetlen mentioned this pull request Feb 11, 2019

fix(ci): Fix CI deps at root. Fixes #883. #884

Merged

8 tasks

jimbo mentioned this pull request Feb 20, 2019

Add changelog for release 2.0 #927

Merged

supernova-at deleted the jimbo/yarn branch July 25, 2019 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Yarn & Babel 7 #753

Yarn & Babel 7 #753

jimbo commented Jan 14, 2019 •

edited

Loading

vercel bot commented Jan 14, 2019

zetlen commented Jan 14, 2019

zetlen left a comment

sharkySharks left a comment •

edited

Loading

sharkySharks Jan 14, 2019

sharkySharks Jan 14, 2019

jimbo Jan 14, 2019

sharkySharks Jan 17, 2019

sharkySharks commented Jan 14, 2019 •

edited

Loading

sirugh commented Jan 14, 2019 •

edited

Loading

sirugh commented Jan 14, 2019

jimbo commented Jan 14, 2019

sharkySharks Jan 17, 2019

sharkySharks Jan 23, 2019

sharkySharks left a comment

jimbo commented Jan 23, 2019 •

edited

Loading

supernova-at left a comment

supernova-at Jan 25, 2019

Yarn & Babel 7 #753

Yarn & Babel 7 #753

Conversation

jimbo commented Jan 14, 2019 • edited Loading

This PR is a:

Summary

Additional Information

Getting Started

Motivation

Yarn

Hoisting

Speed

vercel bot commented Jan 14, 2019

zetlen commented Jan 14, 2019

zetlen left a comment

Choose a reason for hiding this comment

sharkySharks left a comment • edited Loading

Choose a reason for hiding this comment

sharkySharks Jan 14, 2019

Choose a reason for hiding this comment

sharkySharks Jan 14, 2019

Choose a reason for hiding this comment

jimbo Jan 14, 2019

Choose a reason for hiding this comment

sharkySharks Jan 17, 2019

Choose a reason for hiding this comment

sharkySharks commented Jan 14, 2019 • edited Loading

sirugh commented Jan 14, 2019 • edited Loading

sirugh commented Jan 14, 2019

jimbo commented Jan 14, 2019

sharkySharks Jan 17, 2019

Choose a reason for hiding this comment

sharkySharks Jan 23, 2019

Choose a reason for hiding this comment

sharkySharks left a comment

Choose a reason for hiding this comment

jimbo commented Jan 23, 2019 • edited Loading

supernova-at left a comment

Choose a reason for hiding this comment

supernova-at Jan 25, 2019

Choose a reason for hiding this comment

jimbo commented Jan 14, 2019 •

edited

Loading

sharkySharks left a comment •

edited

Loading

sharkySharks commented Jan 14, 2019 •

edited

Loading

sirugh commented Jan 14, 2019 •

edited

Loading

jimbo commented Jan 23, 2019 •

edited

Loading