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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 27, 2024
Merged

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

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b079203
Adds a note explicitating how to add new package managers
arcanis Feb 27, 2024
9213bde
Update CONTRIBUTING.md
arcanis Feb 27, 2024
8b8f877
Update CONTRIBUTING.md
arcanis Feb 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,32 @@ The `dist/` directory now contains the corepack build and the shims.
Call `node ./dist/corepack --help` and behold.
You can also run the tests with `yarn test`.

# Adding a new package manager

New package managers can be added by editing the following files:

- [`config.json`](./config.json),
- [`.github/workflows/sync.yml`](./.github/workflows/sync.yml) that keeps pinned
versions up-to-date,
- [`package.json`](./package.json) to add to add the added shims to the list of
`"publishConfig/bin"` and `"executableFiles"`,
- [`sources/types.ts`](./sources/types.ts) to add the package manager to the
`SupportedPackageManagers` enum,
- [`tests/main.test.ts`](./tests/main.test.ts) tests to ensure the added package
manager works as expected.

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
Copy link

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.

couple of months after the new package manager was introduced.

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
Copy link

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.

there for more information.

## Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:
Expand Down