search

nodejs / corepack

Zero-runtime-dependency package acting as bridge between Node projects and their package managers
MIT License
2.52k stars 165 forks source link

feat!: prompt user before downloading software #360

Closed aduh95 closed 8 months ago

aduh95 commented 8 months ago

Adds a prompt to let user validate each download . No prompt is shown when the software is already in the cache. The prompt can be opt-in/opt-out using an env variable, by default it's only shown when "not using the corepack binary" (i.e. when using the binaries created by corepack enable)

merceyz commented 8 months ago

The problem with logging anything by default is that we'll break anything that depends on the CLI output of various commands. For example yarn -v, pnpm -v, and npm -v wouldn't be guaranteed to print a valid SemVer string anymore.

styfle commented 8 months ago

Isn’t this already handled with “DEBUG=corepack” env var?

aduh95 commented 8 months ago

The issue this PR is trying to address is how can we make sure that if Node.js ships e.g. a pnpm binary with the default installer, how can we make sure the user is well aware that Corepack is involved and is downloading software from the internet. Maybe we can think of a solution that would only apply to the jumper binaries, as I think it's fair to assume folks running corepack pnpm are well aware they're using Corepack.

Isn’t this already handled with “DEBUG=corepack” env var?

The issue of that is it's opt-in, when I think what we would need would be an opt-out.

styfle commented 8 months ago

Maybe print to stderr instead of stdout?

mhdawson commented 8 months ago

@aduh95 I'm not sure a console log is enough in this case. I think the example where npx stops and asks if it's ok to download/install something is a good example of what might be.

merceyz commented 7 months ago

Note that this change breaks our own tests because Corepack now prints stuff to stderr that the tests aren't expecting. Ref https://github.com/nodejs/corepack/pull/392.

Like I wrote in https://github.com/nodejs/corepack/pull/360#issuecomment-1907115451; this change is problematic.

jgarber623 commented 7 months ago

this change is problematic.

Concur. This change broke an automated Dev Container project setup script that calls yarn install --check-files.