Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get we want to use https://pnpm.io/installation#using-corepack but do we need to have this here?
Maybe with pnpm v9 we don't need this anymore: https://github.com/pnpm/pnpm/releases/tag/v9.0.0-rc.2
we would then match the other repositories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We enforce a specific version with the engines field. In case someone tries to install who is not using corepack, or an older (pre-corepack) pnpm version.
So:
packageManager
field: makes sure the correct pnpm version is used when corepack is enabledpreinstall
script: makes sure when corepack is not enabled and an older version of e.g.yarn
is usedpnpm
is used in case corepack is not enabled, and an older version ofpnpm
is usedRecent versions of
yarn
andpnpm
should detect that corepack is intended and refuse to install. Older ones happily install and run scripts regardless. With v9, this field will still be necessary, for contributors that are not on v9 yet. We should port this to other reporitories (and the.npmrc
'sengines-strict
option as well)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this makes sense.
I would wait to see that the next upgrades of pnpm go smoothly, and once they do go all-in to propagate this in all repositories mui/mui-public#157.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks like it does: #3499