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

Upgrade to Yarn v4 (Berry) #3095

Open
wants to merge 6 commits into
base: livekit
Choose a base branch
from
Open

Upgrade to Yarn v4 (Berry) #3095

wants to merge 6 commits into from

Conversation

robintown
Copy link
Member

@robintown robintown commented Mar 14, 2025

#3093 presents a dilemma: We want to consume matrix-js-sdk via its build artifacts, but we also want to continue using development versions of matrix-js-sdk (pinned to Git commits) rather than releases. Development versions, of course, don't come with build artifacts. This threatens to complicate the build process for developers, packagers, CI, everyone. (Now you would always need to clone & link matrix-js-sdk, cross-reference our package.json, check out the right commit, and build it separately.)

The good thing is, modern package managers have figured out a far more elegant way to support this use case: if a Git dependency contains a prepack script, they will run that script before installing the package into node_modules, making this the perfect way to automatically perform a build step. In fact, matrix-js-sdk already has this exact script! We just can't benefit from it as long as we're stuck using Yarn Classic, which hasn't been actively developed for years.

I suggest that now would be as good a time as any to upgrade from Yarn Classic to Yarn v4 (Berry). The Yarn developers put a lot of thought into making the yarn command automatically invoke the right version of the tool depending on the repository you're in, so switching between Yarn Classic and Yarn Berry repositories is a pretty seamless experience. The only obstacle to upgrading that I encountered is that Yarn Berry regresses the developer experience of working with linked packages. However, I believe I've remedied this with a plugin I threw together.

Any reservations on making such an upgrade?

For #3081
Depends on https://github.com/element-hq/element-call-webapp-deployments/pull/3

It's not generally available in browser environments / certain Yarn modes and can easily be replaced by TextEncoder.
@robintown robintown requested a review from a team as a code owner March 14, 2025 07:17
@robintown robintown requested a review from toger5 March 14, 2025 07:17
@toger5
Copy link
Contributor

toger5 commented Mar 14, 2025

Any reservations on making such an upgrade?

This sounds really nice. Since this is basically a full package manager change I feel like it would be appropriate to do a bit of research and collect pro/cons to other alternatives and not make the decision an artifact of us not being able to compile with vite dev.

Comment on lines +3 to +9
If you want to make changes to a package that Element Call depends on and see those changes applied in real time, you can create a link to a local copy of the package. Yarn has a command for this (`yarn link`), but it's not recommended to use it as it ends up modifying package.json with details specific to your development environment.

Instead, you can use our little 'linker' plugin. Create a file named `.links.yaml` in the Element Call project directory, listing the names and paths of any dependencies you want to link. For example:

```yaml
matrix-js-sdk: ../path/to/matrix-js-sdk
"@vector-im/compound-web": /home/alice/path/to/compound-web
Copy link
Contributor

Choose a reason for hiding this comment

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

I am loving this.

Do I understand it correctly, that it just allows us to put this part of the package.json into its own file, so that we can git-ignore it?
pnpm has the same behaviour and I was wondering how this could be elegantly solved.
This is great!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, though I just realized that this method will still modify yarn.lock, sigh. I don't know of a better solution right now.

@@ -297,7 +296,7 @@ export class PosthogAnalytics {
const posthogIdMaterial = "ec" + accountAnalyticsId + client.getUserId();
const bufferForPosthogId = await crypto.subtle.digest(
"sha-256",
Buffer.from(posthogIdMaterial, "utf-8"),
new TextEncoder().encode(posthogIdMaterial),
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed for the yarn berry switch or just randomly here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was trying out Yarn's plug-n-play mode, and it needed this change. I reverted to the normal node_modules linker so I don't think it's strictly necessary. Would you prefer I exclude it?

Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

@robintown robintown added the X-Blocked Cannot be merged due to external dependencies label Mar 14, 2025
@robintown
Copy link
Member Author

This depends on https://github.com/element-hq/element-call-webapp-deployments/pull/3 then, so marking as blocked.

@robintown robintown added X-Blocked Cannot be merged due to external dependencies and removed X-Blocked Cannot be merged due to external dependencies labels Mar 14, 2025
@hughns hughns changed the title Upgrade to Yarn Berry Upgrade to Yarn v4 (Berry) Mar 14, 2025
@robintown
Copy link
Member Author

@toger5

This sounds really nice. Since this is basically a full package manager change I feel like it would be appropriate to do a bit of research and collect pro/cons to other alternatives and not make the decision an artifact of us not being able to compile with vite dev.

I had a look at whether switching to npm or pnpm would be possible. At first, I was under the impression that either would work for us, but it turns out that their handling of Git dependencies is subtly broken.

  • npm just does not execute prepack scripts for Git dependencies, even though its docs claim that it does. It will not build an unlinked matrix-js-sdk from source.
  • pnpm circumvents Git and downloads dependencies directly from GitHub's CDN, meaning that the cache entry for the package lacks a .git directory. matrix-js-sdk assumes .git will exist in order to include a version identifier in the build, which given that we're requesting a Git dependency, is a very reasonable assumption to make. The matrix-js-sdk build fails.

So Yarn appears to be the only package manager out there that handles Git dependencies in a reasonable way, as of now. I think this is a good example of the Yarn project's commitment to stability & correctness around edge cases, which is a reason I would've advocated for it even if npm/pnpm were equally viable. It's a package manager's best feature :)

Bun is another alternative, but I haven't tried it out because switching to an entirely different JS runtime seems like a far deeper rabbit hole.

@toger5
Copy link
Contributor

toger5 commented Mar 14, 2025

@robintown Thank you for going the extra mile of looking into this and providing us with a specific reason/reasons why we want to go that path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
X-Blocked Cannot be merged due to external dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants