-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(cli/config)!: breaking changes #9854
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
base: v11
Are you sure you want to change the base?
Conversation
9a0b040 to
c6117b9
Compare
b96d217 to
3145b8e
Compare
| "pnpm": major | ||
| --- | ||
|
|
||
| `pnpm config list` and `pnpm config get ''` now hide auth-related settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to how npm config list would show username and _password as (protected)
| "pnpm": major | ||
| --- | ||
|
|
||
| `pnpm config get` and `pnpm config list` no longer include non camelCase options from the workspace manifest (`pnpm-workspace.yaml`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| `pnpm config get` and `pnpm config list` no longer include non camelCase options from the workspace manifest (`pnpm-workspace.yaml`). | |
| `pnpm config get` and `pnpm config list` no longer load non camelCase options from the workspace manifest (`pnpm-workspace.yaml`). |
| The naming cases of top-level config keys have been changed for `pnpm config list [--json]` and `pnpm config get [--json] ''`. | ||
| The specifics depend on the appearance of `--json` and the classification of the top-level keys. | ||
|
|
||
| `pnpm config list` and `pnpm config get ''` (without `--json`) imitate an rc file (INI) with best effort. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear what the '' means
| `pnpm config list` and `pnpm config get ''` (without `--json`) imitate an rc file (INI) with best effort. | |
| `pnpm config list` and `pnpm config get '<key>'` (without `--json`) imitate an rc file (INI) with best effort. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, just pnpm config get then, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mixture of camel and kebab case is somewhat confusing.
I think we should make the global config file a yaml file too. In that case almost nothing should remain in kebab-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still the local .npmrc which is kebab-case. Unless we either remove the INI mode completely (pnpm config get|list would print either JSON or raw text, no INI), or remove non-npm settings (i.e. settings that is only relevant to pnpm) from the rc files, or both, there ought to be inconsistency somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make the global config file a yaml file too.
How relevant is npm to pnpm configuration? Does pnpm need npm to do auth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can kind of remove INI mode. We can read only the auth related things from the ini file and ignore anything else.
Alternatively, we could move everything to our own config file but then we need to pass npm CLI the settings somehow. Through npm_config_ env variables for instance. Also, we'd have to implement npm login ourself.
The reason we need the auth settings in ini is because we use npm CLI directly for some commands. Like publish, dist-tag, unpublish, login.
I think it is better to use the npm commands for these.
| import { types } from './types.js' | ||
|
|
||
| export const isRcSetting = (kebabKey: string, extraTypes: Record<string, unknown> = {}): boolean => | ||
| kebabKey.startsWith('@') || kebabKey.startsWith('//') || kebabKey in npmTypes || kebabKey in types || kebabKey in extraTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd limit this to just these:
| kebabKey.startsWith('@') || kebabKey.startsWith('//') || kebabKey in npmTypes || kebabKey in types || kebabKey in extraTypes | |
| kebabKey.startsWith('@') || kebabKey.startsWith('//') |
- any other auth related settings.
TODO:
pnpm config list.packages) from rc files.rawConfig.pnpm config get <array>output JSON.pnpm config listpnpm config list --jsonshould print all top-level keys in camelCase.pnpm config list --jsonshould printregistriesinstead ofregistryand@scope:registry.pnpm config list [--ini]should print workspace-specific keys in camelCase and other in kebab-case.pnpm config list --jsonshould print almost all as camelCase except@scope:registryand//domain:auth.pnpm config listshould print rc keys as kebab-case and workspace-specific keys as camelCase.pnpm get [--json] ''.rawConfigas camelCase instead.pnpm config get.pnpm config set.pnpm config setrefuse kebab-case workspace-specific settings.pnpm config list.pnpm config get.pnpm config set.pnpm-workspace.yaml.isCamelCase.test.ts.RemoveisStrictlyKebabCase.RenamecheckCases.tstoisCamelCase.ts.Excluded because they are not breaking changes:
--format json|ini.--iniis an alias for--format ini.--jsonis an alias for--format json.Cancelled TODO: