feat: introduce 'location' parameter for npm config command #3471
Conversation
|
|
||
| * Default: "user" unless `--global` is passed, which will also set this value | ||
| to "global" | ||
| * Type: "global", "user", or "project" |
There was a problem hiding this comment.
leaving that out was intentional, feels like the built-in use case should be a file overwritten by packagers and not something a user would interactively change on their own to me. I'm definitely open to discuss it though
There was a problem hiding this comment.
I’d expect it to be undocumented regardless, but it seems pretty useful for debugging the source of an unexpected default config (for “get”), and it seems weird to allow it for get and not for set.
There was a problem hiding this comment.
get and list both ignore the parameter entirely. they aren't location specific, they return whatever the most prioritized value(s) is/are. we could allow for the location flag on them as a means of saying "show me only data from this location" in which case I agree exposing built-in would be appropriate. if we did that, though, we would have to remove the default value for the location and have separate behaviors in the commands for a value of null which i don't love...
There was a problem hiding this comment.
ah hm, that's a fair point.
if it's only going to work in set then omitting built-in makes sense; but it seems more useful to me if it works for all of get/list/set, and includes built-in.
There was a problem hiding this comment.
to address the issue of debugging the source of an unexpected config value, how about we allow npm config list to accept config keys for which it will list everywhere that key is defined and what overwrote what.
so npm config get key will continue to return a single value representing the value of key from its highest prioritized source, and npm config list -l key will list every value that key has in any source in a way that makes it clear why it was overridden from other sources.
There was a problem hiding this comment.
For list, that sounds great.
| // for now, this is to avoid inadvertently causing any breakage. the value of | ||
| // global, however, does modify this flag. | ||
| flatten (key, obj, flatOptions) { | ||
| // if global is set, we override ourselves |
There was a problem hiding this comment.
this is here just to maintain the previous behavior of npm config respecting --global, and since at this point that's all this flag even goes to i think it's fine to not warn on it.
darcyclarke
added
Release 7.x
|
This is an iterative change towards where we want to be, param name is fine it's been bikeshedded enough already. We can update the |
PR-URL: #3471 Credit: @nlf Close: #3471 Reviewed-by: @wraithgar
|
Landed in 98905ae |
| if (obj.global) | ||
| obj.location = 'global' |
There was a problem hiding this comment.
This logic doesn't appear to be working correctly. See #3572.
this may incite some further bike shedding, both for the name of the parameter and its allowed values. for now, i elected to take the simplest solution of having one
locationparameter instead of making it specific to the use (i.e.--config-location) and for the values i went with what@npmcli/configuses since it means i can just pass the config straight through.it may warrant some further documentation about exactly what files each value refers to, but i wasn't sure where the appropriate place to put that was so this is a starting point to push the discussion along.