fix(config): handle configDependencies fetch during config commands#10688
fix(config): handle configDependencies fetch during config commands#10688ishan8351 wants to merge 1 commit into
Conversation
|
pnpm audit is failing currently, but it is unrelated to changes made in this PR |
| }) | ||
| config.cliOptions = cliOptions | ||
| if (config.configDependencies) { | ||
| if (config.configDependencies && !opts.ignoreConfigDependencies) { |
There was a problem hiding this comment.
maybe run it anyway but ignore the errors
There was a problem hiding this comment.
Pull request overview
This PR addresses a pnpm CLI crash where running pnpm config set in a workspace with configDependencies could trigger an early config-deps fetch (before the auth token is written), leading to 401 failures.
Changes:
- Adds an
ignoreConfigDependenciesoption to@pnpm/cli-utilsgetConfig()to skip installingconfigDependencies. - Enables that option from the CLI entrypoint for the
configcommand. - Adds a changeset for
@pnpm/cli-utils.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pnpm/src/main.ts | Passes ignoreConfigDependencies based on the active command to avoid early config-deps installs. |
| cli/cli-utils/src/getConfig.ts | Introduces the ignoreConfigDependencies option and guards the config-deps install. |
| .changeset/major-hats-press.md | Declares a patch bump for @pnpm/cli-utils related to config-deps fetching behavior. |
Comments suppressed due to low confidence (1)
cli/cli-utils/src/getConfig.ts:37
- When
opts.ignoreConfigDependenciesis true,installConfigDeps()is skipped, but later the code still derives pnpmfile paths fromconfig.configDependenciesand passes them torequireHooks().requireHooks()throwsPNPMFILE_NOT_FOUNDfor non-optional pnpmfiles, sopnpm config/pnpm setcan now fail ifconfigDependenciesinclude a plugin (e.g.pnpm-plugin-*) whose pnpmfile isn't installed yet. Consider also guarding the pnpmfile injection with!opts.ignoreConfigDependencies(or marking these derived pnpmfiles as optional) to avoid breaking config commands.
if (config.configDependencies && !opts.ignoreConfigDependencies) {
const store = await createStoreController(config)
await installConfigDeps(config.configDependencies, {
registries: config.registries,
rootDir: config.lockfileDir ?? config.rootProjectManifestDir,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
7047c5b to
e6ca98c
Compare
|
updated |
Running pnpm config set with configDependencies triggers installConfigDeps before saving the new config, causing a crash as it tries to download dependencies before auth token is written to .npmrc. This commit adds an catchConfigDependenciesErrors flag to getConfig. During config commands (config, set, get), if an error occurs, catches and logs it and proceeds. Fixes pnpm#10684
e6ca98c to
5377b33
Compare
|
@zkochan and @ishan8351, is there something blocking this PR, or did it just incidentally fall by the wayside? I ask because we are facing a similar issue at work: we are introducing a shared config dependency for security hardening, but said config dependency (and our packages in general) is behind auth. Thus, While we could work around it with a |
|
Hey @fpapado, this is one of a bunch of PRs I opened, then I got occupied somewhere else. Please do take a look, if my implementation's alright and solves your problem, I'll try to finish it. Or you're welcome to take it over. |
Yay, thank you for the response! Main had drifted a bit since this PR was opened, so I made a new set of changes here with a few more integration tests here #11362 🗒️ |
Running pnpm config set with configDependencies triggers installConfigDeps before saving
the new config, causing a crash as it tries to download dependencies before auth token is
written to .npmrc.
This PR adds an catchConfigDependenciesErrors flag to getConfig. During config commands
(config, set, get), if an error occurs, catches and logs it and proceeds.
Fixes #10684