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 Modern #6028

Closed
wants to merge 37 commits into from
Closed

chore: migrate to Yarn Modern #6028

wants to merge 37 commits into from

Conversation

RDIL
Copy link
Contributor

@RDIL RDIL commented Nov 29, 2021

Motivation

Switch to yarn v3

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

i'm not done yet

Related PRs

slorber/responsive-loader#1

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 29, 2021
@RDIL RDIL marked this pull request as draft November 29, 2021 15:36
@Josh-Cena Josh-Cena changed the title Yarn v3 chore: migrate to Yarn v3 Nov 29, 2021
@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Nov 29, 2021
@Josh-Cena
Copy link
Collaborator

Context: RDIL and I had been talking about this on Discord. We both investigated in Yarn berry in the past week, but ran into some problems due to our monorepo setup. Seems he's more experienced in this, so I let him champion this migration.

"@docusaurus/utils": "2.0.0-beta.9",
"@docusaurus/utils-common": "2.0.0-beta.9",
"@docusaurus/utils-validation": "2.0.0-beta.9",
"@docusaurus/core": "workspace:*",
Copy link
Collaborator

@Josh-Cena Josh-Cena Nov 30, 2021

Choose a reason for hiding this comment

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

I guess this is only for testing purposes? I have no idea if it works well with NPM or Lerna.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is how things are done in Yarn 3. Putting an actual version would make it fetch from the npm registry. Plus, this way, the release diff is smaller.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We use lerna to automate the release (yes, a weird mix of yarn workspaces and lerna). In the perfect world we ditch lerna altogether in this PR, but otherwise we have to make sure they interop

Copy link
Contributor

Choose a reason for hiding this comment

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

Putting an actual version would make it fetch from the npm registry

NIT: Only if the range doesn't match the version of the workspace

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, @merceyz Does that mean we can keep using the actual version (2.0.0-beta.9) in the workspace and publish with lerna?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually—no, let's not get stuck on the good old lerna forever. @slorber What's your call? Should we migrate to using Yarn workspaces throughout or should we continue using lerna, provided they would have similar DX and no user-side differences?

packages/docusaurus/package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -38,8 +39,7 @@
"canary:bumpVersion": "yarn lerna version `yarn --silent canary:version` --exact --no-push --yes",
"canary:publish": "yarn lerna publish from-package --dist-tag canary --yes --no-verify-access",
"changelog": "lerna-changelog",
"postinstall": "run-p postinstall:**",
"postinstall:main": "yarn lock:update && yarn build:packages",
"postinstall": "run-p postinstall:\"**\"",
Copy link
Contributor

@armano2 armano2 Dec 1, 2021

Choose a reason for hiding this comment

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

are you sure about this?

i think you are missing

"postinstall:main": "yarn build:packages",

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm still investigating this. It seems the postinstall hooks aren't even executed

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the postinstall hooks aren't even executed

They are, but only when the dependency tree changes.

postinstall is called after a package's dependency tree changes are written to the disk -- e.g. after a dependency or transitive dependency is added, removed, or changed
https://yarnpkg.com/advanced/lifecycle-scripts

package.json Outdated
Comment on lines 19 to 21
"build": "yarn build:packages && yarn build:website",
"build:packages": "lerna run build --no-private",
"build:packages": "yarn workspaces foreach --no-private run build",
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure about this change?

Copy link
Contributor

@armano2 armano2 Dec 1, 2021

Choose a reason for hiding this comment

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

i think you are missing now

"build": "yarn workspaces foreach run build",

Copy link
Collaborator

@Josh-Cena Josh-Cena Dec 1, 2021

Choose a reason for hiding this comment

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

This is implied. yarn build seems to work even without a custom script

Edit. NVM, yarn build is from the yarn.build plugin

.yarnrc.yml Outdated
Comment on lines 6 to 7
- path: .yarn/plugins/@ojkelly/plugin-build.cjs
spec: "https://yarn.build/latest"
Copy link
Contributor

@armano2 armano2 Dec 1, 2021

Choose a reason for hiding this comment

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

can you explain why do we need this plugin?

i don't see any use case for yarn.build -> https://github.com/ojkelly/yarn.build

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, RDIL and I had been talking with Owen on Discord and he recommended tophat/monodeploy instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@armano2 right now I'm using it to actually get the packages to build with an output, plus it has a nice UI.

Copy link

Choose a reason for hiding this comment

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

👋 yeah for sending the packages to npm, tophat/monodeploy is probably a good choice.

yarn.build is more like a bazel/buck, so can build local workspaces in the correct topological order with as much concurrency as possible, and it keeps a record of what's been built so it only rebuild what's changed.

It's also great for doing the same with tests yarn test and you'll find it most powerful when each workspace is treated as a self contained unit.

If you pair it with yarn's vendoring, then you get the speed and soundness from the big tools like bazel/buck.

@@ -559,14 +559,14 @@ export function getMinimizer(
// Using the array syntax to add 2 minimizers
// see https://github.com/webpack-contrib/css-minimizer-webpack-plugin#array
new CssMinimizerPlugin({
// @ts-expect-error: the lib def looks terrible...
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason for code change here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This I have little idea. The typechecking is now reporting errors in different places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it has to do with typescript getting the type definitions from a different place. Not 100% sure what's going on here either.

@RDIL
Copy link
Contributor Author

RDIL commented Dec 8, 2021

To quote the Yarn website:

In a few words, upgrading to the latest versions is critical to a fast and stable Yarn experience. Numerous bugs were fixed since the first major version, and we no longer expect to build new features on the old trunk. Even if you don't plan to use the new default installation strategy called Plug'n'Play your projects will still get benefits from the upgrade:

  • The good old node_modules installer improved as well as various edge cases got fixed
  • A renewed focus on performances and good practices (we now formally track perfs via a dashboard)
  • Improved user experience for various CLI commands and settings (yarn add -i, yarn up, logFilters, ...)
  • New commands and capabilities (such as the TypeScript plugin, or the release workflow)

And of course a very active development cycle.

LONG VERSION While the Yarn Classic line (1.x) remains a pillar of the JavaScript ecosystem, we recommend upgrading if possible. Why's that?
New features: On top of the classic features you're already used to, on top of the new ones you'll discover (yarn dlx, builtin patch: protocol, ...), Modern offers plugins extending Yarn's featureset with changesets, constraints, workspaces, ...

Efficiency: Modern features new install strategies, leading projects to only be a fraction of their past self; as an example, under the default configuration the stock CRA artifacts now only take 45MB instead of 237MB. Performances were improved as well, with most installs now only taking a few seconds even on extremely large projects. We even made it possible to reach zero seconds!

Extensibility: Modern's architecture allows you to build your own features as you need it. No more of you being blocked waiting for us to implement this feature you dream of - you can now do it yourself, according to your own specs! Focused workspaces, custom installs, project validation, ...

Stability: Modern comes after years of experience with maintaining Classic; it allowed us to finally fix longstanding design issues with how some features were implemented. Workspaces are now core components, the resolution pipeline has been streamlined, data structures are more efficient... as a result, Modern is much less likely to suffer from incorrect assumptions and other design flaws.

Future proof: A big reason why we invested in Modern was that we noticed how building new features on Classic was becoming difficult - each change being too likely to have unforeseen consequences. The Modern architecture learned from our mistakes, and was designed to allow us to build features at a much higher pace - as evidenced by our new gained velocity.

@Josh-Cena
Copy link
Collaborator

maybe not now?

It's now or never. We aren't blocked by anything; Yarn v3 is fully stable. This is also internal so there's no release schedule / pressure from users, but it's better if we start doing something

@slorber
Copy link
Collaborator

slorber commented Dec 9, 2021

Thanks

I understand these advantages, but this looks more like an internal quality of life improvement than a critical thing we need asap.

It's now or never.

I don't really understand what you mean. What prevents us to do this upgrade after 2.0?

If we work on this now, this delays 2.0 due to reviewing this PR, ensuring the release process still work as before, the changelog (using lerna-changelog) can still be generated, the canary releases keep working, and various other things that I may not think of.

If it's clear that all the work is done up-frontand well-tested in this PR, including a full release workflow, it's more likely to be merged as it's less likely that this becomes a time sink for me due to unplanned issues

TL.DR: the most important for me is a good step by step test plan that I can simply unroll to be 100% sure that everything will work after merge

@Josh-Cena
Copy link
Collaborator

I understand these advantages, but this looks more like an internal quality of life improvement than a critical thing we need asap.

Indeed. It's just internal; there's no release schedule. It doesn't block the v2 official release and you shouldn't stress about reviewing it. We're just keeping it here so people know that there's a work in progress. Everything else can move on.

It's now or never.

I was saying that "maybe not now" is not adequate because if there's a motivation and nothing's preventing us from doing it, we should do it rightaway, otherwise it would stay in the backlog forever.

Anyways, @slorber don't worry too much about this, @RDIL and I will need to make it production ready first before marking it as ready for review.

@Josh-Cena
Copy link
Collaborator

@merceyz We are currently using Yarn canary, but the build size report action is still failing when running yarn --frozen-lockfile run build:website:en. Is preactjs/compressed-size-action#41 really not needed?

@Josh-Cena
Copy link
Collaborator

Summary of the workflows we need to check:

  • Canary release, currently done with yarn lerna version `yarn --silent canary:version` --exact --no-push --yes && yarn lerna publish from-package --dist-tag canary --yes --no-verify-access
  • E2E test with Verdaccio, currently done with npx --no-install lerna publish --exact --yes --no-verify-access --no-git-reset --no-git-tag-version --no-push --registry "$CUSTOM_REGISTRY_URL" "$NEW_VERSION"
  • yarn workspaces foreach doesn't seem to output progress? Lerna tells me that it's currently executing script in workspace X, but Yarn workspaces just goes silent for two minutes

@merceyz
Copy link
Contributor

merceyz commented Jan 26, 2022

@merceyz We are currently using Yarn canary, but the build size report action is still failing when running yarn --frozen-lockfile run build:website:en. Is preactjs/compressed-size-action#41 really not needed?

Sort of, its change from --frozen-lockfile to --immutable is unnecessary now but the part that fixes the npm variable is needed

- installScript = npm = `yarn --frozen-lockfile`;
+ npm = `yarn`;
+ installScript = `yarn --frozen-lockfile`;

yarn workspaces foreach doesn't seem to output progress? Lerna tells me that it's currently executing script in workspace X, but Yarn workspaces just goes silent for two minutes

Add the -i flag to see the output in real time and the -v flag to prefix each line with the workspace.

@Josh-Cena
Copy link
Collaborator

Add the -i flag to see the output in real time and the -v flag to prefix each line with the workspace.

Thanks! Just curious, is there a way to configure this in .yarnrc?

Sort of, its change from --frozen-lockfile to --immutable is unnecessary now but the part that fixes the npm variable is needed

That means a fix is still needed for this action. I see.

@Josh-Cena Josh-Cena mentioned this pull request Feb 5, 2022
7 tasks
@RDIL RDIL changed the title chore: migrate to Yarn v3 chore: migrate to Yarn Modern Jun 23, 2022
@crutchcorn
Copy link

Hey all, was wondering on the status of this PR and if it was possible for me to pick up the work on migrating the project to Yarn 4?

@RDIL
Copy link
Contributor Author

RDIL commented Mar 31, 2024

@crutchcorn go for it! I put a pause on working on this because @slorber wasn't sure if it was a good idea to make the switch, but I think a new PR with the conflicts fixed could re-start that discussion.

@RDIL RDIL closed this Mar 31, 2024
@slorber
Copy link
Collaborator

slorber commented Apr 4, 2024

I think we could switch to yarn 4 but I'd prefer to keep the node linker for now and turn it on later after testing it a bit more

@RDIL
Copy link
Contributor Author

RDIL commented Apr 4, 2024

@slorber that's totally reasonable. I use the node linker in most of my personal projects, and I think that it tends to generally work better imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants