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

feat!: remove --all flag #351

Merged
merged 7 commits into from
Feb 17, 2024
Merged

feat!: remove --all flag #351

merged 7 commits into from
Feb 17, 2024

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 8, 2024

Force users to opt-in into the package managers they want to support. As more and more package managers are added to Corepack, it's more and more unlikely that our users will be interested in all the package managers Corepack supports, and it is unreasonable to expect Corepack maintainers would be able to perform security audits.

Force users to opt-in into the package managers they want to support.
As more and more package managers are added to Corepack, it's more and
more unlikely that our users will be interested in _all_ the package
managers Corepack supports, and it is unreasonable to expect Corepack
maintainers would be able to perform security audits.
Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

I find the increased friction in user experience a little problematic. Running corepack enable and things Just Work is easy. having to run the command multiple times isn't, especially if the only message people get is "command not found". Can we find alternatives?

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 8, 2024

Yes unfortunately it is likely some script out there relies on it. We could deprecate corepack enable, freeze it to always mean corepack enable pnpm@latest yarn@1.x (i.e don't add more package manager to it), and possibly make it emit a warning.

I expect the --all flag is good to go, no?

@arcanis
Copy link
Contributor

arcanis commented Jan 11, 2024

I expect the --all flag is good to go, no?

The --all flag was intended to help build base offline base images for cloud providers, which frequently want all tools to be included in their images. I don't have a strong opinion as to whether this is super useful or not, but I'd tend to refrain from removing it, to avoid unneeded friction.

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 11, 2024

Maybe @styfle would have an opinion there? I would expect cloud providers to be "picky" about which package managers they enable by default, as more and more package managers are added to Corepack, I would expect they want to activate only a subset.

@styfle
Copy link
Member

styfle commented Jan 11, 2024

I would expect they want to activate only a subset.

At Vercel, we only enable the package manager found in package.json.

I didn't know the --all flag existed until now.

I think we need to look at more long term and see how this would impact the scenario when corepack enable is no longer needed, presumably when it goes stable.

@aduh95 aduh95 requested a review from arcanis January 31, 2024 09:44
Copy link
Contributor

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Not 100% convinced, but I don't have a very strong opinion either way - it can be added back later on if really needed.

@aduh95 aduh95 merged commit d9c70b9 into main Feb 17, 2024
10 checks passed
@aduh95 aduh95 deleted the no-all-option branch February 17, 2024 00:35
@nickserv
Copy link

The old --all functionality that updates pnpm and Yarn can be replicated with:

corepack prepare --activate pnpm yarn

@zanminkian
Copy link

The old --all functionality that updates pnpm and Yarn can be replicated with:

corepack prepare --activate pnpm yarn

Why not using corepack install -g pnpm && corepack install -g yarn? prepare subcommand is confused and is not listed in corepack -h.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants