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

Adds a note clarifying how to add new package managers #408

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Feb 27, 2024

We didn't document the process to add new package managers to Corepack. I added a rough draft based on the discussions we had around the topic lately, feel free to suggest improvements in wording or content.

@arcanis arcanis requested a review from a team February 27, 2024 08:38
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Antoine du Hamel <[email protected]>
@aduh95 aduh95 merged commit 88d504c into main Feb 27, 2024
10 checks passed
@aduh95 aduh95 deleted the mael/adding-package-manager branch February 27, 2024 09:49
Once added, the shims pertaining to new package managers won't be automatically
enabled by `corepack enable` when called without arguments - it'll require users
to explicitly install the relevant shims (e.g. `corepack enable mypm`). A
separate PR adding the package manager to the default list can be opened a

Choose a reason for hiding this comment

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

It feels odd that a package manager would be enabled by default in corepack but not shipped enabled by default in node.js, it feels like these two things should be in lock step

Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly disagree.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Myles. I think the primary way in which users will interact with Corepack will be with the default Node.js install. So I'm curious why we would want Corepack to behave differently when installed another way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the idea was to make corepack enable without arguments relatively liberal (since it requires an explicit action from the user), vs Node.js inclusion which would require some extra vetting since it has implications on what gets added to the standard distribution (for instance, would Node.js want to ship a bun binary? #295).

In a sense, that's already the current situation: Node.js doesn't ship the Yarn / pnpm binaries by default, despite corepack enable installing them both.

Finally, this repository does not manage which package managers are distributed
with default install of Node.js. This is managed in the
[nodejs/node](https://github.com/nodejs/node) repository, refer to the
[CONTRIBUTING.md](https://github.com/nodejs/node/blob/main/CONTRIBUTING.md) over

Choose a reason for hiding this comment

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

Is there existing documentation / governance?

Copy link
Contributor

Choose a reason for hiding this comment

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

PR reviewing process is documented in nodejs/node yes, but I feel like that's not what you're asking. I suggested this phrasing so this doesn't become outdated when/if such process changes on the nodejs/node side.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Feb 27, 2024

@arcanis I think that PRs like this that involve governance (also design decisions and the like) need much more review than being opened and merged in a matter of hours. The TSC should be tagged, as well as relevant teams like @nodejs/npm and @nodejs/package-maintenance, and we should follow the standard rules for landing PRs on the main repo where they stay open for a week if they have one approval, or 48 hours with two approvals.

Alternatively we could define Corepack’s governance and design in the core repo.

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.

5 participants