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

[core] Change package manager to pnpm #36287

Merged
merged 117 commits into from
Jan 3, 2024
Merged

[core] Change package manager to pnpm #36287

merged 117 commits into from
Jan 3, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Feb 21, 2023

Migrated the Core repo from Yarn Classic to pnpm.

Motivation and getting started with pnpm: https://mui-org.notion.site/pnpm-32e8449c77ce403a9f4582a86f1a6c79

Summary of the changes:

  • set up the packageManager field in the root package.json
  • created the pnpm-workspace.yaml file with all projects in the repository
  • changes workspace project inter-dependencies to use the workspace: protocol
  • replaced all occurrences of "yarn" to "pnpm" across scripts, comments, and docs (adjusting the commands where necessary)
  • updated Lerna, Renovate, and CI configs to be aware of pnpm
  • set up pnpm to link workspace project using build outputs (usually the "build" directory under project root). Bundlers and other tools use aliases to point to the untranspiled source code.

Since the question came up in Slack. Just to see what a migration looks like. This PR mostly to see how the @mui/monorepo dependency will behave in X/Toolpad

Migrated the repo up until pnpm docs:dev works. None of the other commands have been tested or migrated. Nothing in the CI pipeline has been migrated.

Some quick notes

  • https://dev.to/andreychernykh/yarn-npm-to-pnpm-migration-guide-2n04

  • pnpm import
     ERR_PNPM_INVALID_OVERRIDE_SELECTOR  Cannot parse the "**/@babel/core" selector in the overrides
    

    => remove **/ everywhere?

  • added .npmrc with

    auto-install-peers = true
    
  • replace workspace dependencies versions in package.json files with workspace:*

  • scripts:

    • yarn workspace docs dev => pnpm --dir ./docs dev

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 21, 2023

I would be curious to see if it speed-up the CI, and by how much. It might be the biggest selling point. https://mui-org.notion.site/Migrate-to-pnpm-2a4f2e9869cf496eae183eff2aeef9a9

@oliviertassinari oliviertassinari added the scope: code-infra Specific to the core-infra product label Feb 21, 2023
@Janpot
Copy link
Member Author

Janpot commented Feb 21, 2023

Yep, it already feels quite fast locally. I think there's two roads to continue with this

  • first try to migrate CI in this repo, to see the impact on build time to see if we should even bother porting to the other repos
  • first try to make the docs work on Toolpad or X to see if we should even bother spending time migrating CI

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 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 9, 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 10, 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 23, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 2, 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 2, 2024
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I left few clarifying questions/comments. Would be great to validate them before merging.

docs/README.md Show resolved Hide resolved
@@ -14,7 +14,7 @@
"strict": true,
"baseUrl": "./",
"paths": {
"react-docgen": ["../api-docs-builder/react-docgen.d.ts"]
Copy link
Member

Choose a reason for hiding this comment

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

Why is this no longe required?

Copy link
Member

Choose a reason for hiding this comment

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

React-docgen types live in a separate package and are simply added as a dev dependency.

"@mui-internal/test-utils": "^1.0.0",
"@mui-internal/babel-macros": "workspace:^",
"@mui-internal/test-utils": "workspace:^",
"@mui/base": "workspace:*",
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, do we need to define the same package as its own dependency?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, otherwise, we won't be able to access the @mui/base contents in tests.

Copy link
Member

Choose a reason for hiding this comment

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

Got it!

@@ -94,10 +94,10 @@ const Badge = React.forwardRef(function Badge<RootComponentType extends React.El
}) as PolymorphicComponent<BadgeTypeMap>;

Badge.propTypes /* remove-proptypes */ = {
// ----------------------------- Warning --------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

These changes look unrelated, should we revert in order to have a smaller pull request?

Copy link
Member

Choose a reason for hiding this comment

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

I had to change the yarn to pnpm in the message, so I figured I might as well fix the grammar and formatting bit. Revering those additional changes won't make the number of changed files smaller.

Copy link
Member

Choose a reason for hiding this comment

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

Aaah, I missed that, I thought it is just the formatting. Got ya, let's keep them.

// | These PropTypes are generated from the TypeScript type definitions |
// | To update them edit TypeScript types and run "yarn proptypes" |
// ----------------------------------------------------------------------
// ┌────────────────────────────── Warning ──────────────────────────────┐
Copy link
Member

Choose a reason for hiding this comment

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

I just left a comment above too. The pull request is anyways heavy, it would be great if we can revert this particular change.

@@ -1,6 +1,7 @@
{
"extends": "../../tsconfig.json",
"compilerOptions": {
"skipLibCheck": true,
Copy link
Member

Choose a reason for hiding this comment

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

What was failing here?

Copy link
Member

Choose a reason for hiding this comment

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

Mismatched NextJS version. I just pinned the nextjs version and removed this compiler option.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

We are ready to go! 🎉 🚀

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 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 3, 2024
@michaldudak michaldudak merged commit a1263e3 into mui:master Jan 3, 2024
22 checks passed
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
@oliviertassinari
Copy link
Member

"preinstall": "npx only-allow pnpm",

Awesome, love to see this.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 15, 2024

I think I found a regression with pnpm:

"dependencies": {
"@babel/runtime": "^7.23.7",
"@floating-ui/react-dom": "^2.0.5",
"@mui/types": "workspace:^",
"@mui/utils": "workspace:^",

As far as I can read this: we can no longer control the minimum version dependency, we are forced to force every developer to update all their dependencies even if there are no breaking changes or reliance on new features, leading to a higher chance of package duplication in the bundle (good for our npm downloads stats, not good for end-users)

@michaldudak
Copy link
Member

How is this different from the workflow we had with Yarn? We used to update the versions of internal dependencies each release to the most current one. Now it's done automatically by pnpm.

Anyway, if you want to allow earlier versions, pnpm does allow to do it (for example workspace:^5.0.0)

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 16, 2024

We used to update the versions of internal dependencies each release to the most current one.

@michaldudak It's simpler during the release process, and a lot less likely to do it wrong. But in theory, if we aren't dependent on the new feature, it's not necessary to increase the range.

Anyway, if you want to allow earlier versions, pnpm does allow to do it (for example workspace:^5.0.0)

Great to know 👍. So if we ever want to change the tradeoff, it looks like we are good.

@Janpot
Copy link
Member Author

Janpot commented Jan 16, 2024

it's not necessary to increase the range.

I thought we kept versioning in lerna, if lerna wasn't increasing the range before pnpm migration, it won't increase it after. Or am I missing something?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 16, 2024

I thought we kept versioning in lerna, if lerna wasn't increasing the range before pnpm migration, it won't increase it after. Or am I missing something?

@Janpot This indeed didn't change. The conclusion for me is:

  • [core] Use Lerna to publish #23793 came with the cons of making it hard to have optimal scope in the dependencies. It's not great, but the alternative is worse. For external dependencies, we still manage this by hand. We only bump the scope when we know we need it.
  • pnpm makes it harder for developers who check the source to know what's the actual version dependency (there is an indirection) but make it clear that it's from the same workspace.

@Janpot
Copy link
Member Author

Janpot commented Jan 17, 2024

The benefit of the workspace:* version range is that it 100% guarantees that they are locally linked. pnpm will never download them from the registry. This is something that could happen in the previous setup if the versions don't match up for some reason. During publishing pnpm does all the work filling in the correct versions.

  • pnpm makes it harder for developers who check the source to know what's the actual version dependency (there is an indirection) but make it clear that it's from the same workspace.

Those people should instead run

npm info @mui/material
# or
pnpm info @mui/material
# or
yarn info @mui/material

It's a much more reliable source for this information as it shows the actually published package. It's also much easier to find the dependencies of a specific version of a package by adding a version range pnpm info @mui/[email protected].

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 2, 2024

I was curious about the CI metric impact of this PR:

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 scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants