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

Loosen packageManager field to support any package on npm #354

Closed
styfle opened this issue Jan 16, 2024 · 10 comments · Fixed by #359
Closed

Loosen packageManager field to support any package on npm #354

styfle opened this issue Jan 16, 2024 · 10 comments · Fixed by #359

Comments

@styfle
Copy link
Member

styfle commented Jan 16, 2024

Could we consider changing the behavior of Corepack to loosen the definition of the packageManager field?

That way you could define any npm package name in the packageManager field and Corepack would fetch from the npm registry if its not found in the hardcoded known package manager list.

Examples

Pros

  1. Less gatekeeping because a new package manager can be used immediately without changing Corepack's src code.
  2. A little more intuitive since the packageManager field feels a lot more like dependencies

Cons

  1. We might lose the ability to catch typos since today those are caught with Usage Error: Unsupported package manager specification.
  2. We might lose the ability to catch wrong package manager install with Usage Error: This project is configured to use

I think number 1 is okay but number 2 sounds problematic. Perhaps number 2 still requires a PR to Corepack.

Related

@arcanis
Copy link
Contributor

arcanis commented Jan 16, 2024

Maybe, with a caveat. Corepack isn't intended to be a package manager. As a result, any tool it installs must not have any dependency.

We might lose the ability to catch typos since today those are caught with Usage Error: Unsupported package manager specification.

If we do this, I believe "generic" packages should be targeted with a dedicated syntax. Something like:

{
    "packageManager": "pkg:[email protected]"
}

Otherwise there'll be a problem with Yarn, which currently retrieves the package from one of two different sources depending on the version (even if we switched Yarn Modern to npm releases, it'd still come from two different packages; we don't want to publish modern releases on the yarn package so as not to break any current user).

This would also let us catch typos on the most popular options.

@styfle
Copy link
Member Author

styfle commented Jan 16, 2024

Maybe, with a caveat. Corepack isn't intended to be a package manager. As a result, any tool it installs must not have any dependency.

Oh thats good to know. Looks like bun has 0 deps but cnpm has 11. Definitely need to document that caveat if we add this behavior. I'm almost wondering if packageManager should accept a url to make it even more generic.

Otherwise there'll be a problem with Yarn, which currently retrieves the package from one of two different sources depending on the version

My suggestion is to not change the behavior of Yarn/pnpm/npm as it is today. Only to open the door to other package managers. Basically, check config.json, and fallback to fetching from the registry if there is no match.

@aduh95
Copy link
Contributor

aduh95 commented Jan 16, 2024

Maybe we could accept either a bare specifier – in which case Corepack checks the config.json to download the right archive – or a URL. I think the URL should address the issue at hand (anyone can use their package manager, or even easily fork an existing package manager) without forcing them to store it on any specific repository. It also makes it clearer that dependencies won't be downloaded. Not sure how Corepack would know what name to use for the binary though 🤔

@styfle
Copy link
Member Author

styfle commented Jan 16, 2024

Not sure how Corepack would know what name to use for the binary though

Since the url is going to point to a file download, how about using the Content-Disposition header?

For example, bun could be:

Content-Disposition: attachment; filename="bun"

Would this work with caching? I guess the url should have the version number in it and corepack could cache by url string?

@arcanis
Copy link
Contributor

arcanis commented Jan 16, 2024

Since the packageManager field is only read when a shim is called, how would Corepack know that a specific shim needs to be installed in the PATH in the first place? For instance, what would make bun globally available so that bun install would read the packageManager field and install bun from the url configured there?

@aduh95
Copy link
Contributor

aduh95 commented Jan 16, 2024

Since the packageManager field is only read when a shim is called, how would Corepack know that a specific shim needs to be installed in the PATH in the first place? For instance, what would make bun globally available so that bun install would read the packageManager field and install bun from the url configured there?

The way I imagine that would be that those folks would do corepack enable bun, and that would create the jumper script. That doesn't seem to be very different from what we have now.

@aduh95
Copy link
Contributor

aduh95 commented Jan 17, 2024

Actually we could have both the "binary name" and the URL:

{
  "packageManager": "aduh95-special-pm@http://example.com/archive.tgz#sha224.xxxxxxxxxxx"
}

If someone tries to run corepack yarn install, Corepack would be able to throw an error, and if you run corepack aduh95-special-pm install, it would "just work" – or if you have an aduh95-special-pm binary in your path installed by Corepack.

That got me thinking, should we support the following:

{
  "packageManager": "pnpm@http://example.com/archive.tgz#sha224.xxxxxxxxxxx"
}

And I think by default Corepack should refuse to do so, so folks by default would run official binaries of package managers that are in config.json, but we should have an env variable to allow to use unofficial ones.

@arcanis
Copy link
Contributor

arcanis commented Jan 17, 2024

I like this idea. We could do something like this:

$ pkgmgr
Command not found: pkgmgr

$ corepack enable pkgmgr

$ pkgmgr
Usage Error: This command can only be used within a project configured to use pkgmgr; check https://... for more details.

$ corepack use pkgmgr
We're going to create a local project using the following package manager:

Name: pkgmgr
Version: 3.0.0
Website url: https://pkgmgr.com
Source url: https://registry.npmjs.com/pkgmgr/-/pkgmgr-3.0.0.tgz

Is this what you intend? [Y/n]

$ pkgmgr
Project installed

@nodejs nodejs deleted a comment from merceyz Jan 17, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jan 17, 2024

Worth noting that, at that time, none of the Corepack commands are "interactive", meaning you type the command, Corepack will execute it without requiring any additional step from the user. The suggested workflow breaks this, which may or may not be worth it.
We might find the following to be more fitted:

$ corepack use pkgmgr
Usage Error: Invalid package manager specification in CLI arguments. Consider adding the `--from-npm` flag if you meant to use the npm package `pkgmgr` as your package manager
$ corepack use pkgmgr --from-npm
$ cat package.json
{ "packageManager": "pkgmgr@https://registry.npmjs.com/pkgmgr/-/pkgmgr-3.0.0.tgz#sha1.xxxxxxxx" }
$ pkgmgr
Project installed

@mhdawson
Copy link
Member

I liked the what @arcanis had in #354 (comment) because it makes it clear what is going to be installed, where from, and the user needs to confirm that they want it installed.

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 a pull request may close this issue.

4 participants