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

[code infra] migrate to pnpm #2546

Merged
merged 143 commits into from
Jan 7, 2024
Merged

[code infra] migrate to pnpm #2546

merged 143 commits into from
Jan 7, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 25, 2023

Migrating yarn to pnpm

benchmark

I just compared 4 CI runs, not doing any serious statistics here

First number is cache restoration step, second number is the install step.

From cache yarn pnpm
Yes 0s + 3m55s = 3m55s 0s + 55s = 55s
No 50s + 35s = 1m25s 35s + 2s = 37s

deduplication takes more time in pnpm than in yarn. We can mitigate the effect by only running it when the lock file has changed.

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Aug 25, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 25, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 26, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 26, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 20, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 21, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 4, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 4, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 4, 2024
@Janpot Janpot marked this pull request as ready for review January 4, 2024 22:20
@Janpot Janpot mentioned this pull request Jan 4, 2024
@Janpot Janpot requested a review from a team January 5, 2024 11:03
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Awesome!

@Janpot Janpot merged commit 0140ebd into mui:master Jan 7, 2024
11 checks passed
@Janpot Janpot deleted the pnpm-try branch January 7, 2024 12:48
@Janpot Janpot changed the title [code infra] pnpm take 2 [code infra] migrate to pnpm Jan 7, 2024
@Janpot Janpot mentioned this pull request Jan 12, 2024
@@ -16,21 +16,22 @@
"toolpad": "./dist/index.js"
},
"scripts": {
"preinstall": "npx only-allow pnpm",
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: I'm not sure how necessary is this.
Trying to run yarn on a pnpm setup produces the following error:

error This project's package.json defines "packageManager": "yarn@[email protected]". However the current global version of Yarn is 1.22.21.

Presence of the "packageManager" field indicates that the project is meant to be used with Corepack, a tool included by default with all official Node.js distributions starting from 16.9 and 14.19.
Corepack must currently be enabled by running corepack enable in your terminal. For more information, check out https://yarnpkg.com/corepack.

Am I missing something or this script can actually be omitted? 🤔

Copy link
Member Author

@Janpot Janpot Apr 9, 2024

Choose a reason for hiding this comment

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

I believe that error only appears when corepack is enabled. There are older versions of yarn which would happily install even though the packageManager field exists. e.g. I was able to install with [email protected] (not through corepack) when I remove the preinstall script.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Then it makes sense to add it in. 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants