Skip to content

feat(cli/config)!: breaking changes#9854

Merged
zkochan merged 92 commits intov11from
cli-config-breaking-changes
Oct 28, 2025
Merged

feat(cli/config)!: breaking changes#9854
zkochan merged 92 commits intov11from
cli-config-breaking-changes

Conversation

@KSXGitHub
Copy link
Copy Markdown
Contributor

@KSXGitHub KSXGitHub commented Aug 14, 2025

TODO:

  • Remove auth information from pnpm config list.
  • Stop reading workspace specific settings (such as packages) from rc files.
  • Exclude workspace specific settings from rawConfig.
  • Make pnpm config get <array> output JSON.
  • Undo 529f9bd, merge the workspace specific settings into rawConfig as camelCase instead.
    • Consider reverting 3b10ab5 or correcting the changeset.
    • Implement: Changing case during initial load.
    • Implement: Changing case for pnpm config get.
    • Implement: Changing case for pnpm config set.
    • Implement: Make pnpm config set refuse kebab-case workspace-specific settings.
    • Implement: Changing case for pnpm config list.
    • Implement: Exclude unknown rc fields from pnpm config get.
    • Implement: Exclude unknown rc fields from pnpm config set.
    • Implement: Do not read non-camelCase fields from pnpm-workspace.yaml.
    • Create isCamelCase.test.ts.
    • Remove isStrictlyKebabCase.
    • Rename checkCases.ts to isCamelCase.ts.
    • Fix existing tests.
    • Test: Generic.
    • Test: Convert case.
    • Test: Ignore non-rc kebab-case keys.
    • Test: Ignore camelCase keys from rc files.
    • Changeset.
  • Stop printing things in INI format.
  • Stop loading non auth and non registry settings from rc files.
  • Fix pnpm config set.
  • All non auth settings should be printed as camelCase.
  • Resolve TODOs from:
    // NOTE: this block of code in this location is pointless.
    // TODO: move this block of code to after the code that loads pnpm-workspace.yaml.
    // TODO: unskip test `getConfig() sets mergeGiBranchLockfiles when branch matches mergeGitBranchLockfilesBranchPattern`.
  • Replace npm_config_* with pnpm_config_* for pnpm specific settings: https://gist.github.com/KSXGitHub/f4c4bf4294b32f19fcd0c0ab9bb250cc
  • Changeset.
Excluded because they are not breaking changes:
  • Explicit INI format.
    • --format json|ini.
    • --ini is an alias for --format ini.
    • --json is an alias for --format json.
Cancelled TODO:
  • More consistent casing for pnpm config list
    • pnpm config list --json should print all top-level keys in camelCase.
    • pnpm config list --json should print registries instead of registry and @scope:registry.
    • pnpm config list [--ini] should print workspace-specific keys in camelCase and other in kebab-case.
    • pnpm config list --json should print almost all as camelCase except @scope:registry and //domain:auth.
    • pnpm config list should print rc keys as kebab-case and workspace-specific keys as camelCase.
    • The same for pnpm get [--json] ''.
    • Test.
    • Changeset.
  • Test 529f9bd
    • Fix existing tests.
    • Test: Generic.
    • Test: Overriding.
    • Test: Convert case.

@zkochan zkochan added this to the v11.0 milestone Aug 14, 2025
@KSXGitHub KSXGitHub force-pushed the cli-config-breaking-changes branch from 9a0b040 to c6117b9 Compare August 14, 2025 10:40
@KSXGitHub KSXGitHub force-pushed the cli-config-breaking-changes branch from b96d217 to 3145b8e Compare August 21, 2025 13:29
@KSXGitHub KSXGitHub force-pushed the cli-config-breaking-changes branch from b39e84d to b64ea5e Compare October 27, 2025 10:52
* fix: `filter` on `pnpm-workspace.yaml`

* docs: changeset

* fix: actual fix

* fix: don't set default on config
@KSXGitHub KSXGitHub requested a review from Copilot October 27, 2025 15:34
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements breaking changes to how pnpm handles configuration settings, moving workspace-specific settings from .npmrc files to pnpm-workspace.yaml files, and standardizing the naming conventions between the two formats. The changes focus on separating concerns between npm-compatible settings (in .npmrc) and pnpm-specific workspace settings (in pnpm-workspace.yaml).

Key changes:

  • Configuration settings now strictly enforce that workspace-specific settings must be in camelCase in pnpm-workspace.yaml, while rc options remain in kebab-case
  • Auth-related information is now censored in pnpm config list and pnpm config get commands
  • Array values returned by pnpm config get are now properly formatted as JSON arrays instead of comma-separated strings

Reviewed Changes

Copilot reviewed 44 out of 46 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
packages/naming-cases/* New package providing utilities to validate and check naming case conventions (camelCase, kebab-case)
config/plugin-commands-config/src/* Updated config command handlers to enforce naming conventions, censor protected settings, and properly format output
config/config/src/* Modified config loading to filter non-auth settings from rc files and properly handle workspace manifest settings
pnpm/test/* Updated tests to use pnpm-workspace.yaml instead of .npmrc for workspace-specific settings
config/config/test/* Added comprehensive tests for the new configuration loading behavior
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)

config/plugin-commands-config/src/configSet.ts:1

  • The test case was changed from foo=bar=qar to lockfile-dir=foo=bar, but this changes the nature of the test. The original test appeared to verify handling of arbitrary keys with multiple = signs, while the new test uses a known config key. Consider adding a separate test case for the original scenario to maintain coverage of edge cases.
import fs from 'fs'

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +290 to +292
// NOTE: this block of code in this location is pointless.
// TODO: move this block of code to after the code that loads pnpm-workspace.yaml.
// TODO: unskip test `getConfig() sets mergeGiBranchLockfiles when branch matches mergeGitBranchLockfilesBranchPattern`.
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

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

The TODO comment identifies a known issue but the code is left in its current (incorrect) location. This creates technical debt and causes at least one test to be skipped. The code should be moved to the correct location as part of this PR, or a separate issue should be created to track this work.

Copilot uses AI. Check for mistakes.
@zkochan
Copy link
Copy Markdown
Member

zkochan commented Oct 28, 2025

Let's aim at merging the PR this week. If something is not done yet, it can be done in a new PR. Just have the existing tests pass.

@KSXGitHub KSXGitHub force-pushed the cli-config-breaking-changes branch from be9aed0 to 5b34702 Compare October 28, 2025 13:28
@KSXGitHub KSXGitHub force-pushed the cli-config-breaking-changes branch from 4b9964c to 8b2921b Compare October 28, 2025 14:11
@KSXGitHub KSXGitHub requested a review from zkochan October 28, 2025 14:39
@zkochan
Copy link
Copy Markdown
Member

zkochan commented Oct 28, 2025

Overall seems fine to me. I might review it again after merge.

@KSXGitHub KSXGitHub marked this pull request as ready for review October 28, 2025 15:34
@zkochan zkochan merged commit ae43ac7 into v11 Oct 28, 2025
15 of 19 checks passed
@zkochan zkochan deleted the cli-config-breaking-changes branch October 28, 2025 16:09
Comment on lines +6 to +7
`pnpm config list` and `pnpm config get` (without argument) now show top-level keys as camelCase.
Exception: Keys that start with `@` or `//` would be preserved (their cases don't change).
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zkochan I just discovered that this particular changeset is a misinformation. This is a change I planned to implement though, so it will be included in a different PR.

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 this pull request may close these issues.

3 participants