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 warning when using npm through Corepack #407

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 10 additions & 8 deletions README.md
Expand Up @@ -3,7 +3,7 @@
Corepack is a zero-runtime-dependency Node.js script that acts as a bridge
between Node.js projects and the package managers they are intended to be used
with during development. In practical terms, **Corepack lets you use Yarn, npm,
and pnpm without having to install them**.
and other supported package managers without having to install them**.
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove pnpm from this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mistake; I meant to remove npm and leave pnpm.


## How to Install

Expand Down Expand Up @@ -50,8 +50,8 @@ See [`CONTRIBUTING.md`](./CONTRIBUTING.md).
### When Building Packages

Just use your package managers as you usually would. Run `yarn install` in Yarn
projects, `pnpm install` in pnpm projects, and `npm` in npm projects. Corepack
will catch these calls, and depending on the situation:
projects, `pnpm install` in pnpm projects, etc. Corepack will catch these
calls, and depending on the situation:

- **If the local project is configured for the package manager you're using**,
Corepack will silently download and cache the latest compatible version.
Expand Down Expand Up @@ -79,7 +79,7 @@ Here, `yarn` is the name of the package manager, specified at version `3.2.3`,
along with the SHA-224 hash of this version for validation.
`packageManager@x.y.z` is required. The hash is optional but strongly
recommended as a security practice. Permitted values for the package manager are
`yarn`, `npm`, and `pnpm`.
`yarn` and `pnpm`.

You can also provide a URL to a `.js` file (which will be interpreted as a
CommonJS module) or a `.tgz` file (which will be interpreted as a package, and
Expand Down Expand Up @@ -161,8 +161,6 @@ alias yarn="corepack yarn"
alias yarnpkg="corepack yarnpkg"
alias pnpm="corepack pnpm"
alias pnpx="corepack pnpx"
alias npm="corepack npm"
alias npx="corepack npx"
```

On Windows PowerShell, you can add functions using the `$PROFILE` automatic
Expand All @@ -173,8 +171,6 @@ echo "function yarn { corepack yarn `$args }" >> $PROFILE
echo "function yarnpkg { corepack yarnpkg `$args }" >> $PROFILE
echo "function pnpm { corepack pnpm `$args }" >> $PROFILE
echo "function pnpx { corepack pnpx `$args }" >> $PROFILE
echo "function npm { corepack npm `$args }" >> $PROFILE
echo "function npx { corepack npx `$args }" >> $PROFILE
```

### `corepack disable [... name]`
Expand Down Expand Up @@ -299,6 +295,12 @@ There are a wide variety of networking issues that can occur while running `core
`curl [URL]` (ipv4) and `curl -6 [URL]` (ipv6) from your shell.
- Check your proxy settings (see [Environment Variables](#environment-variables)).

## Non-goals

### Official npm integration

Official npm integration is a non-goal at this time. The npm team made it clear they don't wish npm to be distributed through Corepack (some of their concerns are listed [here](https://github.com/nodejs/node/issues/50963#issuecomment-1862957925)). Corepack focuses on the use case from officially supported package managers.
Copy link
Contributor

Choose a reason for hiding this comment

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

That's no longer true since we added support for HTTP references, the goal is very much to help user run any package manager.

Copy link
Member

@styfle styfle Feb 27, 2024

Choose a reason for hiding this comment

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

Agreed. This warning might need to apply to pnpm and bun depending on how you define "officially supported" by their team. Its best to leave it out.

The log "Corepack is about to download" should be enough to differentiate Corepack usage vs calling directly.

Copy link
Member

Choose a reason for hiding this comment

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

No need to repeat the header of this section

Suggested change
Official npm integration is a non-goal at this time. The npm team made it clear they don't wish npm to be distributed through Corepack (some of their concerns are listed [here](https://github.com/nodejs/node/issues/50963#issuecomment-1862957925)). Corepack focuses on the use case from officially supported package managers.
The npm team made it clear they don't wish npm to be distributed through Corepack (some of their concerns are listed [here](https://github.com/nodejs/node/issues/50963#issuecomment-1862957925)). Corepack focuses on the use case from officially supported package managers.


## Contributing

See [`CONTRIBUTING.md`](./CONTRIBUTING.md).
Expand Down
23 changes: 23 additions & 0 deletions sources/corepackUtils.ts
@@ -1,10 +1,12 @@
import {UsageError} from 'clipanion';
import {createHash} from 'crypto';
import {once} from 'events';
import {FileHandle} from 'fs/promises';
import fs from 'fs';
import type {Dir} from 'fs';
import Module from 'module';
import path from 'path';
import {stderr, stdin} from 'process';
import semver from 'semver';

import * as engine from './Engine';
Expand Down Expand Up @@ -162,6 +164,27 @@ export async function installVersion(installTarget: string, locator: Locator, {s
url = decodeURIComponent(version);
}

if (process.env.COREPACK_ENABLE_DOWNLOAD_PROMPT === `1`) {
console.error(`Corepack is about to download ${url}`);

if (locator.name === `npm`)
console.error(`Corepack support for npm is provided on a best-effort basis and is not officially supported by the npm team`);

if (stdin.isTTY && !process.env.CI) {
stderr.write(`Do you want to continue? [Y/n] `);
stdin.resume();
const chars = await once(stdin, `data`);
stdin.pause();

// n / N
if (chars[0][0] === 0x6e || chars[0][0] === 0x4e)
throw new UsageError(`Aborted by the user`);

// Add a newline to separate Corepack output from the package manager
console.error();
}
}

// Creating a temporary folder inside the install folder means that we
// are sure it'll be in the same drive as the destination, so we can
// just move it there atomically once we are done
Expand Down
25 changes: 3 additions & 22 deletions sources/httpUtils.ts
@@ -1,8 +1,6 @@
import assert from 'assert';
import {UsageError} from 'clipanion';
import {once} from 'events';
import {stderr, stdin} from 'process';
import {Readable} from 'stream';
import assert from 'assert';
import {UsageError} from 'clipanion';
import {Readable} from 'stream';

async function fetch(input: string | URL, init?: RequestInit) {
if (process.env.COREPACK_ENABLE_NETWORK === `0`)
Expand Down Expand Up @@ -39,23 +37,6 @@ export async function fetchAsJson(input: string | URL, init?: RequestInit) {
}

export async function fetchUrlStream(input: string | URL, init?: RequestInit) {
if (process.env.COREPACK_ENABLE_DOWNLOAD_PROMPT === `1`) {
console.error(`Corepack is about to download ${input}`);
if (stdin.isTTY && !process.env.CI) {
stderr.write(`Do you want to continue? [Y/n] `);
stdin.resume();
const chars = await once(stdin, `data`);
stdin.pause();

// n / N
if (chars[0][0] === 0x6e || chars[0][0] === 0x4e)
throw new UsageError(`Aborted by the user`);

// Add a newline to separate Corepack output from the package manager
console.error();
}
}

const response = await fetch(input, init);
const webStream = response.body;
assert(webStream, `Expected stream to be set`);
Expand Down