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.

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

chore: migrate to Yarn 2 #2316

Merged
merged 3 commits into from
Aug 3, 2020
Merged

chore: migrate to Yarn 2 #2316

merged 3 commits into from
Aug 3, 2020

Conversation

ylemkimon
Copy link
Member

@ylemkimon ylemkimon commented Jul 16, 2020

Yarn 2 supports PnP (Plug'n'Play). (https://yarnpkg.com/features/pnp)

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2020

Codecov Report

Merging #2316 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2316   +/-   ##
=======================================
  Coverage   93.96%   93.96%           
=======================================
  Files          84       84           
  Lines        5434     5434           
  Branches      959      959           
=======================================
  Hits         5106     5106           
  Misses        297      297           
  Partials       31       31           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 591bc7b...9ced0e6. Read the comment docs.

@ylemkimon ylemkimon changed the base branch from master to bump-dep July 16, 2020 23:19
],
"rules": {
"react/prop-types": 0
}
Copy link
Member Author

Choose a reason for hiding this comment

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

ESLint has issues with shared configs and PnP (yarnpkg/berry#8), so I've moved website/.eslintrc to here.

@@ -0,0 +1 @@
yarnPath: ".yarn/releases/yarn-berry.js"
Copy link
Member Author

Choose a reason for hiding this comment

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

https://yarnpkg.com/getting-started/install:

Using a single package manager across your system has always been a problem. To be stable, installs need to be run with the same package manager version across environments, otherwise there's a risk we introduce accidental breaking changes between versions - after all, that's why the concept of lockfile was introduced in the first place! And with Yarn being in a sense your very first project dependency, it should make sense to "lock it" as well.

For this reason, Yarn 2 and later are meant to be managed on a by-project basis. Don't worry, little will change!

@@ -112,7 +113,7 @@ jobs:

- run:
name: Verify screenshots and generate diffs and new screenshots
command: node dockers/screenshotter/screenshotter.js --selenium-ip localhost -b $CIRCLE_JOB --verify --diff --new --coverage
command: yarn node dockers/screenshotter/screenshotter.js --selenium-ip localhost -b $CIRCLE_JOB --verify --diff --new --coverage

This comment was marked as duplicate.

Copy link
Member

Choose a reason for hiding this comment

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

Is yarn monkey patching node?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinbarabash Sorry, I've quoted the wrong documentation. https://yarnpkg.com/advanced/migration#call-your-scripts-through-yarn-node-rather-than-node:

We now need to inject some variables into the environment for Node to be able to locate your dependencies. In order to make this possible, we ask you to use yarn node which transparently does the heavy lifting.

Note: this section only applies to the shell CLI. The commands defined in your scripts are unaffected, as we make sure that node always points to the right location, with the right variables already set.

Copy link
Member Author

@ylemkimon ylemkimon Aug 2, 2020

Choose a reason for hiding this comment

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

Looking at the source, it seems it just adds --require .pnp.js to NODE_OPTIONS variable. Preloaded .pnp.js then registers a custom module resolver.

@@ -1,5 +1,6 @@
// @flow
const {targets, createConfig} = require('./webpack.common');
// $FlowIgnore
Copy link
Member Author

@ylemkimon ylemkimon Jul 16, 2020

Choose a reason for hiding this comment

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

Flow has issues resolving dependencies with PnP (yarnpkg/berry#634), but as we don't have any dependencies in main codebase, I think it's an acceptable loss. Furthermore, Typescript doesn't have this issue (#2326).

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we don't have flow-typed for them and they don't have type information, so nothing is changed.

"dependencies": {}
"dependenciesMeta": {
"docusaurus": {
"unplugged": true
Copy link
Member Author

Choose a reason for hiding this comment

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

Docusaurus requires write access to node_modules, so we cannot use PnP and should unplug it. (yarnpkg/berry#818 (comment))

Copy link
Member

Choose a reason for hiding this comment

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

If docusaurus is "unplugged" does this mean that it and all of its dependencies are installed into node_modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinbarabash AFAIK, only the dependency itself is unplugged. Its dependencies remain PnP-ed.

Copy link
Member

Choose a reason for hiding this comment

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

That's pretty cool.

@ylemkimon ylemkimon changed the title Migrate to Yarn 2 feat: migrate to Yarn 2 Jul 17, 2020
@ylemkimon ylemkimon changed the title feat: migrate to Yarn 2 chore: migrate to Yarn 2 Jul 17, 2020
Base automatically changed from bump-dep to master July 25, 2020 18:38
Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

I knew about the big changes in yarn 2, but haven't had the chance to experiment with it in any projects yet. I'm interested to see how this turns out.

bower.json Outdated
@@ -1,36 +0,0 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Is removing this file necessary to migrate to yarn 2?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinbarabash No, I had to update the Yarn version in Netlify, where I realized it installs Bower and runs bower install. I've separated changes to #2372 and it's merged.

@@ -112,7 +113,7 @@ jobs:

- run:
name: Verify screenshots and generate diffs and new screenshots
command: node dockers/screenshotter/screenshotter.js --selenium-ip localhost -b $CIRCLE_JOB --verify --diff --new --coverage
command: yarn node dockers/screenshotter/screenshotter.js --selenium-ip localhost -b $CIRCLE_JOB --verify --diff --new --coverage
Copy link
Member

Choose a reason for hiding this comment

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

Is yarn monkey patching node?

"dependencies": {}
"dependenciesMeta": {
"docusaurus": {
"unplugged": true
Copy link
Member

Choose a reason for hiding this comment

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

If docusaurus is "unplugged" does this mean that it and all of its dependencies are installed into node_modules?

Comment on lines +3 to 15
// $FlowIgnore
const TerserPlugin = require('terser-webpack-plugin');
// $FlowIgnore
const MiniCssExtractPlugin = require('mini-css-extract-plugin');
// $FlowIgnore
const PnpWebpackPlugin = require('pnp-webpack-plugin');

const {version} = require("./package.json");

// $FlowIgnore
const browserslist = require('browserslist')();
// $FlowIgnore
const caniuse = require('caniuse-lite');
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need $FlowIngores for these imports now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

Comment on lines +33 to +36
!.yarn/releases
!.yarn/plugins
!.yarn/sdks
!.yarn/versions
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any files checked into for this director or .website, did you forget to add some files to this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kevinbarabash They are files recommended by the doc. Yarn automatically uses Yarn in parent directories, so there are currently no files for website.

@edemaine
Copy link
Member

edemaine commented Aug 2, 2020

I was looking at Yarn 2 the other day. It's annoying that it requires checking in the entire yarn binary (2MB). PnP looks pretty cool though!

@ylemkimon
Copy link
Member Author

ylemkimon commented Aug 2, 2020

@edemaine But I think it's in the right direction. It allows to use a single, even outdated, Yarn across projects. Git nowadays handles large files really smoothly.

Copy link
Member

@kevinbarabash kevinbarabash left a comment

Choose a reason for hiding this comment

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

Let's give it a try.🤞

@ylemkimon ylemkimon merged commit f97c545 into master Aug 3, 2020
@ylemkimon
Copy link
Member Author

ylemkimon commented Aug 3, 2020

I wonder Dependabot will work with Yarn 2 (dependabot/dependabot-core#1297). I should've searched more. There was another issue in Dependabot, dependabot/dependabot-core#2030, so it breaks lockfile.

@ylemkimon ylemkimon deleted the yarn-v2 branch August 3, 2020 16:45
@kevinbarabash
Copy link
Member

🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants